Shapes editor should not be triggered form overwritten properties

VERIFIED FIXED in Firefox 60

Status

P2
enhancement
VERIFIED FIXED
10 months ago
6 months ago

People

(Reporter: rcaliman, Assigned: rcaliman)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 60

Firefox Tracking Flags

(firefox60 fixed, firefox61 verified, firefox62 verified)

Details

Attachments

(2 attachments)

(Assignee)

Description

10 months ago
Created attachment 8950876 [details]
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.
(Assignee)

Updated

10 months ago
Blocks: 1435373
No longer blocks: 1242029
Comment hidden (mozreview-request)
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
Status: NEW → ASSIGNED
Flags: needinfo?(rcaliman)
Priority: -- → P2
(Assignee)

Comment 3

10 months ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
No longer blocks: 1435373
(Assignee)

Updated

10 months ago
Blocks: 1242029

Comment 7

10 months ago
mozreview-review
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.
Attachment #8950912 - Flags: review?(pbrosset) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 9

10 months ago
Pushed by archaeopteryx@coole-files.de:
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
Keywords: checkin-needed

Comment 10

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b314ccf28a2f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60

Comment 11

7 months ago
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.
Status: RESOLVED → VERIFIED
status-firefox61: --- → verified
status-firefox62: --- → verified

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.