1433389 - Expose input[type=number] pseudo-elements to chrome stylesheets

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Tim Nguyen :ntim]

In bug 1429573, we may want to use the pseudo elements in chrome stylesheets.

Unfortunately, they're only exposed to UA stylesheets.

Comment 1

[Tim Nguyen :ntim]

Jonathan, would it be ok to expose the pseudo elements in chrome stylesheets ? It'd be nice to style them within the Firefox UI part of the de-xbl work.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(I think we don't have chrome-only pseudo-elements right now, but it shouldn't be hard to implement I think, let me know if you want me to do that).

Comment 3

[Tim Nguyen :ntim]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> (I think we don't have chrome-only pseudo-elements right now, but it
> shouldn't be hard to implement I think, let me know if you want me to do
> that).

That would be great, thanks! It's currently the only blocker to landing bug 1429573 :)

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Tim Nguyen :ntim from comment #1)
> Jonathan, would it be ok to expose the pseudo elements in chrome stylesheets?

Sounds fine to me.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1433389: Make input[type=number] pseudo-elements accessible to chrome.

Review commit: https://reviewboard.mozilla.org/r/215864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215864/

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 8945758 [details]
Bug 1433389: Make input[type=number] pseudo-elements accessible to chrome.

https://reviewboard.mozilla.org/r/215864/#review222324

::: layout/style/nsCSSPseudoElements.h:42
(Diff revision 1)
>  // to whether the pseudo-element is tree-like, but we don't support these
>  // pseudo-classes on ::before and ::after generated content yet.  See
>  // http://dev.w3.org/csswg/selectors4/#pseudo-elements.
>  #define CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE  (1<<3)
> -// Is content prevented from parsing selectors containing this pseudo-element?
> -#define CSS_PSEUDO_ELEMENT_UA_SHEET_ONLY               (1<<4)
> +// Should this pseudo-element be enabled only for UA sheets?
> +#define CSS_PSEUDO_ELEMENT_ENABLED_IN_UA_SHEETS (1<<4)

Using the term "enabled" here didn't make any sense to me at first until I realized you were going for symmetry with CSS_PROPERTY_ENABLED_IN_*. In fact even then initially I thought the "enabled" in the existing CSS_PROPERTY_ENABLED_IN_* define names didn't make any sense either. For a casual reader those defines seems backwards. It seems like they should use the term "restricted to" rather than "enabled" since having an "enable" bit implicitly *disable* other things, as these can, is unexpected.

Digging into this code a bit more I realized the awkward naming seems to be due to the fact that these defines can optionally work with and modify the behavior of a disabling/enabling pref (not obvious from sites where they're referenced unfortunately). The situation seems to be that:

 * if a feature has an associated enabling/disabling pref
   these bits will force *enable* the feature to always be on
   in UA/chrome sheets

 * if there is no pref these bits will *restrict* the feature
   to only be available in UA/chrome sheets (and the feature
   will be *implicitly* (ugh!) disabled in author sheets)

The fact that in one scenario the defines enable, and in the other they disable, unfortunately makes naming them well difficult!

I have ideas on how the code could be changed so that the existing CSS_PROPERTY_ENABLED_IN_* define names could be changed to make sense in both scenarios given above, but I'm not sure it makes sense to pursue that here, or necessarily as a prerequisite of this bug. Right now CSS_PSEUDO_ELEMENT doesn't provide a way to hide pseudo-elements behind a pref, so there is no need for the defines you're changing to have "enable" in their names. We only have the second of the two scenarios given above, so really these flags are just to restrict behavior, and "_ONLY" in the define names would make a lot more sense. For now, I think you should keep that naming so as not to extend naming confusion into this code for the sake of symmetry (where we don't have it anyway).

::: layout/style/nsCSSPseudoElements.h:45
(Diff revision 1)
>  #define CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE  (1<<3)
> -// Is content prevented from parsing selectors containing this pseudo-element?
> -#define CSS_PSEUDO_ELEMENT_UA_SHEET_ONLY               (1<<4)
> +// Should this pseudo-element be enabled only for UA sheets?
> +#define CSS_PSEUDO_ELEMENT_ENABLED_IN_UA_SHEETS (1<<4)
> +// Should this pseudo-element be enabled only for UA sheets and chrome
> +// stylesheets?
> +#define CSS_PSEUDO_ELEMENT_ENABLED_IN_CHROME (1<<5)

There are three reasons I'd rather just have the one define right now:

 * having more adds complexity, runtime overhead and bloat to
   IsEnabled() below which is currently unnecessary

 * the CSS_PSEUDO_ELEMENT_ENABLED_IN_UA_SHEETS and
   CSS_PSEUDO_ELEMENT_ENABLED_IN_CHROME defines are essentially
   dead code right now

 * if we're to use the term "_ONLY" in the naming for now, it
   makes more sense if there's only one thing we're restricting
   to (being able to technically restrict "only" to UA and "only"
   to chrome sheets at the same time is awkward)

