[Rule View] Ensure the correct editable property is focused when editing and removing properties in the rule view

RESOLVED FIXED in Firefox 42

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bgrins, Assigned: gl)

Tracking

({regression})

Trunk
Firefox 42
regression
Points:
---

Firefox Tracking Flags

(firefox41 unaffected, firefox42 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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

2 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

2 years ago
Assignee: nobody → gabriel.luong
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8641923 [details] [diff] [review]
1188897.patch
(Assignee)

Updated

2 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

2 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

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6db24ff8f55f
(Assignee)

Comment 5

2 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)

Updated

2 years ago
Depends on: 1190047
(Assignee)

Comment 6

2 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

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=53d5d4a8247e
(Reporter)

Comment 8

2 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

2 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

2 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 11

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/942ac3261a69
https://hg.mozilla.org/mozilla-central/rev/942ac3261a69
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.