Closed Bug 1082899 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

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.)
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).
Assignee: nobody → dbaron
Mentor: dbaron
Status: NEW → ASSIGNED
The code for this is basically copied from nsCSSPseudoElements.
Attachment #8508851 - Flags: review?(birtles)
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.
Attachment #8508856 - Flags: review?(birtles)
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.)
Attachment #8508854 - Flags: feedback?(bzbarsky)
Attachment #8508855 - Flags: superreview?(bzbarsky)
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 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?
Attachment #8508854 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8508855 [details] [diff] [review]
patch 5 - Add :-moz-native-anonymous pseudo-class, exposed only to UA style sheets

sr=me
Attachment #8508855 - Flags: superreview?(bzbarsky) → superreview+
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.
Attachment #8508850 - Flags: review?(birtles) → review+
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
Attachment #8508851 - Flags: review?(birtles) → review+
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.
Attachment #8508853 - Flags: review?(birtles) → review+
Attachment #8508854 - Flags: review?(birtles) → review+
Attachment #8508855 - Flags: review?(birtles) → review+
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 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)?
Attachment #8508856 - Flags: review?(birtles) → review+
(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.
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
I filed bug 1088179 for patch 6; this one should be marked fixed when patches 1-5 hit central.
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.