DevTools: Fix flickering hint when typing in TextPrompt (issue 2285993002 by einbinder@chromium.org)

[einb...@chromium.org]

Reviewers: lushnikov
CL: https://codereview.chromium.org/2285993002/

Message:
Ptal

Description:
DevTools: Fix flickering hint when typing in TextPrompt

BUG=none

Affected files (+43, -13 lines):
M third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js


Index: third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
diff --git a/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js b/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
index 839f4b9bc35705059205e754168429f1a2463eca..5f93fe58556ec66b6c864d8ab1a0b496a40d80f1 100644
--- a/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
+++ b/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
@@ -45,6 +45,7 @@ WebInspector.TextPrompt = function(completions, stopCharacters)
this._completionStopCharacters = stopCharacters || " =:[({;,!+-*/&|^<>.";
this._autocompletionTimeout = WebInspector.TextPrompt.DefaultAutocompletionTimeout;
this._title = "";
+ this._previousText = "";
}

WebInspector.TextPrompt.DefaultAutocompletionTimeout = 250;
@@ -189,6 +190,7 @@ WebInspector.TextPrompt.prototype = {
} else {
this._element.textContent = x;
}
+ this._previousText = this.userEnteredText();

this.moveCaretToEndOfPrompt();
this._element.scrollIntoView();
@@ -287,7 +289,14 @@ WebInspector.TextPrompt.prototype = {
*/
_updateAutoComplete: function(force)
{
- this.clearAutoComplete();
+ var singleCharInput = false;
+ var singleCharDelete = false;
+ var text = this.userEnteredText();
+ if (this._previousText === text.substring(0, text.length - 1))
+ singleCharInput = true;
+ else if (this._previousText.substring(0, this._previousText.length - 1) === text)
+ singleCharDelete = true;
+ this.clearAutoComplete(false, singleCharInput, singleCharDelete);
this.autoCompleteSoon(force);
},

@@ -361,6 +370,7 @@ WebInspector.TextPrompt.prototype = {
{
if (this._needUpdateAutocomplete)
this._updateAutoComplete();
+ this._previousText = this.userEnteredText();
},

/**
@@ -379,24 +389,41 @@ WebInspector.TextPrompt.prototype = {

/**
* @param {boolean=} includeTimeout
+ * @param {boolean=} singleCharInput
+ * @param {boolean=} singleDeleteChar
*/
- clearAutoComplete: function(includeTimeout)
+ clearAutoComplete: function(includeTimeout, singleCharInput, singleDeleteChar)
{
- if (includeTimeout && this._completeTimeout) {
- clearTimeout(this._completeTimeout);
- delete this._completeTimeout;
- }
+
+ if (includeTimeout)
+ this._clearAutocompleteTimeout();
+
delete this._waitingForCompletions;

if (!this.autoCompleteElement)
return;
-
- this.autoCompleteElement.remove();
- delete this.autoCompleteElement;
+ if (singleCharInput) {
+ this._hintPrefix += this.autoCompleteElement.textContent.charAt(0);
+ this.autoCompleteElement.textContent = this.autoCompleteElement.textContent.substring(1);
+ } else if (singleDeleteChar && this._hintPrefix) {
+ this.autoCompleteElement.textContent = this._hintPrefix.charAt(this._hintPrefix.length - 1) + this.autoCompleteElement.textContent;
+ this._hintPrefix = this._hintPrefix.substring(0, this._hintPrefix.length - 1);
+ } else {
+ this.autoCompleteElement.remove();
+ delete this.autoCompleteElement;
+ }
delete this._userEnteredRange;
delete this._userEnteredText;
},

+ _clearAutocompleteTimeout: function()
+ {
+ if (this._completeTimeout) {
+ clearTimeout(this._completeTimeout);
+ delete this._completeTimeout;
+ }
+ },
+
/**
* @param {boolean=} force
*/
@@ -413,7 +440,7 @@ WebInspector.TextPrompt.prototype = {
*/
complete: function(force, reverse)
{
- this.clearAutoComplete(true);
+ this._clearAutocompleteTimeout();
var selection = this._element.getComponentSelection();
var selectionRange = selection && selection.rangeCount ? selection.getRangeAt(0) : null;
if (!selectionRange)
@@ -428,11 +455,11 @@ WebInspector.TextPrompt.prototype = {
else if (!force) {
// BUG72018: Do not show suggest box if caret is followed by a non-stop character.
var wordSuffixRange = selectionRange.startContainer.rangeOfWord(selectionRange.endOffset, this._completionStopCharacters, this._element, "forward");
- if (wordSuffixRange.toString().length)
+ if (wordSuffixRange.toString().length + this.userEnteredText().length - this.text().length)
shouldExit = true;
}
if (shouldExit) {
- this.hideSuggestBox();
+ this._removeSuggestionAids();
return;
}

@@ -526,7 +553,7 @@ WebInspector.TextPrompt.prototype = {
}

if (!this._waitingForCompletions || !annotatedCompletions.length) {
- this.hideSuggestBox();
+ this._removeSuggestionAids();
return;
}

@@ -566,8 +593,11 @@ WebInspector.TextPrompt.prototype = {
var prefixTextNode = createTextNode(prefixText);
fullWordRange.insertNode(prefixTextNode);

+ if (this.autoCompleteElement)
+ this.autoCompleteElement.remove();
this.autoCompleteElement = createElementWithClass("span", "auto-complete-text");
this.autoCompleteElement.textContent = suffixText;
+ this._hintPrefix = completionText.substring(0, wordPrefixLength);

prefixTextNode.parentNode.insertBefore(this.autoCompleteElement, prefixTextNode.nextSibling);

[lush...@chromium.org]

Could you please also briefly describe in the issue description the root cause
of this poor behavior?


https://codereview.chromium.org/2285993002/diff/1/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
(right):

https://codereview.chromium.org/2285993002/diff/1/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode395
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:395:
clearAutoComplete: function(includeTimeout, singleCharInput,
singleDeleteChar)
why are these arguments a part of public interface? They feel like an
implementation detail

https://codereview.chromium.org/2285993002/

[einb...@chromium.org]

Ptal

https://codereview.chromium.org/2285993002/diff/1/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
(right):

https://codereview.chromium.org/2285993002/diff/1/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode395
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:395:
clearAutoComplete: function(includeTimeout, singleCharInput,
singleDeleteChar)

On 2016/08/29 at 22:10:41, lushnikov wrote:
> why are these arguments a part of public interface? They feel like an
implementation detail

Done.

https://codereview.chromium.org/2285993002/

[lush...@chromium.org]

https://codereview.chromium.org/2285993002/diff/20001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
(right):

https://codereview.chromium.org/2285993002/diff/20001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode293
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:293: if
(this._autocompleteElement) {
what is this._autocompleteElement? AFAIU it is never defined.

How does this work altogether then?..

https://codereview.chromium.org/2285993002/

[einb...@chromium.org]

I added a test. ptal

https://codereview.chromium.org/2285993002/diff/20001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
(right):

https://codereview.chromium.org/2285993002/diff/20001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode293
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:293: if
(this._autocompleteElement) {

On 2016/08/31 at 15:11:16, lushnikov wrote:
> what is this._autocompleteElement? AFAIU it is never defined.
>
> How does this work altogether then?..

Was a typo in a refactor. Fixed it.

https://codereview.chromium.org/2285993002/

[lush...@chromium.org]

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
(right):

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode293
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:293: if
(this.autoCompleteElement) {
can we hide autoCompleteElement inside text prompt in a follow-up? It's
a pity someone reaches right into it.

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode295
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:295: if
(this._previousText === text.substring(0, text.length - 1)) {
can we generalize this to the "this.userEnteredText() is prefix of
previous text"?

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode299
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:299: }
nit: else on the same line as "}"

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode402
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:402:
clearAutoComplete: function(includeTimeout)
I'm confused with TextPrompt API.

So there are three methods which in some way hide autocomplete:
- clearAutoComplete - cleans grey text
- hideSuggestBox - hides suggest box
- _removeCompletionAids - does both clearAutoComplete and hideSuggestBox

In which situation clients of TextPrompt need to call either
clearAutoComlpete or hideSuggestBox, and
not both?

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode404
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:404: if
(includeTimeout)
why would anyone ever want to call this with includeTimeout === false?

https://codereview.chromium.org/2285993002/

[einb...@chromium.org]

ptal

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
(right):

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode293
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:293: if
(this.autoCompleteElement) {

On 2016/09/01 at 21:25:33, lushnikov wrote:
> can we hide autoCompleteElement inside text prompt in a follow-up?
It's a pity someone reaches right into it.

Yes, I am planning a TextPrompt rewrite/refactor.

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode295
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:295: if
(this._previousText === text.substring(0, text.length - 1)) {

On 2016/09/01 at 21:25:33, lushnikov wrote:
> can we generalize this to the "this.userEnteredText() is prefix of
previous text"?

No, it's checking that exactly one character was deleted.

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode299
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:299: }

On 2016/09/01 at 21:25:33, lushnikov wrote:
> nit: else on the same line as "}"

Done.

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode402
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:402:
clearAutoComplete: function(includeTimeout)

On 2016/09/01 at 21:25:33, lushnikov wrote:
> I'm confused with TextPrompt API.
>
> So there are three methods which in some way hide autocomplete:
> - clearAutoComplete - cleans grey text
> - hideSuggestBox - hides suggest box
> - _removeCompletionAids - does both clearAutoComplete and
hideSuggestBox
>
> In which situation clients of TextPrompt need to call either
clearAutoComlpete or hideSuggestBox, and
> not both?

I think we can get away with always hiding SuggestBox.

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode404
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:404: if
(includeTimeout)

On 2016/09/01 at 21:25:33, lushnikov wrote:
> why would anyone ever want to call this with includeTimeout === false?

I don't think there is a good reason.

https://codereview.chromium.org/2285993002/

[lush...@chromium.org]

there's a lot going on here; can we split this in patches?

https://codereview.chromium.org/2285993002/

[einb...@chromium.org]

I merged with the other patch. PTAL

https://codereview.chromium.org/2285993002/

[lush...@chromium.org]

https://codereview.chromium.org/2285993002/diff/80001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js
(right):

https://codereview.chromium.org/2285993002/diff/80001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode293
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:293: }

else if (this._previousText.substring(0, this._previousText.length - 1)

=== text) {
let's use .startsWith?

https://codereview.chromium.org/2285993002/diff/80001/third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js#newcode301
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:301:
delete this._waitingForCompletions;
why would you need to delete this?

The _waitingForCompletions flag also doesn't do its job properly - it is
supposed to cancel the
completionsReady callback, but it is not hard to model the situation
when it doesn't.

Let's fix in a separate patch?

https://codereview.chromium.org/2285993002/