Closed Bug 1265639 Opened 8 years ago Closed 5 years ago
Add support for pseudo element aliases
58 bytes, text/x-review-board-request
To retain the backward compatibility when working on bug 775235 for pseudo elements or pseudo classes, both the prefix version and unprefix version need to be supported. That is, we need an alias mechanism for pseudo element and class. This will be needed for bug 205202 (::-moz-list-bullet and ::marker) and bug 509958 (::-moz-selection and ::selection)
Is it complicated enough that a "mechanism" is needed? In the past, I think we've just had two pseudo-classes in the pseudo-class list and implemented both the same way.
Take ::-moz-list-bullet for example, when constructing nsBulletFrame, we need to use CSSPseudoElementType::mozListBullet type to get the correct style for the nsBulletFrame. If we then implement ::maker type, we cannot just use the marker pseudo type to construct the nsBulletFrame. If we do, all the css rules for ::-moz-list-bullet will not be applied.  https://dxr.mozilla.org/mozilla-central/rev/67ac40fb8f680ea5e03805552187ba1b5e8392a1/layout/generic/nsBlockFrame.cpp#6911-6928
Comment #1 is correct that pseudo-classes do not need an alias mechanism, but pseudo-elements are still needed it because frame code often uses CSSPseudoElementType to resolve the correct style context.
Summary: Support for pseudo element and pseudo class aliasing → Add support for pseudo element aliases
The idea is to support parsing both pseudo-element and the alias, but given both of them the same pseudo-element type via nsCSSPseudoElements::GetPseudoType(). Review commit: https://reviewboard.mozilla.org/r/51021/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51021/
Comment on attachment 8749528 [details] MozReview Request: Bug 1265639 - Support pseudo element aliases. The idea behind the patch in comment 4, and un-prefixing ::-moz-selection (bug 509958) is a possible use case.
Attachment #8749528 - Flags: feedback?(dbaron)
What feedback are you looking for? (It also seem like whether to support ::selection is a separate question from how an alias mechanism should work, so they should be separate patches.)
I'm looking for the feedback of alias mechanism implementation. If it's on the right track, we might need to add another bool preference argument to the CSS_PSEUDO_ELEMENT_ALIAS like we did nsCSSPropList.h. Right, the support for ::selection should in a separate patch in bug 509958. I include it in the patch to demonstrate the alias usage. However, the mechanism might not worth much right now without a first use case. (::marker might be another good candidate, but I feel it's more complicated to implement.)
Flags: needinfo?(tlin) → needinfo?(dbaron)
Comment on attachment 8749528 [details] MozReview Request: Bug 1265639 - Support pseudo element aliases. Sorry for the delay getting to this. I think the idea of having pseudo-element aliases is fine. However, I would prefer not to expose them as public names on nsCSSPseudoElements::, since that would tend to lead people to incorrectly use them in code, which would result in their not doing anything (e.g., testing GetPseudo() on a style context against a member of nsCSSPseudoElements). If necessary, they could be part of their own atom list (nsCSSPseudoElementAliases::) if there's a need to have them atomized, although it's not clear to me there's any need for that. It might be fine if they're just included more directly into the code that converts aliases to the real values (currently nsCSSPseudoElements::GetPseudoType). The other problem here is that when an alias is used, I think you're creating a style context that has the unaliased GetPseudoType() but the aliased GetPseudo(), since you're doing the alias handling in nsCSSPseudoElements::GetPseudoType. In order to follow this approach without risk of code all over the tree getting aliases wrong, we might also need to remove the remaining uses of mPseudoTag from nsStyleContext, and switch all of them (and uses of GetPseudo(), of which there are many) to using GetPseudoType(). It's not clear to me whether this will be trivial or whether it will be hard to do. In the long term, it is probably what we'd like to do; having both GetPseudo() and GetPseudoType() should be unnecessary. If it's hard, it might make more sense to do the alias handling earlier in the parsing process. (In that case, we'd also need to make sure that aliases are handled correctly for getComputedStyle.)
Attachment #8749528 - Flags: feedback?(dbaron) → feedback-
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
Resolution: WONTFIX → DUPLICATE
You need to log in before you can comment on or make changes to this bug.