1032984 - Shift + Home does not highlight value instead appends autocomplete value

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[tripad.mishra]

Attachment: Pressing Shift and Home

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

Shift + Home key on any property does not select(highlight) the value, instead it appends the default value from auto complete.

(See the attached GIF)



Expected results:

This is pretty frustrating as instead of the expect behavior of highlighting the whole value, it appends the default value and now you need to select the whole value again to erase it.

Comment 1

[Brian Grinstead [:bgrins]]

Can confirm on OSX with fn+arrow.  This appends the selected portion of the autocomplete onto the end of the input.  Happens with fn+left/fn+right/fn+up/fn+down

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Jen, assigning to you as a potential polish bug.

Brian, since Jen is unfamiliar with this part of the codebase would you be available to mentor her here, or is someone else a better fit?

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Setting devedition-40 bugs to P2, filter on C17C996C-A0BE-4153-8057-FAB642D0746D

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] from comment #2)
> Brian, since Jen is unfamiliar with this part of the codebase would you be
> available to mentor her here, or is someone else a better fit?

Happy to, but I don't have much advice beyond the fact that you should start by looking at the _onKeyPress function in the inplace-editor file [0].  And, good luck!

[0]:  https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/inplace-editor.js#856

Comment 5

[tripad.mishra]

Yay! Finally this is being looked into :D

Just wanted to add that
*Shift + End* also has a similar issues.

And i have verified this on multiple versions of Ubuntu also.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to J. Ryan Stinnett [:jryans] from comment #2)
> > Brian, since Jen is unfamiliar with this part of the codebase would you be
> > available to mentor her here, or is someone else a better fit?
> 
> Happy to, but I don't have much advice beyond the fact that you should start
> by looking at the _onKeyPress function in the inplace-editor file [0].  And,
> good luck!
> 
> [0]: 
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/
> inplace-editor.js#856

My best guess is that End/Home are somehow being treated similar to arrow keys and finishing the value within the editor, but also not changing the selection range for some reason.  In the case of home/end (with or without shift) we should just leave the event alone and not do any special things - the default behavior of the <input> should be fine.

Comment 8

[Jen Fong-Adwent [:ednapiranha]]

Unassigning myself for now until next week (hopefully).

Comment 9

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1032984.patch

This test is somewhat correct, but doesn't actually check the rule view - it checks any editable input (which works currently with the shift+home behavior on the inspector side, just not on the rule view side). Any idea how to specify the rule view property in this test?

Also let me know if the rest is more or less on the right track - thanks!

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8595038 [details] [diff] [review]
Bug1032984.patch

Review of attachment 8595038 [details] [diff] [review]:
-----------------------------------------------------------------

The fix seems good.  When trying to figure out exactly how to assert things in browser_inplace-editor, I realized that there is an easier test case to modify: styleinspector/test/browser_ruleview_completion-existing-property_01.js.

::: browser/devtools/shared/inplace-editor.js
@@ +881,5 @@
>      } else if (aEvent.altKey && !aEvent.shiftKey) {
>        increment *= smallIncrement;
>      }
>  
> +    // Default to highlighting text from the position of the cursor rather than

Nit: "Default to highlighting text from the position of the cursor" -> "Use default cursor movement"

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: ruleview-failing-test-case.patch

Since I came up with this when trying to figure out what was going on, may as well share it

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8595038 [details] [diff] [review]
Bug1032984.patch

Review of attachment 8595038 [details] [diff] [review]:
-----------------------------------------------------------------

Also, on Mac fn+down and fn+up seem to cause the same problem.

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> Also, on Mac fn+down and fn+up seem to cause the same problem.

Which I believe maps to Page Up / Page Down

Comment 14

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1032984.patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9181c815969

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8595427 [details] [diff] [review]
Bug1032984.patch

Review of attachment 8595427 [details] [diff] [review]:
-----------------------------------------------------------------

Nice

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Gregory Szorc [:gps]]

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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3ae82e9e39da