[Rule View] Ensure the correct editable property is focused when editing and removing properties in the rule view
RESOLVED
FIXED
in Firefox 42
Status
People
(Reporter: bgrins, Assigned: gl)
Tracking
({regression})
Firefox Tracking Flags
(firefox41 unaffected, firefox42 fixed)
Details
Attachments
(1 attachment, 3 obsolete attachments)
41.47 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
STR: Open rule view Add a property by typing 'color: red' Add another one like 'background:' Once the value field focuses, press shift+tab Expected: The 'background' field becomes selected Actual: The 'red' field becomes selected, and the background property disappears.
(Reporter) | ||
Comment 1•4 years ago
|
||
I think this regressed as part of one of the [rule-view-correctness] fixes, possibly to do with not previewing the value when editing starts. This is more of a UX issue than a correctness issue, but I'd still like to see if we can get a fix in place in time for 42.
status-firefox41: --- → unaffected
(Assignee) | ||
Updated•4 years ago
|
Assignee: nobody → gabriel.luong
Status: NEW → ASSIGNED
(Assignee) | ||
Comment 2•4 years ago
|
||
Created attachment 8641923 [details] [diff] [review] 1188897.patch
(Assignee) | ||
Updated•4 years ago
|
Summary: When shift+tabbing out of a newly created property empty value field, the focus moves up to the previous property value instead of the name of the current property → [Rule View] Ensure the correct editable property is focused when editing and removing properties in the rule view
(Assignee) | ||
Comment 3•4 years ago
|
||
Created attachment 8641959 [details] [diff] [review] 1188897.patch [1.0]
Attachment #8641923 -
Attachment is obsolete: true
Attachment #8641959 -
Flags: review?(bgrinstead)
(Assignee) | ||
Comment 4•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6db24ff8f55f
(Assignee) | ||
Comment 5•4 years ago
|
||
Comment on attachment 8641959 [details] [diff] [review] 1188897.patch [1.0] Handling of swatch onRevert to the original value seems to have regressed throughout the [rule-view-correctness]
Attachment #8641959 -
Flags: review?(bgrinstead)
(Assignee) | ||
Comment 6•4 years ago
|
||
Created attachment 8642920 [details] [diff] [review] 1188897.patch [2.0]
Attachment #8641959 -
Attachment is obsolete: true
Attachment #8642920 -
Flags: review?(bgrinstead)
(Assignee) | ||
Comment 7•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=53d5d4a8247e
(Reporter) | ||
Comment 8•4 years ago
|
||
Comment on attachment 8642920 [details] [diff] [review] 1188897.patch [2.0] Review of attachment 8642920 [details] [diff] [review]: ----------------------------------------------------------------- Just using the rule view with this patch applied, it feels good and I haven't seen any issues yet. Haven't had a chance to fully review the code / tests but I've done a quick pass through the rule-view changes. I'll take another look tomorrow. ::: browser/devtools/styleinspector/rule-view.js @@ +946,5 @@ > */ > + editClosestTextProperty: function(textProperty, direction) { > + let index = this.textProps.indexOf(textProperty); > + > + if (direction == Ci.nsIFocusManager.MOVEFOCUS_FORWARD) { Nit === across this patch to match style in this file @@ +3330,5 @@ > + // If a name is empty or value is empty and the focus is going moving > + // backward or escape was pressed, the entire property should always be > + // removed. > + if (!value.trim() || > + (!this.prop.value && Please split this up a bit using a variable so it's a bit easier to read, something like: let moveBack = !direction || direction === Ci.nsIFocusManager.MOVEFOCUS_BACKWARD; if (!value.trim() || (moveBack && !this.prop.value)) { } @@ +3397,5 @@ > this.committed.value == val.value && > this.committed.priority == val.priority; > > + if (value.trim() && > + ((!commit && !this.ruleEditor.isEditing && !direction) || This is another one of those complicated conditions that could use a descriptive variable name
(Assignee) | ||
Comment 9•4 years ago
|
||
Created attachment 8644017 [details] [diff] [review] 1188897.patch [3.0]
Attachment #8642920 -
Attachment is obsolete: true
Attachment #8642920 -
Flags: review?(bgrinstead)
Attachment #8644017 -
Flags: review?(bgrinstead)
(Reporter) | ||
Comment 10•4 years ago
|
||
Comment on attachment 8644017 [details] [diff] [review] 1188897.patch [3.0] Review of attachment 8644017 [details] [diff] [review]: ----------------------------------------------------------------- Functionality-wise, this seems great. Testing this stuff (inplace editor / _applyingModifications / etc) is kind of clunky, but I think it's following the existing conventions for the rule view so if we ever go through and update that we can hit this at the same time. ::: browser/devtools/styleinspector/rule-view.js @@ +3321,5 @@ > } > > + // If a name is empty, or value is empty and the focus is moving backward > + // or escape was pressed, the entire property should always be removed. > + let moveBackOrEscape = !direction || This variable name doesn't reflect the truth in all cases (in the case of a blur, !direction would be true but escape was never pressed). I think this may be more clear (please double check to make sure I got it right, but I think this is it): // Remove a property if the name is empty if (!value.trim()) { this.remove(direction); return; } // Remove a property if the value is empty and the value is not about to be focused if (!this.prop.value && direction !== Ci.nsIFocusManager.MOVEFOCUS_FORWARD) { this.remove(direction); return; } ::: browser/devtools/styleinspector/test/browser_ruleview_add-property-cancel_02.js @@ +3,5 @@ > http://creativecommons.org/publicdomain/zero/1.0/ */ > > "use strict"; > > +// Tests adding a new property and escapes the new empty property value editor Nit: extra whitespace
Attachment #8644017 -
Flags: review?(bgrinstead) → review+
Comment 12•4 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/942ac3261a69
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Updated•8 months ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•