1178967 - [Ruleview] autocomplete popup DOESN'T collapse when I press Home/End while editing CSS rule

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[arni2033]

Attachment: autocomplete popup DOESN'T collapse when I press Home(End).webm

Steps-To-Reproduce (watch video):

1. Open devtools -> Inspector -> Rules
2. Create new rule: type "back" and see the autocomplete popup with suggestions
3. Press Home
4. Press Down. Go to step 3.

Result:

I got something like "backgroundbackface-visiground-attachmentbackgroundbackface-visibility" in the rule field

Expectations:

Autocomplete popup should've EITHER collapsed OR not inserted rules.
I personally think that popup should rather collapse when you press "Home", but should insert the currently selected suggestion (just like when you press Left/Right)

Comment 1

[Patrick Brosset <:pbro>]

Thanks for the report. This looks bad, we should fix it.

Comment 2

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

I couldn't reproduce it on Mac, nothing happens when I press Home/End

https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/inplace-editor.js#919

Comment 3

[arni2033]

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #2)
> I couldn't reproduce it on Mac, nothing happens when I press Home/End
Indeed, nothing happens, but the autocomplete popup should close when you press Home/End
That's what bug is about. Read STR, watch video.

Comment 4

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

What I understand from the code is, it was intended for home/end to act like everywhere else, to move cursor around, which doesn't seem to work at the moment, maybe we should fix this instead?

--

So, Home and End don't do anything, but I couldn't get that "backgroundbackface-visiground-attachmentbackgroundbackface-visibility" thing, pressing down simply traverses the autocomplete list, not entering anything.

Comment 5

[arni2033]

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #4)
> home/end to act like everywhere else, to move cursor around, which doesn't seem to work at the moment
Oh, wait. When I press Home in Step 3, then cursor moves to the beginning of the line. I thought you meant "nothing happens with autocomplete popup when I press Home/End"
I don't think it's possible, but looks like your issue is Mac-specific

Comment 6

[Patrick Brosset <:pbro>]

Reproduced on Windows 10. Let's make this bug windows-specific.
One option is definitely closing the auto-complete popup when you press Home/End.

Comment 7

[Julian Descottes [:jdescottes]]

For the record, the issue also occurs on Linux. On OSX HOME/END does not move the caret to the end of inputs on Firefox (not sure why?). That's why we cannot reproduce the issue on this OS.

As mentioned here, we can either close the autocomplete popup on HOME/END. 
Another option is to preventDefault for those keys if the popup is open.

Inspector bug triage. Filter on CLIMBING SHOES.
Users relying on HOME/END have a hard time using ruleview editors -> P2.

Comment 8

[Julian Descottes [:jdescottes]]

The code responsible for handling key events here is in inplace-editor.js::_onKeypress [1]

The autocomplete popup should be closed when the user presses HOME or END (same as when we receive LEFT, RIGHT, BACKSPACE, DELETE).
As mentioned in previous comments, the currently highlighted entry should be the new value of the input but that should work right away.

Code change should be relatively small but we need to either modify an existing test or add a new one for this (similar to devtools/client/inspector/rules/test/browser_rules_completion-existing-property_01.js).

[1] https://hg.mozilla.org/mozilla-central/file/fc15477ce628599519cb0055f52cc195d640dc94/devtools/client/shared/inplace-editor.js#l983

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Take it!

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Hey Julien! Do you know what this line at [1] is used for? 

I noticed that my local patch works as expected after adding "HOME", "END" at [2] but somehow this._preventSuggestions = true at [1] prevents autocomplete popup again after typing "HOME" or "END". I'm not sure what's intention behind and is that safe to get rid of it in this patch?

[1] https://hg.mozilla.org/mozilla-central/file/fc15477ce628599519cb0055f52cc195d640dc94/devtools/client/shared/inplace-editor.js#l999
[2] https://hg.mozilla.org/mozilla-central/file/fc15477ce628599519cb0055f52cc195d640dc94/devtools/client/shared/inplace-editor.js#l1017

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Attachment: MozReview Request: Bug 1178967 - Collapse autocomplete popup when pressing Home/En r?jdescottes

Review commit: https://reviewboard.mozilla.org/r/54828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54828/

Comment 13

[Ricky Chien [:rickychien] (inactive)]

Uploaded my current patch without test case.

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8755809 [details]
MozReview Request: Bug 1178967 - Collapse autocomplete popup when pressing Home/En r?jdescottes

Patch looks good, just needs a test. 

The _preventSuggestions flag is only used in _maybeSuggestCompletion, to avoid showing the suggestions popup. With your patch, pressing HOME or END can never lead to _maybeSuggestCompletion, so it's safe to remove them as you did.

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8755809 [details]
MozReview Request: Bug 1178967 - Collapse autocomplete popup when pressing Home/En r?jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54828/diff/1-2/

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Test case isn't too hard to write since we can reused original test to simulate arni2033's STR.

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8755809 [details]
MozReview Request: Bug 1178967 - Collapse autocomplete popup when pressing Home/En r?jdescottes

https://reviewboard.mozilla.org/r/54828/#review51702

Looks good thanks! Let's review one last time after fixing up the test (see my comment).

::: devtools/client/inspector/rules/test/browser_rules_completion-existing-property_01.js:47
(Diff revision 2)
>    ["VK_HOME", "", -1, 0],
>    ["VK_END", "", -1, 0],
>    ["VK_PAGE_UP", "", -1, 0],
>    ["VK_PAGE_DOWN", "", -1, 0],
> +  ["d", "display", 1, 3],
> +  ["VK_HOME", "display", -1, 0],

As you can see on the try run results, this test failed on all platforms other than OSX.

The reason for this is that HOME/END on OSX do not move the caret at the beginning/end of the input in Firefox. One option here would be to rely on Services.appinfo.OS to check if we are running the test on OSX or not. Personally I would just rewrite the steps to fit Windows/Linux behaviour and skip them if Services.appinfo.OS === "Darwin". 

Whatever you choose, this will require a comment to clarify why we handle OSX differently.

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8755809 [details]
MozReview Request: Bug 1178967 - Collapse autocomplete popup when pressing Home/En r?jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54828/diff/2-3/

Comment 19

[Ricky Chien [:rickychien] (inactive)]

I prefer to add ["VK_RIGHT", "display", -1, 0] to make sure to move caret at the end of line and comment that line on top of it.

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8755809 [details]
MozReview Request: Bug 1178967 - Collapse autocomplete popup when pressing Home/En r?jdescottes

https://reviewboard.mozilla.org/r/54828/#review51722

::: devtools/client/inspector/rules/test/browser_rules_completion-existing-property_01.js:48
(Diff revisions 2 - 3)
>    ["VK_END", "", -1, 0],
>    ["VK_PAGE_UP", "", -1, 0],
>    ["VK_PAGE_DOWN", "", -1, 0],
>    ["d", "display", 1, 3],
>    ["VK_HOME", "display", -1, 0],
>    ["i", "display", 1, 2],

This will still fail on Windows and Linux :)

On OSX, after pressing HOME, the caret and selection remain the same. The selected text is "isplay". When you type "i", "isplay" is replaced by "i".

But on Windows and Linux, the caret moves to the beginning of the input. This means that typing "i" will result in "idisplay".

Comment 21

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8755809 [details]
MozReview Request: Bug 1178967 - Collapse autocomplete popup when pressing Home/En r?jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54828/diff/3-4/

Comment 22

[Ricky Chien [:rickychien] (inactive)]

Try result looks good to me.

I workaround this by merely removing ["i", "display", 1, 2]. I think it makes sense for the case that pressing HONE / END should hide popup an autocomplete.

Comment 23

[Julian Descottes [:jdescottes]]

Comment on attachment 8755809 [details]
MozReview Request: Bug 1178967 - Collapse autocomplete popup when pressing Home/En r?jdescottes

https://reviewboard.mozilla.org/r/54828/#review52126

Looks good to me, thanks!

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f5ce758ba0fd

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5ce758ba0fd

Comment 26

[Tanvir Rahman]

I have reproduced this bug with Nightly 42.0a1(2015-06-30) on Windows 10, 64 bit!

The Bug's fix is now verified on Beta 49.0b1.

Build ID 	20160802125918
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[bugday-20160803]