Open Bug 1618565 Opened 5 years ago Updated 1 year ago

Support ::marker pseudo-element in inactiveCSS

Categories

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

defect

Tracking

(Not tracked)

People

(Reporter: pbro, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

  • open data:text/html,<style>li::before{content:"";position:relative;top:10px;}li::marker{content:"";position:relative;top:10px;}</style><ul><li>test
  • open the inspector and select the ::marker pseudo-element

Expected: the top property should not be greyed-out
Actual: the top property is marked as being inactive

This comes from the fact that ::marker doesn't seem to be supported well in InactiveCSS.
As a comparison, try the ::before pseudo-element in that page, you'll see things work well there.
Also, the same happens for other types of warnings, it's not only due to the top property.

So far, investigation shows that it could be coming from this line:

return node.ownerGlobal.getComputedStyle(bindingElement, pseudo);

This is where we get the computed style for the currently selected element in the inspector.
node.ownerGlobal is just the container window object.
bindingElement in this case is the <li> element.
pseudo is the string :marker.

And, sure enough, if I run the equivalent line of code on the test page, using the console:
window.getComputedStyle($("li"), ":marker");
Then I get "static" instead of "relative" as I was expecting.

So it could be a problem (or limitation) with how :marker was implemented in Firefox.

Mats, I hope you can help here. Are we defining the ::marker pseudo element correctly in nsCSSPseudoElementList.h? Though not defined in CSS2, it seems that the element can be selected with the single-colon syntax. Should it have the CSS_PSEUDO_ELEMENT_IS_CSS2 flag? Likewise, the marker box seemingly does contain elements according to the spec, but we don't give it the CSS_PSEUDO_ELEMENT_CONTAINS_ELEMENTS flag. Are these flags set correctly?

Flags: needinfo?(mats)

(In reply to Brad Werth [:bradwerth] from comment #2)

Though not defined in CSS2, it seems that the element can be selected with the single-colon syntax. Should it have the CSS_PSEUDO_ELEMENT_IS_CSS2 flag?

No, it shouldn't support single-colon syntax, and it doesn't.

Likewise, the marker box seemingly [does contain elements according to the spec][1], but we don't give it the CSS_PSEUDO_ELEMENT_CONTAINS_ELEMENTS flag.

Well, it contains anonymous content only. An author can't inject a <div> or whatever into it. The CSS_PSEUDO_ELEMENT_CONTAINS_ELEMENTS is for ::first-line/letter which wraps DOM content, including elements.

Are these flags set correctly?

Yes, I believe they are.

Flags: needinfo?(mats)

(In reply to Patrick Brosset <:pbro> from comment #1)

And, sure enough, if I run the equivalent line of code on the test page, using the console:
window.getComputedStyle($("li"), ":marker");
Then I get "static" instead of "relative" as I was expecting.

Two problems: first you need to use "::marker" since ":marker" just behaves as any other unknown pseudo like ":foo"; second, we have disabled support for many properties on ::marker because the CSS spec isn't finished yet. This is controlled by the layout.css.marker.restricted pref. If you toggle that on and use two colons you should get relative.

I would expect that pref to be enabled by default soon-ish, so I wouldn't spend time on trying to figure out which properties apply in the current situation.

Thanks, Mats. That's very helpful. So, to summarize, this is working as intended. ::marker is restricted in which properties it allows, and "top" is not on that list. As far as devtools, we can consider whether we want to customize the message in this situation. Patrick, would you rather we close this as INVALID, or do we keep it open for a more specific tooltip/message?

Flags: needinfo?(pbrosset)

Thank you both, this is very helpful.
My hunch for the best approach to take here is: let's bail out if the element is the ::marker pseudo-element and not attempt to show warnings. Because the list of properties supported is so tiny, and because we can't access them yet until layout.css.marker.restricted is ON, I think it's safer to just ignore this pseudo-element.
If we wanted to support ::marker, we'd have to do it on a case by case basis in our inactivecss rules engine, and would end up complexifying it quite substantially for not much reasons.
So, we should keep this bug, but use it to ignore ::marker pseudo-elements. We should also add a comment in the code explaining why, as things may change in the future and we might want to revisit this decision.
Happy to hear other thoughts on this.

Flags: needinfo?(pbrosset)

I left the above patch to give an idea of what I have in mind. Feel free to pick up from here and land this (it would be good to add a test).

The priority flag is not set for this bug.
:gl, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(gl)
Flags: needinfo?(gl)
Priority: -- → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: