DevTools: Fix flickering hint when typing in TextPrompt

DevTools: Fix flickering hint when typing in TextPrompt
The grey hint text in the TextPrompt was being removed on every keystroke, and then  re-added the next frame if it was still needed for the suggestion.

BUG=none

Committed: https://crrev.com/37a665db550b7afc5175dee09596a46a8fcf6697
Cr-Commit-Position: refs/heads/master@{#426681}

[einbinder, 2016-08-27 00:36:10]

einbinder@chromium.org changed reviewers:
+ lushnikov@chromium.org

[einbinder, 2016-08-27 00:36:10]

Ptal

[einbinder, 2016-08-27 01:37:10]

The CQ bit was checked by einbinder@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-27 01:37:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-27 03:19:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-27 03:19:36]

Dry run: This issue passed the CQ dry run.

[lushnikov, 2016-08-29 22:10:41]

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/d...
File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js (right):

https://codereview.chromium.org/2285993002/diff/1/third_party/WebKit/Source/d...
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

[einbinder, 2016-08-30 21:50:37]

Description was changed from

==========
DevTools: Fix flickering hint when typing in TextPrompt

BUG=none
==========

to

==========
DevTools: Fix flickering hint when typing in TextPrompt

The grey hint text in the TextPrompt was being removed on every keystroke, and
then  re-added the next frame if it was still needed for the suggestion.

BUG=none
==========

[einbinder, 2016-08-30 22:22:35]

Ptal

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

https://codereview.chromium.org/2285993002/diff/1/third_party/WebKit/Source/d...
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.

[einbinder, 2016-08-30 22:56:33]

Description was changed from

==========
DevTools: Fix flickering hint when typing in TextPrompt

The grey hint text in the TextPrompt was being removed on every keystroke, and
then  re-added the next frame if it was still needed for the suggestion.

BUG=none
==========

to

==========
DevTools: Fix flickering hint when typing in TextPrompt
The grey hint text in the TextPrompt was being removed on every keystroke, and
then  re-added the next frame if it was still needed for the suggestion.

BUG=none
==========

[lushnikov, 2016-08-31 15:11:16]

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

https://codereview.chromium.org/2285993002/diff/20001/third_party/WebKit/Sour...
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?..

[einbinder, 2016-08-31 21:19:01]

I added a test. ptal

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

https://codereview.chromium.org/2285993002/diff/20001/third_party/WebKit/Sour...
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.

[lushnikov, 2016-09-01 21:25:33]

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

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Sour...
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/Sour...
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/Sour...
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/Sour...
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/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:404: if
(includeTimeout)
why would anyone ever want to call this with includeTimeout === false?

[einbinder, 2016-09-01 23:28:46]

ptal

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

https://codereview.chromium.org/2285993002/diff/40001/third_party/WebKit/Sour...
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/Sour...
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/Sour...
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/Sour...
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/Sour...
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.

[lushnikov, 2016-09-02 21:55:15]

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

[einbinder, 2016-09-06 23:56:23]

I merged with the other patch. PTAL

[lushnikov, 2016-09-07 22:11:01]

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

https://codereview.chromium.org/2285993002/diff/80001/third_party/WebKit/Sour...
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/Sour...
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?

[lushnikov, 2016-10-13 23:56:03]

Description was changed from

==========
DevTools: Fix flickering hint when typing in TextPrompt
The grey hint text in the TextPrompt was being removed on every keystroke, and
then  re-added the next frame if it was still needed for the suggestion.

BUG=none
==========

to

==========
DevTools: Fix flickering hint when typing in TextPrompt
The grey hint text in the TextPrompt was being removed on every keystroke, and
then  re-added the next frame if it was still needed for the suggestion.

BUG=none
==========

[lushnikov, 2016-10-13 23:56:03]

lushnikov@chromium.org changed reviewers:
- lushnikov@chromium.org

[einbinder, 2016-10-18 00:57:33]

einbinder@chromium.org changed reviewers:
+ lushnikov@chromium.org

[einbinder, 2016-10-18 00:57:33]

http://crbug.com/645665 got fixed, so we can land this.

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

