Closed Bug 1583901 Opened 6 years ago Closed 4 years ago

[Inactive CSS] Display a warning when 'fill-*', 'stroke-*', or 'paint-order' are used on invalid elements

Categories

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

Product:
DevTools
Component:
Inspector: Rules
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: miker, Assigned: sebo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [importance-8.4%])

Attachments

(1 file)

Bug 1583901 - New inactive CSS rule to show warning when fill-* or stroke-* is used on non-SVG-basic-shape and non-SVG-text elements. r=jdescottes
48 bytes, text/x-phabricator-request
Details | Review

Main file:
devtools/server/actors/utils/inactive-property-helper.js

invalidProperties: [
"marker",
"marker-start",
"marker-mid",
"marker-end",
"stroke",
"stroke-dasharray",
"stroke-dashoffset",
"stroke-width",
]

inactive-css-only-certain-svg-1 = <strong>{ $property }</strong> has no effect on this element since it can only be applied to the following SVG elements: <ul><li>altGlyph</li><li>circle</li><li>ellipse</li><li>line</li><li>path</li><li>polygon</li><li>polyline</li><li>rect</li><li>text</li><li>textPath</li><li>tref</li><li>tspan</li></ul>

inactive-css-only-certain-svg-fix = Ensure you are setting this property on one of the SVG elements listed above. { learn-more }

Assignee: nobody → sebastianzartner
Status: NEW → ASSIGNED

Julian commented in the patch review that the marker-* properties actually only apply to shapes. What was probably meant are all properties that apply to text content elements and shapes. Those are according to SVG 2, Fill and Strokes 3 and the current implementation in Gecko the following properties:

  • fill
  • fill-opacity
  • stroke
  • stroke-dasharray
  • stroke-dashoffset
  • stroke-linecap
  • stroke-linejoin
  • stroke-miterlimit
  • stroke-opacity
  • stroke-width
  • paint-order

Where paint-order is a special case because it also applies to HTML elements since bug 1435684 when -webkit-text-stroke is defined at the same time. All other properties are not supported on HTML elements yet, as far as I can see.

Sebastian

Summary: [Inactive CSS] Display a warning when 'marker-*' or 'stroke-*' are used on invalid elements → [Inactive CSS] Display a warning when 'fill-*', 'stroke-*', or 'paint-order' are used on invalid elements
See Also: → 1686583
See Also: → 1686585
Attachment #9196289 - Attachment description: Bug 1583901 - New inactive CSS rule to show warning when marker-* or stroke-* is used on non-SVG-basic-shape and non-SVG-text elements. r=mtigley → Bug 1583901 - New inactive CSS rule to show warning when fill-* or stroke-* is used on non-SVG-basic-shape and non-SVG-text elements. r=jdescottes
Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/870a4ac0af45 New inactive CSS rule to show warning when fill-* or stroke-* is used on non-SVG-basic-shape and non-SVG-text elements. r=fluent-reviewers,flod,jdescottes

The SVG 2 specification suggests that CSS properties be valid on all elements even if they have no effect. On top of that since fill and stroke are inherit rather than reset properties authors often put them on <g> and <svg> container elements and then have them work on all their children.

I really don't think this is the right thing to do here.