That is I'd rather have just one define called CSS_PSEUDO_ELEMENT_UA_AND_CHROME_SHEET_ONLY for now.

Like you I would like naming symmetry with CSS_PROPERTY_ENABLED_IN_*, but I'd just rather do that after cleaning up the naming of those existing defines which looks non-trivial and shouldn't block the UI folks making progress.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945758 [details]
Bug 1433389: Make input[type=number] pseudo-elements accessible to chrome.

https://reviewboard.mozilla.org/r/215864/#review222324

> Using the term "enabled" here didn't make any sense to me at first until I realized you were going for symmetry with CSS_PROPERTY_ENABLED_IN_*. In fact even then initially I thought the "enabled" in the existing CSS_PROPERTY_ENABLED_IN_* define names didn't make any sense either. For a casual reader those defines seems backwards. It seems like they should use the term "restricted to" rather than "enabled" since having an "enable" bit implicitly *disable* other things, as these can, is unexpected.
> 
> Digging into this code a bit more I realized the awkward naming seems to be due to the fact that these defines can optionally work with and modify the behavior of a disabling/enabling pref (not obvious from sites where they're referenced unfortunately). The situation seems to be that:
> 
>  * if a feature has an associated enabling/disabling pref
>    these bits will force *enable* the feature to always be on
>    in UA/chrome sheets
> 
>  * if there is no pref these bits will *restrict* the feature
>    to only be available in UA/chrome sheets (and the feature
>    will be *implicitly* (ugh!) disabled in author sheets)
> 
> The fact that in one scenario the defines enable, and in the other they disable, unfortunately makes naming them well difficult!
> 
> I have ideas on how the code could be changed so that the existing CSS_PROPERTY_ENABLED_IN_* define names could be changed to make sense in both scenarios given above, but I'm not sure it makes sense to pursue that here, or necessarily as a prerequisite of this bug. Right now CSS_PSEUDO_ELEMENT doesn't provide a way to hide pseudo-elements behind a pref, so there is no need for the defines you're changing to have "enable" in their names. We only have the second of the two scenarios given above, so really these flags are just to restrict behavior, and "_ONLY" in the define names would make a lot more sense. For now, I think you should keep that naming so as not to extend naming confusion into this code for the sake of symmetry (where we don't have it anyway).

Yeah, I think consistency is nice, but agreed, let's leave just a single UA_AND_CHROME_ONLY or something.

> There are three reasons I'd rather just have the one define right now:
> 
>  * having more adds complexity, runtime overhead and bloat to
>    IsEnabled() below which is currently unnecessary
> 
>  * the CSS_PSEUDO_ELEMENT_ENABLED_IN_UA_SHEETS and
>    CSS_PSEUDO_ELEMENT_ENABLED_IN_CHROME defines are essentially
>    dead code right now
> 
>  * if we're to use the term "_ONLY" in the naming for now, it
>    makes more sense if there's only one thing we're restricting
>    to (being able to technically restrict "only" to UA and "only"
>    to chrome sheets at the same time is awkward)
> 
> That is I'd rather have just one define called CSS_PSEUDO_ELEMENT_UA_AND_CHROME_SHEET_ONLY for now.
> 
> Like you I would like naming symmetry with CSS_PROPERTY_ENABLED_IN_*, but I'd just rather do that after cleaning up the naming of those existing defines which looks non-trivial and shouldn't block the UI folks making progress.

Sounds good, mind filing a bug with your proposed cleanup?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945758 [details]
Bug 1433389: Make input[type=number] pseudo-elements accessible to chrome.

https://reviewboard.mozilla.org/r/215864/#review222324

> Sounds good, mind filing a bug with your proposed cleanup?

(I mean with the thoughts on it, not with a patch, unless you want to of course ;))

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Actually, I just noticed something while implementing the proposed thing. Since stylo handles anon boxes as pseudo-elements, it'd expose anon-boxes to chrome, which is bad. Gecko has another codepath for this (all the nsCSSAnonBoxes stuff), but I think we still need the "ua only, but not chrome" bit.

Given that I'd rather land this, wdyt? Happy to make the naming not suck in a followup.

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 8945758 [details]
Bug 1433389: Make input[type=number] pseudo-elements accessible to chrome.

https://reviewboard.mozilla.org/r/215864/#review223538

Okay, let's go with this for now.

I should point out that I'm not familiar with the Stylo code, but from what I can see your changes make sense. I'll leave it up to you whether you think your changes could do with a once over from someone that's more familiar with it.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c524fa25f58
Make input[type=number] pseudo-elements accessible to chrome. r=jwatt

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7c524fa25f58