Closed Bug 987870 Opened 6 years ago Closed 6 years ago

[rule-view] Cursor should move between inplace editor auto-opened suggestions

Categories

(DevTools :: Inspector, defect)

x86
macOS
defect
Not set

Tracking

(firefox30 fixed, firefox31 fixed)

RESOLVED FIXED
Firefox 31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: bgrins, Assigned: Optimizer)

References

Details

Attachments

(1 file, 1 obsolete file)

STR:

Type "color:" into a new rule property
A dropdown opens with a suggested list of colors
Press 'down'

Expected that pressing arrow keys moves around the list.  Won't move around the list until you start typing a character and one of the results becomes selected.
Attached patch patch (obsolete) — Splinter Review
Fixes this and https://bugzilla.mozilla.org/show_bug.cgi?id=913955#c15
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Attachment #8396600 - Flags: review?(bgrinstead)
Comment on attachment 8396600 [details] [diff] [review]
patch

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

Just need to replace accelKey with metaKey and I'll r+ with a green try

::: browser/devtools/shared/inplace-editor.js
@@ +872,5 @@
>          aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_RIGHT) {
>        if (this.popup && this.popup.isOpen) {
>          this.popup.hidePopup();
>        }
> +    } else if (!cycling && !aEvent.accelKey && !aEvent.altKey &&

accelKey -> metaKey

::: browser/devtools/styleinspector/test/browser_bug894376_css_value_completion_new_property_value_pair.js
@@ +20,5 @@
>  //    selectedIndex of the popup,
>  //    total items in the popup
>  //  ]
>  let testData = [
> +  ["a", {accelKey: true, ctrlKey: true}, "", -1, 0],

accelKey -> metaKey

@@ +87,5 @@
>        checkState();
>      });
>    }
> +  else if (/(right|back_space|escape|return)/ig.test(key) ||
> +           (modifiers.accelKey || modifiers.ctrlKey)) {

accelKey -> metaKey
Attachment #8396600 - Flags: review?(bgrinstead)
Attached patch patch v2Splinter Review
Not converting to metaKey in synthesizeKey as there it wants accelKey to simulate cmd.

pushed to try : https://tbpl.mozilla.org/?tree=Try&rev=caba97f4c351
Attachment #8396600 - Attachment is obsolete: true
Attachment #8396682 - Flags: review?(bgrinstead)
Found another weird bug here (with and without patch applied).  If I type "color:" and the list opens up then select one of them with my mouse the property disappears nothing gets applied.
That is a regression from a long time back when some output parser refactoring was done. I don;t know if I have a bug on file though.
(In reply to Girish Sharma [:Optimizer] from comment #5)
> That is a regression from a long time back when some output parser
> refactoring was done. I don;t know if I have a bug on file though.

Comparing with Nightly, I think Bug 912189 has aggravated this problem.  In nightly, after selecting one with the mouse it picks it but the popup sticks around - and if you do it a couple of times some weird stuff happens.  Now if you pick with the mouse then everything disappears and the property is deleted.

We need to either fix this problem here or make sure that a bug is filed that will land along with 912189 to fix this one.
(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Girish Sharma [:Optimizer] from comment #5)
> > That is a regression from a long time back when some output parser
> > refactoring was done. I don;t know if I have a bug on file though.
> 
> Comparing with Nightly, I think Bug 912189 has aggravated this problem.  In
> nightly, after selecting one with the mouse it picks it but the popup sticks
> around - and if you do it a couple of times some weird stuff happens.  Now
> if you pick with the mouse then everything disappears and the property is
> deleted.
> 
> We need to either fix this problem here or make sure that a bug is filed
> that will land along with 912189 to fix this one.

This bug already clubs 2 separate issues. Followup is the right way imo.
(In reply to Girish Sharma [:Optimizer] from comment #7)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > (In reply to Girish Sharma [:Optimizer] from comment #5)
> > > That is a regression from a long time back when some output parser
> > > refactoring was done. I don;t know if I have a bug on file though.
> > 
> > Comparing with Nightly, I think Bug 912189 has aggravated this problem.  In
> > nightly, after selecting one with the mouse it picks it but the popup sticks
> > around - and if you do it a couple of times some weird stuff happens.  Now
> > if you pick with the mouse then everything disappears and the property is
> > deleted.
> > 
> > We need to either fix this problem here or make sure that a bug is filed
> > that will land along with 912189 to fix this one.
> 
> This bug already clubs 2 separate issues. Followup is the right way imo.

Fine with me - I've filed Bug 988001
Attachment #8396682 - Flags: review?(bgrinstead) → review+
Let's wait for try to return before checking in to make sure our meta/accel keys are working across all platforms
green enough. (random unrelated oranges)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07bc2edf7b9d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Comment on attachment 8396682 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 912189
User impact if declined: This bug fixes an issue introduced by 912189. Also, this patch is needed as a part of a bigger queue so as to uplift the chunks-by-dir feature of tests to aurora. see https://tbpl.mozilla.org/?tree=Try&rev=bfee331a418c
Testing completed (on m-c, etc.): mc
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none
Attachment #8396682 - Flags: approval-mozilla-aurora?
Attachment #8396682 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.