1429248 - input[type=color] color-swatch has its color overriden with document colors disabled

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tim Nguyen :ntim]

STR:
- Disable document colors from about:preferences
- Go to data:text/html,<input type="color"/>

The color swatch has its color overriden with document colors disabled.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

So, we were talking about what the cleanest way of doing this was... Some options considered:

 * Special-casing NAC subtrees to avoid using doc colors... Doable in theory, but kind of annoying because pseudo-elements are user-stylable, _but_ the only case we care about are actually pseudo-elements.
 * Adding a chrome-only CSS property or something to control which properties are affected by this. Actually annoying, since we need Color to be an early property, and this would need to be the same, so suddenly you don't get deterministic order.
 * Special-casing the ::-moz-color-swatch pseudo. Kind of nasty, but... :)

Cam, any other idea?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1429248: Special-case pseudo style attributes to avoid ignoring doc colors.

After a bit of thought this seems the easiest, but let me know if you think
otherwise.

We could also move the special-case to the other background-color special-case
that we have below the is_ignored_when_colors_disabled check for transparent
colors, but that didn't seem as clean to me. Happy to change it though.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

That patch implements the third option discussed above. Not sold on that though, so if you think there's a cleaner way...

Comment 4

[Cameron McCormack (:heycam)]

Can we allow document colors in all NAC subtrees specified by style="" attributes?  We can check for CascadeLevel::StyleAttribute{,Important}.  Though all NAC subtrees also includes SVG <use> shadow trees, where we want to ignore any colors from style="" attributes.  (Ignoring the point that SVG content is often pretty useless if you ignore document colors...)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Well we could, but then we need to thread that information through StyleResolver via CascadeFlags or what not, which I thought it wasn't that great.

But I can do that I guess. Do you know if there's a way to detect <use> subtrees without walking up the tree? It'd be unfortunate having to do that all the time just to thread a flag that may not end up being used...

Comment 6

[Cameron McCormack (:heycam)]

There's no way to tell if you're in an SVG <use> shadow tree without searching up the tree, I think, so special casing sounds OK.

Comment 7

[Cameron McCormack (:heycam)]

There's also ::cue, which uses a style="" attribute to apply styles that come from WebVTT files.  Can you check whether we need to handle that too?  If so, then maybe we should base this on the CSS_PSEUDO_ELEMENT_SUPPORTS_STYLE_ATTRIBUTE flag instead of hard coding the pseudo name.

Also, do we need to check for background-color specifically?  Can we just allow any color-related property that comes from style=""?

Comment 8

[Cameron McCormack (:heycam)]

Actually I'm not certain if the style="" is on the element corresponding to ::cue itself, but there are other style="" attributes set with colors such as this:

https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/dom/media/webvtt/vtt.jsm#622-645

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941247 [details]
Bug 1429248: Special-case pseudo style attributes to avoid ignoring doc colors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211516/diff/1-2/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I updated it to allow pseudo-element style attributes for now.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941247 [details]
Bug 1429248: Special-case pseudo style attributes to avoid ignoring doc colors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211516/diff/2-3/

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8941247 [details]
Bug 1429248: Special-case pseudo style attributes to avoid ignoring doc colors.

https://reviewboard.mozilla.org/r/211516/#review219218

r=me after the commit message is updated to be more like a commit message. :-).  (Although in general I do like to review commit messages...)

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Fixed in https://hg.mozilla.org/mozilla-central/rev/1b62dd209727c2482150340c669c3957d9920670