Closed Bug 1438127 Opened 6 years ago Closed 6 years ago

Shapes editor should not be triggered form overwritten properties


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



(firefox60 fixed, firefox61 verified, firefox62 verified)

Firefox 60
Tracking Status
firefox60 --- fixed
firefox61 --- verified
firefox62 --- verified


(Reporter: rcaliman, Assigned: rcaliman)


(Blocks 1 open bug)



(2 files)

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

- 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.


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


The editor should not be triggered from overwritten properties.
Blocks: 1435373
No longer blocks: 1242029
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.
Severity: normal → enhancement
Flags: needinfo?(rcaliman)
Priority: -- → P2
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.
Flags: needinfo?(rcaliman)
No longer blocks: 1435373
Blocks: 1242029
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.

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.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.
Attachment #8950912 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
Pushed by
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
Keywords: checkin-needed
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.