(In reply to Robert Longson [:longsonr] from comment #5)

The SVG 2 specification suggests that CSS properties be valid on all elements even if they have no effect.

The specification says "Applies to: shapes and text content elements". And the point of the inactive CSS feature is to point out that the applied CSS doesn't have an effect on the current element.

On top of that since fill and stroke are inherit rather than reset properties authors often put them on <g> and <svg> container elements and then have them work on all their children.

That's covered by the hint which is: <strong>{ $property }</strong> has no effect on this element since it can only be applied to SVG basic shapes and text elements, though it may have an effect on its descendant elements.

Let me know if you feel strongly about not providing a hint that those properties only have an effect on shapes and text content elements.

Sebastian

Flags: needinfo?(sebastianzartner)

(In reply to Cristian Brindusan [:cbrindusan] from comment #4)

Backed out changeset 870a4ac0af45 (bug 1583901) for devtools failures at browser_rules_preview-tooltips-sizes.js.
https://hg.mozilla.org/integration/autoland/rev/3837805e531398c01dcc75d0ff93fafe792b8c96

Obviously, some unrelated code (the one related to ImageTooltipHelper.js) slipped in there.

Before I fix the patch for landing, let's clarify Robert's concerns first.

Sebastian

I've cc'd the other SVG Peers so let's see if they have anything to say...

Yeah, I agree with Robert that we shouldn't take this patch.

(In reply to Robert Longson [:longsonr] from comment #5)

On top of that since fill and stroke are inherit rather than reset properties authors often put them on <g> and <svg> container elements and then have them work on all their children.

Agreed -- speaking more broadly, this is a reason why we probably shouldn't have devtools make this sort of "inactive property" warning for any inherited-by-default properties (perhaps with rare exceptions where it very-obviously goes against best practices for some reason; but I can't think of any such cases at the moment).

For non-inherited-by-default properties, it makes some sense to adapt the "Applies to:" line into a inactive-property devtools warning. But for inherited-by-default properties, there are totally legitimate reasons that authors might set it on an ancestor where it's technically inactive, in order to concisely set the value for all of the descendants where it is absolutely active.

As an analogy: this patch is similar to if we decided to warn authors that list-style is inactive on non-list-item elements, because CSS tells us it's only applicable to elements with display:list-item. This would be a bad thing for us to warn about. Strictly speaking, we'd be correct to warn; but since the property is inherited by default, it's entirely possible for it to be "inactive" on the element that it's set on, and yet still very "active" on its list-item descendants. And it's in fact kind of pathological for an author to have to set it on every individual list-item element in order to change the style of that item, if they're really wanting to style a whole list.

Concrete example: suppose an author had the following markup:

data:text/html,<ol style="list-style: lower-roman"><li><li><li>

If we had such a rule, then we'd warn about the list-style property there, saying something like "list-style is inactive; it's only valid on elements with display:list-item. In doing so, we would effectively be prompting the author to rewrite their markup with a bunch of clumsy duplication like so:

data:text/html,<ol><li style="list-style: lower-roman"><li  style="list-style: lower-roman"><li style="list-style: lower-roman">

...which is obviously not actually an improvement.

BTW, I did a quick skim of https://searchfox.org/mozilla-central/source/devtools/server/actors/utils/inactive-property-helper.js and I didn't notice any inherited-by-default properties that are currently listed there right now. So I think we're OK on this front, with the current set of properties that we're warning about. But we should keep this in mind when adding new rules there.

So I would suggest that we:
(1) WONTFIX this bug (since per its current summary, I don't think it's a change we should make, as discussed above)

(2) Ideally, add a note to the header-comment in inactive-property-helper.js to warn folks about this in the future.

I'll spin off a helper-bug to do (2), since I just came up with some suggested prose and it's probably best for me to just post a patch rather than to suggest that someone else do so.

(Sorry to be pushing back on this after you've already gotten a patch up and reviewed, :sebo. I appreciate the work that went into preparing this; in a perfect world, we would've noticed this and wontfix'ed the bug before you'd come along and written a patch to implement the requested behavior.)

(In reply to Daniel Holbert [:dholbert] from comment #12)

(2) Ideally, add a note to the header-comment
[...]
I'll spin off a helper-bug to do (2)

Filed: bug 1688538.

See Also: → 1688538

(In reply to Daniel Holbert [:dholbert] from comment #10)

As an analogy: this patch is similar to if we decided to warn authors that list-style is inactive on non-list-item elements, because CSS tells us it's only applicable to elements with display:list-item. This would be a bad thing for us to warn about.

Ok, that's convincing.

(In reply to Daniel Holbert [:dholbert] from comment #12)

(Sorry to be pushing back on this after you've already gotten a patch up and reviewed, :sebo. I appreciate the work that went into preparing this; in a perfect world, we would've noticed this and wontfix'ed the bug before you'd come along and written a patch to implement the requested behavior.)

It's ok. And it's good that we clarified it now and we're lucky the patch didn't land in mozilla-central yet, then.

Sebastian

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX

Sorry about this Sebo :/ but thanks for jumping in :dholbert. I also feel like we should never show warnings that might lead users to incorrectly update their CSS, but I didn't think about the inheritance use case.

We'll keep this in mind for the rest of the inactive CSS bugs, https://bugzilla.mozilla.org/showdependencytree.cgi?id=1540753&hide_resolved=1 maybe we should do a quick pass to review those. I also wonder if any of our current warnings have this issue.

Daniel and I can wontfix some of the inactive CSS bugs we see as problematic because they are inherit properties if you like Julian.

(In reply to Julian Descottes [:jdescottes] from comment #15)

We'll keep this in mind for the rest of the inactive CSS bugs, https://bugzilla.mozilla.org/showdependencytree.cgi?id=1540753&hide_resolved=1
maybe we should do a quick pass to review those.

I'm doing a brief skim over some of those right now (I might not catch everything so I'd appreciate if someone else could also audit the still-open ones).

I also wonder if any of our current warnings have this issue.

I had the same wondering (see comment 11) but I think we're OK on that front. (But a double-check/second-opinion wouldn't hurt.)

(In reply to Robert Longson [:longsonr] from comment #16)

Daniel and I can wontfix some of the inactive CSS bugs we see as problematic because they are inherit properties if you like Julian.

Thanks for doing that Robert & Daniel, greatly appreciated!

(In reply to Daniel Holbert [:dholbert] from comment #17)

(In reply to Julian Descottes [:jdescottes] from comment #15)

We'll keep this in mind for the rest of the inactive CSS bugs, https://bugzilla.mozilla.org/showdependencytree.cgi?id=1540753&hide_resolved=1
maybe we should do a quick pass to review those.

I'm doing a brief skim over some of those right now (I might not catch everything so I'd appreciate if someone else could also audit the still-open ones).

I'll go through them as well, thanks for the help!

Sure! I finished my skim of those bugs and I wontfix'ed a handful.

Specifically: I won'tfix'ed bug 1551583, bug 1583893, bug 1583900, bug 1583908, bug 1583909, bug 1686583, and bug 1686585,

And I added a note on bug 1583902 which covers a mix of properties, some inherited and some not.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: