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

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Developer Tools: CSS Rules Inspector
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: arni2033, Assigned: rickychien, Mentored)

Tracking

unspecified
Firefox 49
Unspecified
Windows
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [btpp-fix-later][good taipei bug])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8627881 [details]
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)
Thanks for the report. This looks bad, we should fix it.
Mentor: pbrosset@mozilla.com

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
(Reporter)

Comment 3

2 years ago
(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.
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.
Flags: needinfo?(pbrosset)
(Reporter)

Comment 5

2 years ago
(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
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.
Flags: needinfo?(pbrosset)
OS: Unspecified → Windows
(Reporter)

Updated

2 years ago
Has STR: --- → yes
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
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.
Priority: -- → P2
Whiteboard: [btpp-fix-later]
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
Mentor: pbrosset@mozilla.com → jdescottes@mozilla.com
Whiteboard: [btpp-fix-later] → [btpp-fix-later][good taipei bug]
Duplicate of this bug: 1063801
(Assignee)

Comment 10

2 years ago
Take it!
Assignee: nobody → rchien
Status: NEW → ASSIGNED
(Assignee)

Comment 11

2 years ago
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
Flags: needinfo?(jdescottes)
(Assignee)

Comment 12

2 years ago
Created attachment 8755809 [details]
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/
(Assignee)

Comment 13

2 years ago
Uploaded my current patch without test case.
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.
Flags: needinfo?(jdescottes)
Attachment #8755809 - Flags: feedback+
(Assignee)

Comment 15

2 years ago
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/
Attachment #8755809 - Attachment description: MozReview Request: Bug 1178967 - Collapse autocomplete popup when pressing Home/En → MozReview Request: Bug 1178967 - Collapse autocomplete popup when pressing Home/En r?jdescottes
Attachment #8755809 - Flags: feedback+ → review?(jdescottes)
(Assignee)

Comment 16

2 years ago
Test case isn't too hard to write since we can reused original test to simulate arni2033's STR.
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.
Attachment #8755809 - Flags: review?(jdescottes)
(Assignee)

Comment 18

2 years ago
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/
Attachment #8755809 - Flags: review?(jdescottes)
(Assignee)

Comment 19

2 years ago
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 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".
Attachment #8755809 - Flags: review?(jdescottes)
(Assignee)

Comment 21

2 years ago
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/
Attachment #8755809 - Flags: review?(jdescottes)
(Assignee)

Comment 22

2 years ago
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.
Attachment #8755809 - Flags: review?(jdescottes) → review+
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!
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 24

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/f5ce758ba0fd
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f5ce758ba0fd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Comment 26

a year ago
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]
You need to log in before you can comment on or make changes to this bug.