Closed
Bug 1082899
Opened 11 years ago
Closed 11 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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: dbaron, Assigned: dbaron)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
20.31 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
2.92 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
3.03 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
5.43 KB,
patch
|
birtles
:
review+
bzbarsky
:
feedback+
|
Details | Diff | Splinter Review |
3.09 KB,
patch
|
birtles
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
6.34 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Mentor: dbaron
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → dbaron
Mentor: dbaron
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #8508850 -
Flags: review?(birtles)
Assignee | ||
Comment 3•11 years ago
|
||
The code for this is basically copied from nsCSSPseudoElements.
Attachment #8508851 -
Flags: review?(birtles)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8508853 -
Flags: review?(birtles)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8508854 -
Flags: review?(birtles)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #8508855 -
Flags: review?(birtles)
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8508855 -
Flags: superreview?(bzbarsky)
Assignee | ||
Comment 9•11 years ago
|
||
Assignee | ||
Comment 10•11 years ago
|
||
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•11 years ago
|
||
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 12•11 years ago
|
||
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 13•11 years ago
|
||
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 14•11 years ago
|
||
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 15•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8508854 -
Flags: review?(birtles) → review+
Updated•11 years ago
|
Attachment #8508855 -
Flags: review?(birtles) → review+
Comment 16•11 years ago
|
||
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•11 years ago
|
||
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+
Assignee | ||
Comment 18•11 years ago
|
||
(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.
Assignee | ||
Comment 19•11 years ago
|
||
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
Assignee | ||
Comment 20•11 years ago
|
||
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•11 years ago
|
||
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
Whiteboard: [leave open]
Assignee | ||
Comment 22•11 years ago
|
||
I filed bug 1088179 for patch 6; this one should be marked fixed when patches 1-5 hit central.
Whiteboard: [leave open]
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 24•10 years ago
|
||
Please add a document to MDN: https://developer.mozilla.org/en-US/docs/new?slug=%3A-moz-native-anonymous&parent=1312
You need to log in
before you can comment on or make changes to this bug.
Description
•