Closed Bug 1207084 Opened 9 years ago Closed 9 years ago

colors that are set to CSS variables do not obey browser.display.document_color_use (breaks in-content pages in high contrast mode)

Categories

(Core :: CSS Parsing and Computation, defect)

41 Branch
All
Windows 10
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 - wontfix
firefox42 + fixed
firefox43 + fixed
firefox44 + fixed

People

(Reporter: k.kolev1985, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150917150946

Steps to reproduce:

1. Switch to the high-contrast Black theme in Windows.
2. Open Firefox.
3. Click on "Tools" -> "Settings" (or "Options" - can't remember how it was called in english).
4. From the settings categories on the left, choose "Content".
5. Click on the "Colors button on the right.
6. In the "Colors" dialog, make sure that the option to ignore colors on pages is set to "Only with high-contrast themes". If it is not - change it accordingly and save the changes by clicking on the "OK" button in the "Colors" dialog.
7. Take a look of the text and background colors in the "Settings" page as a whole and in some pop-up dialogs like the one for setting up the colors.


Actual results:

1. The "Settings" page uses its own colors for the background, but the text color is taken from the currently used High-Contrast Black theme in Windows.

2. Some controls in pop-up dialogs (like the combo-boxes) don't fully obey the currently High-Contrast Black theme in Windows.

3. And some others (see the attached ZIP archive with screenshots).


Expected results:

The background, text and selection colors for the controls should obey the currently used High-Contrast Black theme in Windows.
Component: Untriaged → Disability Access
OS: Unspecified → Windows 10
Hardware: Unspecified → All
Version: unspecified → 41 Branch
Triage: Forwarding this to proper product and component.
Component: Disability Access → Preferences
Product: Firefox → Toolkit
Component: Preferences → Themes
Can you provide the zipfile you mentioned, or specifics about the controls that don't behave well beyond the background color not being correct?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(k_kolev1985)
Summary: Firefox Settings page doesn't obey Windows High-Contrast theme setting → Firefox Settings page doesn't work well with Windows High-Contrast themes with bright foreground colors
In the screenshots from the ZIP archive, it is obvious that the main "Preferences" screen of Firefox 41.0 doesn't obey the colors set in the currently used theme in Windows. The background color should be black, the text color should be white and the selection color should be cyan. If needs be, I'll provide screenshots from Firefox 40.0, where the behavior is correct.
Flags: needinfo?(k_kolev1985)
(In reply to Kostadin Kolev from comment #3)
> If needs
> be, I'll provide screenshots from Firefox 40.0, where the behavior is
> correct.

Oh, this is a regression? I don't need screenshots, but then I guess I should go find out when this broke (ie what patch broke it).
OK.

BTW: It seams that selection in list-boxes and drop-down list-boxes (combo-boxes) is not correct even in v40.0 of Firefox. In this case each selected list item should have a cyan background and black foreground. Instead, the selection background is black, the foreground - white and the selection is indicated only by a white rectangle around the selected item. Should we discuss this here, or should I create a new bug for it?
(In reply to Kostadin Kolev from comment #5)
> OK.
> 
> BTW: It seams that selection in list-boxes and drop-down list-boxes
> (combo-boxes) is not correct even in v40.0 of Firefox. In this case each
> selected list item should have a cyan background and black foreground.
> Instead, the selection background is black, the foreground - white and the
> selection is indicated only by a white rectangle around the selected item.
> Should we discuss this here, or should I create a new bug for it?

I've just figured out what caused the general issue here, so please file a specific new bug for that issue. Thank you!
Cameron, Seth says you know more about this, can you help?

As a trivial test case:

http://jsbin.com/quxazizale/edit?html,css,output

If you use Windows and enable a high contrast theme, the background of the output should no longer be blue.

If you're not on Windows, you can toggle the "Override the colors specified by the page with my selections above:" preference in the preferences/options (Content > Colors) to flip the pref, and the same should happen.
Component: Themes → CSS Parsing and Computation
Flags: needinfo?(cam)
Product: Toolkit → Core
Summary: Firefox Settings page doesn't work well with Windows High-Contrast themes with bright foreground colors → colors that are set to CSS variables do not obey browser.display.document_color_use (breaks in-content pages in high contrast mode)
(this regressed in about:preferences because of bug 1161156 ( https://hg.mozilla.org/integration/mozilla-inbound/rev/1d7104ec78e7 ) that started using lots of CSS variables for these pages)
CSSParserImpl::ParsePropertyWithVariableReferences calls mTempData.MapRuleInfoInto, which is nsCSSExpandedDataBlock::MapRuleInfoInto, which does call MapSinglePropertyInto.

My guess is that maybe aRuleData->mLevel is set to eUserSheet or eAgentSheet by the time we get to this, which makes ShouldIgnoreColors always return false, though I haven't checked this.  Maybe we need to reset aRuleData->mLevel based on a level from the rule tree, if we have it?  I don't see anything that makes aRuleData->mLevel be something reasonable here, at least.
Hrmpf. Maybe I'm wrong? On current nightly, it seems to work after a refresh using the simplified testcase, so clearly something more finnicky is going on here that's breaking it in the case of about:preferences... On the other hand, I can verify that if I open the devtools on about:preferences and replace the background styling of the XUL <page> with a plain CSS color name ("red"), it takes the system background color (when in high contrast mode), whereas the CSS variable seems to be overriding that system background color. So variables are definitely at play here, but it doesn't seem to be affecting web content in the same way. I'll try to look into this more tomorrow.
Flags: needinfo?(gijskruitbosch+bugs)
If David's idea is right, the finnicky-ness could be that mLevel is left at different values depending on which rule nodes get traversed in WalkRuleTree.  Can you try adding a

  aRuleData->mLevel = nsStyleSet::eAgentSheet;

above this line:

  https://dxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#2189

just to force ShouldIgnoreColors to false, and see what happens?

It sounds like in any case we need to be storing mLevel (and maybe mImportant?) on the token stream so that we can set it on the nsRuleData in here.
Is bug 1203651 a dupe?
(In reply to Loic from comment #12)
> Is bug 1203651 a dupe?

No, that's a dupe of a different bug...
(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Loic from comment #12)
> > Is bug 1203651 a dupe?
> 
> No, that's a dupe of a different bug...

Oh, hm, misled by the summary (see bug 1047595). No, I guess it's a dupe of this one.
Blocks: 1207508
(In reply to Cameron McCormack (:heycam) from comment #11)
> If David's idea is right, the finnicky-ness could be that mLevel is left at
> different values depending on which rule nodes get traversed in
> WalkRuleTree.  Can you try adding a
> 
>   aRuleData->mLevel = nsStyleSet::eAgentSheet;
> 
> above this line:
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.
> cpp#2189
> 
> just to force ShouldIgnoreColors to false, and see what happens?
> 
> It sounds like in any case we need to be storing mLevel (and maybe
> mImportant?) on the token stream so that we can set it on the nsRuleData in
> here.

That doesn't help - the issue is that in this case we expect the rules to be ignored because of high contrast mode - in other words, they should be treated as non-agent/user level stylesheets.

I tried setting the mLevel to eDocSheet, and that *did* fix the issue. Does that help? :-)
Flags: needinfo?(gijskruitbosch+bugs)
Also, if the real fix is hard to do quickly, it might make sense to temporarily just commit the assignment of eDocSheet to mLevel, on the assumption that UA and user sheets don't use variables (true for UA sheets... maybe not completely true for user sheets).
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #17)
> Also, if the real fix is hard to do quickly, it might make sense to
> temporarily just commit the assignment of eDocSheet to mLevel, on the
> assumption that UA and user sheets don't use variables (true for UA
> sheets... maybe not completely true for user sheets).

I've been looking at a "more correct" fix (which per earlier comments sounded like "put the level on the tokenstream"), but obviously I'm not at all at home in this code...

It seems to me like:
a) token streams get constructed by the CSSParser based on a sheet
b) sheets don't know their level because that's determined by the styleset they're in
c) the CSSParser has no idea about stylesets
d) there doesn't seem to be a straightforward way to figure out at what level a sheet is associated with a styleset from within the sheet.

Did I misunderstand the idea here? I'm also not sure if sheets can be in different stylesets at different levels (in theory vs. in practice, I guess) and how that would affect all of this... I'm happy to keep poking at this, but a pointer or two would really help. :-)
I think you're right that you can't tell what level you're at within the nsCSSParser.  However I think there might be a simpler way to set the level on the nsCSSValueTokenStream -- just do it in nsCSSDataBlock::MapRuleInfoInto.  In here, we know that aRuleData->mLevel is accurate, since it's called in the earlier part of nsRuleNode::WalkRuleTree where we are setting mLevel, so any time we copy a token stream nsCSSValue into the aRuleData, we can copy mLevel into the nsCSSValueTokenStream.  We could start off with it being some particular "uninitialized" value, and assert that it's either "uninitialized" or the same as the value we're about to set it to.  And then in nsRuleNode::ResolveVariableReferences we can assert that it's not "uninitialized" and copy it back to aRuleData->mLevel.

Does that make sense?
Flags: needinfo?(cam)
And by nsCSSDataBlock::MapRuleInfoInto, I meant nsCSS{Expanded,Compressed}DataBlock::MapRuleInfoInto, although I suspect only the Compressed one is needed.
Bug 1207084 - keep sheet level on tokenstream for use in ruledata when resolving variables, to fix hcm issues with CSS variables, r?heycam
Attachment #8666419 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #21)
> And by nsCSSDataBlock::MapRuleInfoInto, I meant
> nsCSS{Expanded,Compressed}DataBlock::MapRuleInfoInto, although I suspect
> only the Compressed one is needed.

OK... I think I managed. This patch worked for the issue as reported in this bug in my testing... I've pushed to try from reviewboard to see if the assert turns up the need for me to add the same setter to the other datablock: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a10dcf3e1d5 (or if I'm going to end up breaking any tests...)
(In reply to :Gijs Kruitbosch from comment #23)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a10dcf3e1d5 (or if
> I'm going to end up breaking any tests...)

Looks like tests didn't run for some reason?
Comment on attachment 8666419 [details]
MozReview Request: Bug 1207084 - keep sheet level on tokenstream for use in ruledata when resolving variables, to fix hcm issues with CSS variables, r?heycam

https://reviewboard.mozilla.org/r/20549/#review18413

::: layout/style/nsCSSDataBlock.cpp:265
(Diff revision 1)
> +                if (val->GetUnit() == eCSSUnit_TokenStream) {

Can you add a comment in here explaining why we do this and why we do it at this point?

::: layout/style/nsRuleNode.cpp:2183
(Diff revision 1)
> +    uint16_t oldLevel = aRuleData->mLevel;

Using AutoRestore might be clearer:

```
  AutoRestore<uint16_t> saveLevel(aRuleData->mLevel);
```

r=me with comments addressed, assuming the assertion doesn't fire.
Attachment #8666419 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #24)
> (In reply to :Gijs Kruitbosch from comment #23)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a10dcf3e1d5 (or if
> > I'm going to end up breaking any tests...)
> 
> Looks like tests didn't run for some reason?

Yeah, I need to file this, it's a bug in either reviewboard's bind-autoland, or try syntax parser, depending on who you want to blame. I repushed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1deb3eeaad56

which looks pretty good, at a glance. Going to poke at the tidbits of orange (apparently W-e10s can be ignored, for instance...)
Thanks! So this just landed... assuming this is going to stick, Cameron, how do you feel about uplift here? It's still pretty early in the beta cycle, and the impact here is pretty bad, but I don't have a good overview of how scary this change is from a CSS/layout perspective.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
While this is a valid issue, I do not think it is something that we would do a dot release for. The criteria here is similar to RC week which includes top crashes/hangs, major regressions that makes the build unusable for a significant set of end-users, data loss scenarios without a good workaround, severe perf regressions, etc. This is not a wontfix for 41.
https://hg.mozilla.org/mozilla-central/rev/cf08a151e70d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(In reply to :Gijs Kruitbosch from comment #29)
> Thanks! So this just landed... assuming this is going to stick, Cameron, how
> do you feel about uplift here? It's still pretty early in the beta cycle,
> and the impact here is pretty bad, but I don't have a good overview of how
> scary this change is from a CSS/layout perspective.

It feels reasonably low risk to me; we're early in the beta cycle so let's request uplift.  I just noticed one more issue with the patch: can you make nsCSSValueTokenStream::operator== compare mLevels too?  Looking at where operator== is called from it is going to be equal anyway, but best to keep it accurate.
Flags: needinfo?(cam)
Attached patch Patch for betaSplinter Review
Comment on attachment 8667874 [details] [diff] [review]
Patch for beta

Approval Request Comment
[Feature/regressing bug #]: variable usage in browser styles
[User impact if declined]: users with high contrast mode or overriding website colors within Firefox can't read about:preferences and some other internal Firefox pages
[Describe test coverage new/current, TreeHerder]: no
[Risks and why]: pretty limited - should only affect how variables and the ignoring of builtin site colors interact
[String/UUID change made/needed]: no
Attachment #8667874 - Flags: review+
Attachment #8667874 - Flags: approval-mozilla-beta?
Attachment #8667874 - Flags: approval-mozilla-aurora?
Comment on attachment 8667874 [details] [diff] [review]
Patch for beta

Polish patch, taking it.
Attachment #8667874 - Flags: approval-mozilla-beta?
Attachment #8667874 - Flags: approval-mozilla-beta+
Attachment #8667874 - Flags: approval-mozilla-aurora?
Attachment #8667874 - Flags: approval-mozilla-aurora+
Attached patch Patch for auroraSplinter Review
Meh, conflicts between aurora and beta, but yeah.
Firefox 42 has partly fixed this bug. While the white-on-white (text-background) no longer appears in high-contrast color schemes, the color "samples" - "Text", "Background", "Visited Links" and "Unvisited" links appears as black. When any of those color "samples" are clicked, the color palette appears, but with all squares black. Thus, it is impossible to know - by looking at the color choice screen - which color choices have been made and - therefore - to change color choices.
(In reply to Jay Simkin from comment #45)
> Firefox 42 has partly fixed this bug. While the white-on-white
> (text-background) no longer appears in high-contrast color schemes, the
> color "samples" - "Text", "Background", "Visited Links" and "Unvisited"
> links appears as black. When any of those color "samples" are clicked, the
> color palette appears, but with all squares black. Thus, it is impossible to
> know - by looking at the color choice screen - which color choices have been
> made and - therefore - to change color choices.

This is a different bug. It's bug 1047595. It's been recently fixed, but the fix is not part of Firefox 42.
You need to log in before you can comment on or make changes to this bug.