1082899 - implement a pseudo-class that matches native-anonymous content, exposed only to UA style sheets (or maybe also chrome)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron:]

Per bug 1020244 comment 75 and bug 1020244 comment 79, we should implement a pseudo-class that matches native-anonymous content.  This will allow the UA stylesheet to select such content without polluting a namespace that is available to Web content authors.

We should use this both for the work in bug 1020244 and to replace the _moz_anonclass stuff already in ua.css.  (That, in turn, should use the class attribute to benefit from the performance optimizations we do for the class attribute.)

This also requires that we add a mechanism to make pseudo-classes available only to UA style sheets.  We already have such a mechanism for pseudo-elements (see nsCSSPseudoElements, nsCSSPseudoElementList); pseudo-classes can have a similar mechanism.

(Probably chrome-only, which is what we have for pseudo-elements, is good enough, and we don't need to limit it to UA-only.)

Comment 1

[David Baron :dbaron:]

And it looks like this will work for both of those use cases, since both are cases of content created in an nsIAnonymousContentCreator::CreateAnonymousContent method, and nsCSSFrameConstructor::GetAnonymousContent calls content->SetIsNativeAnonymousRoot() on the nodes returned by CreateAnonymousContent, so it should already be native-anonymous (as it should be).

Comment 2

[David Baron :dbaron:]

Attachment: patch 1 - Add a flags field to the CSS_PSEUDO_CLASS and related macros

Comment 3

[David Baron :dbaron:]

Attachment: patch 2 - Add flags storage and accessors for nsCSSPseudoClasses

The code for this is basically copied from nsCSSPseudoElements.

Comment 4

[David Baron :dbaron:]

Attachment: patch 3 - Add flag for marking pseudo-classes as UA-sheet only

Comment 5

[David Baron :dbaron:]

Attachment: patch 4 - Rename existing flag for pseudo-elements to better match what it is (UA sheets, not chrome sheets, although EditorOverride is included)

Comment 6

[David Baron :dbaron:]

Attachment: patch 5 - Add :-moz-native-anonymous pseudo-class, exposed only to UA style sheets

Comment 7

[David Baron :dbaron:]

Attachment: patch 6 - Convert touch caret code to using :-moz-native-anonymous so that it doesn't expose styles for magic attribute names to content

I tested locally on a Flame that using this without the ua.css changes
breaks the touch caret (it doesn't appear), but with the ua.css changes
the touch caret appears again.

Comment 8

[David Baron :dbaron:]

Comment on attachment 8508854 [details] [diff] [review]
patch 4 - Rename existing flag for pseudo-elements to better match what it is (UA sheets, not chrome sheets, although EditorOverride is included)

Boris, just want to check that this matches your understanding of what mUnsafeRulesEnabled means.  (I think I traced through the paths that set it.)

Comment 9

[David Baron :dbaron:]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1aaf6b6e194f
https://tbpl.mozilla.org/?tree=Try&rev=1aaf6b6e194f

Comment 10

[David Baron :dbaron:]

Also note that a few of the selectors in patch 6 have repeated classes now; I wanted it that way because I didn't want to change the relative specificity of any of the touchcaret selectors.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8508854 [details] [diff] [review]
patch 4 - Rename existing flag for pseudo-elements to better match what it is (UA sheets, not chrome sheets, although EditorOverride is included)

This matches all the uses of mUnsafeRulesEnabled except one: in CSSStyleSheet::ParseSheet.  I think that one is just buggy and should probably be changed to pass false or throw on attempts to reparse UA sheets or something.  Please file a followup?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8508855 [details] [diff] [review]
patch 5 - Add :-moz-native-anonymous pseudo-class, exposed only to UA style sheets

sr=me

Comment 13

[Brian Birtles (:birtles)]

Comment on attachment 8508850 [details] [diff] [review]
patch 1 - Add a flags field to the CSS_PSEUDO_CLASS and related macros

>diff --git a/layout/style/nsCSSPseudoClassList.h b/layout/style/nsCSSPseudoClassList.h
>--- a/layout/style/nsCSSPseudoClassList.h
>+++ b/layout/style/nsCSSPseudoClassList.h

The top of this file contains a description of the different arguments to the macros. We should include a description of the flags argument there.

r=birtles with description added.

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8508851 [details] [diff] [review]
patch 2 - Add flags storage and accessors for nsCSSPseudoClasses

>diff --git a/layout/style/nsCSSPseudoClasses.cpp b/layout/style/nsCSSPseudoClasses.cpp
>--- a/layout/style/nsCSSPseudoClasses.cpp
>+++ b/layout/style/nsCSSPseudoClasses.cpp
>@@ -27,16 +27,28 @@ using namespace mozilla;
> 
> static const nsStaticAtom CSSPseudoClasses_info[] = {
> #define CSS_PSEUDO_CLASS(name_, value_, flags_, pref_) \
>   NS_STATIC_ATOM(name_##_pseudo_class_buffer, &sPseudoClass_##name_),
> #include "nsCSSPseudoClassList.h"
> #undef CSS_PSEUDO_CLASS
> };
> 
>+// Separate from the array above so that we can have an array of
>+// nsStaticAtom (to pass to NS_RegisterStaticAtoms and
>+// nsAtomListUtils::IsMember), but with corresponding indices (so the
>+// i-th element of this array is the flags for the i-th pseudo-element
>+// in the previous array).
>+static const uint32_t CSSPseudoClasses_flags[] = {
>+#define CSS_PSEUDO_CLASS(name_, value_, flags_, pref_) \
>+  flags_,
>+#include "nsCSSPseudoClassList.h"
>+#undef CSS_PSEUDO_CLASS
>+};

This comment took me a moment to understand. I think it would be more clear if the first part appeared before CSSPseudoClasses_info. e.g. "Array of nsStaticAtom for passing to NS_RegisterStaticAtoms etc." then "Parallel array to containing the flags for each corresponding pseudo-element in CSSPseudoClasses_info" or something of that nature.

>+/* static */ uint32_t
>+nsCSSPseudoClasses::FlagsForPseudoClass(const Type aType)
>+{
>+  size_t index = static_cast<size_t>(aType);
>+  NS_ASSERTION(index < ArrayLength(CSSPseudoClasses_flags),
>+               "argument must be a pseudo-element");

This should be "pseudo-class".

r=birtles with comments addressed

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8508853 [details] [diff] [review]
patch 3 - Add flag for marking pseudo-classes as UA-sheet only

>diff --git a/layout/style/nsCSSPseudoClasses.h b/layout/style/nsCSSPseudoClasses.h
>--- a/layout/style/nsCSSPseudoClasses.h
>+++ b/layout/style/nsCSSPseudoClasses.h
>@@ -5,16 +5,19 @@
> 
> /* atom list for CSS pseudo-classes */
> 
> #ifndef nsCSSPseudoClasses_h___
> #define nsCSSPseudoClasses_h___
> 
> #include "nsStringFwd.h"
> 
>+// This pseudo-element is accepted only in UA style sheets.
>+#define CSS_PSEUDO_CLASS_UA_SHEET_ONLY                 (1<<0)
>+

I'd prefer we used an enum type for this for type-safety and better documentation (e.g. all the zeros added in part 1 would be a little easier to make sense of). I think we have MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS if we need to do bitwise operations.

Is that possible and practical?


r=me anyway but I'd be interested to know what you think.

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8508855 [details] [diff] [review]
patch 5 - Add :-moz-native-anonymous pseudo-class, exposed only to UA style sheets

Oh, I forgot to ask,

>diff --git a/layout/style/nsCSSPseudoClassList.h b/layout/style/nsCSSPseudoClassList.h
...
> // Match nodes that are HTML but not XHTML
> CSS_PSEUDO_CLASS(mozIsHTML, ":-moz-is-html", 0, "")
> 
>+CSS_PSEUDO_CLASS(mozNativeAnonymous, ":-moz-native-anonymous",
>+                 CSS_PSEUDO_CLASS_UA_SHEET_ONLY, "")
>+

Does this need a comment?

Comment 17

[Brian Birtles (:birtles)]

Comment on attachment 8508856 [details] [diff] [review]
patch 6 - Convert touch caret code to using :-moz-native-anonymous so that it doesn't expose styles for magic attribute names to content

>diff --git a/layout/style/ua.css b/layout/style/ua.css
>--- a/layout/style/ua.css
>+++ b/layout/style/ua.css
>@@ -304,89 +304,89 @@ parsererror|sourcetext {
>   font-family: -moz-fixed;
>   margin-top: 2em;
>   margin-bottom: 1em;
>   color: red;
>   font-weight: bold;
>   font-size: 12pt;
> }
> 
>-div[\_moz_anonclass="mozTouchCaret"].moz-touchcaret,
>-div[\_moz_anonclass="mozTouchCaret"].moz-selectioncaret-left,
>-div[\_moz_anonclass="mozTouchCaret"].moz-selectioncaret-right {
>+:-moz-native-anonymous.moz-touchcaret.moz-touchcaret,
>+:-moz-native-anonymous.moz-touchcaret.moz-selectioncaret-left,
>+:-moz-native-anonymous.moz-touchcaret.moz-selectioncaret-right {

I'm curious about the '.moz-touchcaret.moz-touchcaret' here. That seems redundant but I may be missing something. Is this to increase specificity?

It seems like we could just use:

  :-moz-native-anonymous.moz-touchcaret {

here?

Likewise in 3 other places in this file.


r=me with either appropriate simplifications to the selectors or a comment about why we need the redundant class selectors.

Also, why do we only convert the touch caret code and not other uses of _moz_anonclass (e.g. in editor)?

Comment 18

[David Baron :dbaron:]

(In reply to Brian Birtles (:birtles) from comment #15)
> I'd prefer we used an enum type for this for type-safety and better
> documentation (e.g. all the zeros added in part 1 would be a little easier
> to make sense of). I think we have MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS if
> we need to do bitwise operations.
> 
> Is that possible and practical?

I think it is.  I can do this in a followup patch.


(In reply to Brian Birtles (:birtles) from comment #17)
> I'm curious about the '.moz-touchcaret.moz-touchcaret' here. That seems
> redundant but I may be missing something. Is this to increase specificity?

I didn't want to change the specificity because I wasn't sure how to test that my changes were still correct.  I'll note that in the commit message.

> Also, why do we only convert the touch caret code and not other uses of
> _moz_anonclass (e.g. in editor)?

I thought the touch caret code was higher priority since it's all content rather than just edited content; I filed bug 1086641 on converting the editor uses.

Comment 19

[David Baron :dbaron:]

I filed followup bugs:

For comment 11:
bug 1088120 - fix incorrect setting of mUnsafeRulesEnabled in CSSStyleSheet::ParseSheet

For comment 15:
bug 1088121 - convert flags on nsCSSPseudoElements and nsCSSPseudoClasses to enum or enum class

And I had also previously filed:

For comment 17:
bug 1086641 - convert editor styles from _moz_anonclass to :-moz-native-anonymous

Comment 20

[David Baron :dbaron:]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/25a3e04caeed
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c66f58d9a401
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6925c2f5d701
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/858d2174b9c1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/550bb1f5b801
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac58452ef7d

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Patch 6 backed out for test_selectioncarets.py failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/83cf4504a5f8

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3253484&repo=mozilla-inbound

Comment 22

[David Baron :dbaron:]

I filed bug 1088179 for patch 6; this one should be marked fixed when patches 1-5 hit central.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/25a3e04caeed
https://hg.mozilla.org/mozilla-central/rev/c66f58d9a401
https://hg.mozilla.org/mozilla-central/rev/6925c2f5d701
https://hg.mozilla.org/mozilla-central/rev/858d2174b9c1
https://hg.mozilla.org/mozilla-central/rev/550bb1f5b801

Comment 24

[yiorsi]

Please add a document to MDN: https://developer.mozilla.org/en-US/docs/new?slug=%3A-moz-native-anonymous&parent=1312