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

Status: RESOLVED FIXED

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Brian Grinstead [:bgrins]]

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.

Comment 2

[Gabriel [:gl] (ΦωΦ)]

Created attachment 8641923 [details] [diff] [review]
1188897.patch

Comment 3

[Gabriel [:gl] (ΦωΦ)]

Created attachment 8641959 [details] [diff] [review]
1188897.patch [1.0]

Comment 4

[Gabriel [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6db24ff8f55f

Comment 5

[Gabriel [:gl] (ΦωΦ)]

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]

Comment 6

[Gabriel [:gl] (ΦωΦ)]

Created attachment 8642920 [details] [diff] [review]
1188897.patch [2.0]

Comment 7

[Gabriel [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53d5d4a8247e

Comment 8

[Brian Grinstead [:bgrins]]

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

Comment 9

[Gabriel [:gl] (ΦωΦ)]

Created attachment 8644017 [details] [diff] [review]
1188897.patch [3.0]

Comment 10

[Brian Grinstead [:bgrins]]

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

Comment 11

[Pulsebot]

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

Comment 12

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/942ac3261a69