https://codereview.chromium.org/2285993002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:293: } else if
(this._previousText.substring(0, this._previousText.length - 1) === text) {
On 2016/09/07 at 22:11:01, lushnikov wrote:
> let's use .startsWith?

Done.

https://codereview.chromium.org/2285993002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:301: delete
this._waitingForCompletions;
On 2016/09/07 at 22:11:01, lushnikov wrote:
> 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?

Done.

[lushnikov, 2016-10-18 18:50:43]

"I'm confused why is this not updated" (Joel)

[einbinder, 2016-10-19 21:57:26]

ptal

[lushnikov, 2016-10-19 23:36:47]

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

https://codereview.chromium.org/2285993002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:286:
_updateAutoComplete: function(force)
let's get rid of this method - it's confusing

https://codereview.chromium.org/2285993002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:289: if
(this._autocompleteElement) {
var hasCommonPrefix = text.startsWith(this._previousText) ||
this._previousText.startsWith(text);
var text = this.userEnteredText();
if (this._autocompleteElement && hasCommonPrefix)
    this._autocompleteElement.textContent =
this._currentHintText.substring(text.length);
else
    this._clearAutocompleteElement();
this._previousText = text;
this.autoCompleteSoon(force);

https://codereview.chromium.org/2285993002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:398:
this._completionRequestId++;
let's move this into _clearAutocompleteTimeout

[einbinder, 2016-10-20 01:21:44]

ptal

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

https://codereview.chromium.org/2285993002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:286:
_updateAutoComplete: function(force)
On 2016/10/19 at 23:36:47, lushnikov wrote:
> let's get rid of this method - it's confusing

Done.

https://codereview.chromium.org/2285993002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:289: if
(this._autocompleteElement) {
On 2016/10/19 at 23:36:47, lushnikov wrote:
> var hasCommonPrefix = text.startsWith(this._previousText) ||
this._previousText.startsWith(text);
> var text = this.userEnteredText();
> if (this._autocompleteElement && hasCommonPrefix)
>     this._autocompleteElement.textContent =
this._currentHintText.substring(text.length);
> else
>     this._clearAutocompleteElement();
> this._previousText = text;
> this.autoCompleteSoon(force);

Done.

https://codereview.chromium.org/2285993002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:398:
this._completionRequestId++;
On 2016/10/19 at 23:36:47, lushnikov wrote:
> let's move this into _clearAutocompleteTimeout

Done.

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

https://codereview.chromium.org/2285993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:409:
this._completeTimeout = setTimeout(this.complete.bind(this, force), immediately
? 0 : this._autocompletionTimeout);
I was mistaken about every autocomplete being in a promise resolve now. That was
for CodeMirrorTextEditor's autocomplete. I haven't unified them yet.

[lushnikov, 2016-10-20 20:49:27]

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

https://codereview.chromium.org/2285993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:351:
this._autocompleteElement.textContent =
this._currentHintText.substring(text.length);
we should update currentHintText in applySuggestion

https://codereview.chromium.org/2285993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:433: if
(wordSuffixRange.toString().length + this.userEnteredText().length -
this.text().length)
Let's put the condition more explicitly, mentioning autocompleteHint

[einbinder, 2016-10-20 21:13:47]

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

https://codereview.chromium.org/2285993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:351:
this._autocompleteElement.textContent =
this._currentHintText.substring(text.length);
On 2016/10/20 at 20:49:27, lushnikov wrote:
> we should update currentHintText in applySuggestion

Currently applySuggestion doesn't update the hint. It clears it and selects
text.

https://codereview.chromium.org/2285993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:433: if
(wordSuffixRange.toString().length + this.userEnteredText().length -
this.text().length)
On 2016/10/20 at 20:49:27, lushnikov wrote:
> Let's put the condition more explicitly, mentioning autocompleteHint

Done.

[lushnikov, 2016-10-20 21:15:30]

lgtm

[einbinder, 2016-10-20 21:35:44]

The CQ bit was checked by einbinder@chromium.org

[commit-bot: I haz the power, 2016-10-20 21:36:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 23:17:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 23:17:20]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[einbinder, 2016-10-20 23:26:59]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2016-10-20 23:27:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2285993002/#ps200001
(title: "fix test")

[commit-bot: I haz the power, 2016-10-20 23:27:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 01:33:57]

Description was changed from

==========
DevTools: Fix flickering hint when typing in TextPrompt
The grey hint text in the TextPrompt was being removed on every keystroke, and
then  re-added the next frame if it was still needed for the suggestion.

BUG=none
==========

to

==========
DevTools: Fix flickering hint when typing in TextPrompt
The grey hint text in the TextPrompt was being removed on every keystroke, and
then  re-added the next frame if it was still needed for the suggestion.

BUG=none
==========

[commit-bot: I haz the power, 2016-10-21 01:33:58]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-10-21 13:25:49]

Description was changed from

==========
DevTools: Fix flickering hint when typing in TextPrompt
The grey hint text in the TextPrompt was being removed on every keystroke, and
then  re-added the next frame if it was still needed for the suggestion.

BUG=none
==========

to

==========
DevTools: Fix flickering hint when typing in TextPrompt
The grey hint text in the TextPrompt was being removed on every keystroke, and
then  re-added the next frame if it was still needed for the suggestion.

BUG=none

Committed: https://crrev.com/37a665db550b7afc5175dee09596a46a8fcf6697
Cr-Commit-Position: refs/heads/master@{#426681}
==========

[commit-bot: I haz the power, 2016-10-21 13:25:51]

Patchset 11 (id:??) landed as
https://crrev.com/37a665db550b7afc5175dee09596a46a8fcf6697
Cr-Commit-Position: refs/heads/master@{#426681}