1438127 - Shapes editor should not be triggered form overwritten properties

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[Razvan Caliman [:rcaliman]]

Attachment: Editor triggered on polygon (bug) but shows up as circle (correct).

When competing styles match the same element, the shapes editor should not be triggered by the toggle next to overwritten properties.

Steps:
- Run this in the URL bar:
data:text/html;charset=utf-8,<style>.polygon {background: red; width:300px; height: 300px; clip-path: polygon(0 0 , 100% 0, 100% 100%);}</style><div class="polygon" style="clip-path: circle()"></div>

- Notice the `polygon` class and the inline style

- Open the DevTools Inspector, Rule view 

- Click the toggle to edit the polygon shape from the crossed-out `clip-path` declaration.

Result:

The shapes editor shows up to edit a circle, the clip-path value from the inline style which has the highest specificity.

Expected:

The editor should not be triggered from overwritten properties.

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1438127 - Prevent shape editor from being triggered from overwritten or disabled properties in the rules panel. Hide editor swatches (aka icon triggers) from showing up on disabled or overwritten properties.

Review commit: https://reviewboard.mozilla.org/r/220170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220170/

Comment 2

[Patrick Brosset <:pbro>]

Thanks Razvan. I'm looking at the commit now and testing it locally too. One thing I'd like to discuss: what about not displaying the shape icon for invalid/overwritten properties at all?
This way, the problem wouldn't happen at all. Also now the icon is there and looks clickable but clicking it doesn't do anything. So I personally would find it better if the icon wasn't there.
Doing this might also make the code change easier too.
What are your thoughts on this?

I think we should also do this for other swatches, like display:grid/flex. Right now we show the flex icon for overwritten display:flex properties, but clicking doesn't do anything at all and that can be confusing.

Comment 3

[Razvan Caliman [:rcaliman]]

I don't have any argument against that. It does reduce cognitive load.

For invalid values, the editor swatch doesn't show up at all, as expected.

For overwritten rules, we can hide the swatches with with a simple CSS descendant selector based on the class that shows the strike-through. Something like:

.ruleview-overridden .ruleview-swatch {
  display: none;
}

It's trivial to implement and doesn't touch any of the other logic.

I don't know of the UX implications for other tools, but for the shapes editor this wouldn't hurt.

Comment 4

[Razvan Caliman [:rcaliman]]

Comment on attachment 8950912 [details]
Bug 1438127 - Prevent shape editor from being triggered from overwritten or disabled properties in the rules panel. Hide editor swatches (aka icon triggers) from showing up on disabled or overwritten properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220170/diff/1-2/

Comment 5

[Razvan Caliman [:rcaliman]]

Comment on attachment 8950912 [details]
Bug 1438127 - Prevent shape editor from being triggered from overwritten or disabled properties in the rules panel. Hide editor swatches (aka icon triggers) from showing up on disabled or overwritten properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220170/diff/2-3/

Comment 6

[Razvan Caliman [:rcaliman]]

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

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8950912 [details]
Bug 1438127 - Prevent shape editor from being triggered from overwritten or disabled properties in the rules panel. Hide editor swatches (aka icon triggers) from showing up on disabled or overwritten properties.

https://reviewboard.mozilla.org/r/220170/#review226518

Thank you Razvan. Looks good. I only have a couple minor comments, no need to block R+ on them. So, please address them, get a green try build, and feel free to land after that without asking for review again.

::: devtools/client/inspector/rules/test/browser_rules_shapes-toggle_02.js:36
(Diff revision 3)
> -  info("Checking the initial state of the CSS shapes toggle in the rule-view.");
> -  ok(shapeToggle && overriddenShapeToggle, "Shapes highlighter toggles are visible.");
> +  // eslint-disable-next-line max-len
> +  ok(shapeToggle && overriddenShapeToggle, "Shapes highlighter toggles exist in the DOM.");

We usually avoid disabling eslint unless absolutely necessary, and here you can easily work around the max-len rule by wrapping on 2 lines, which is something we do very often in the code:

```
ok(shapeToggle && overriddenShapeToggle,
   "Shapes highlighter toggles exist in then DOM");
```

::: devtools/client/inspector/rules/test/browser_rules_shapes-toggle_02.js:43
(Diff revision 3)
> -    "active in the rule-view.");
> -  ok(!shapeToggle.classList.contains("active") &&
> +  // eslint-disable-next-line max-len
> +  is(overriddenShapeToggleStyle.display, "none", "Overwritten shape highlighter toggle is not visible");

Same comment here.

::: devtools/client/inspector/shared/highlighters-overlay.js
(Diff revision 3)
>        event.stopPropagation();
>  
>        this.toggleFlexboxHighlighter(this.inspector.selection.nodeFront);
>      } else if (this._isRuleViewShape(event.target)) {
>        event.stopPropagation();
> -

This line removal seems a bit unrelated. Can you please revert this one change so the hg history doesn't get change at this location.

Comment 8

[Razvan Caliman [:rcaliman]]

Comment on attachment 8950912 [details]
Bug 1438127 - Prevent shape editor from being triggered from overwritten or disabled properties in the rules panel. Hide editor swatches (aka icon triggers) from showing up on disabled or overwritten properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220170/diff/3-4/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b314ccf28a2f
Prevent shape editor from being triggered from overwritten or disabled properties in the rules panel. Hide editor swatches (aka icon triggers) from showing up on disabled or overwritten properties. r=pbro

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b314ccf28a2f

Comment 11

[Hani Yacoub, Desktop QA]

Build ID: 20180528091514
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

Verified as fixed on Firefox Nightly 62.0a1 and Firefox 61.0b9  on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.