437366 - styled form controls (color property) not reacting correctly when 'Allow pages to choose their own colors...' is unchecked.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[philippe (part-time)]

Attachment: test case

STR
(these assume default theme at both browser and OS level - black text, white background).

1. load testcase
2. open Preferences > Content > Colors
3. adjust your colours: set dark/black background, light/white 'text' colour.
4. uncheck the 'Allow pages to choose their own colors...'

Actual result: watch the second row of form controls: the text in the form controls doesn't change colour (remains the default OS 'Text' colour)
Expected result: the text-colour in the styled form controls changes to the one specified in step 3 above.

Regression from FX 2. I suspect this regressed from bug 58048, but I haven't checked this yet

(Via the forums: http://forums.mozillazine.org/viewtopic.php?p=3398626#3398626)

Comment 1

[philippe (part-time)]

Attachment: screenshot

What I see on OS X - default settings, with the colour/background changes as indicated.

That is with
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008060417 Minefield/3.1a1pre

Camino trunk builds and WinXP FX 3 RC2 show the same

Comment 3

[Kai Liu]

A bit of searching turned up bug 419140 and bug 405207.  Those were about changing the system color scheme, while this is about changing the browser color scheme, but the underlying problem may be the same...

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It sounds like you're saying that you want bug 58048 to be reverted, although it's not completely clear.  If so, I think this should be wontfix.

Comment 5

[user2037]

I opened 437392 because styled form elements may be impossible to see because the background and border (if any) both match the surrounding background color.

Perhaps a more practical solution would be to enforce some minimum border (using the user's preferred, or system, foreground color) when form elements are styled and the user doesn't allow pages to use their own colors.

Comment 6

[Boris Zbarsky [:bzbarsky]]

It sounds like the real problem is that we're using a system color for the text but not for the background or something?  At least once we drop native theming...

That's odd, though.  forms.css certainly uses a system color for the background _and_ the text.  So the browser color settings shouldn't matter.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, I see what's going on here.  The author rule is higher precedence than the forms.css rule.  So we see that ruledata first.  We're ignoring it, and it has a background color style, so we set the background to user default (in this case black).  We do NOT set the color.

Then we process the ua rule.  There is already a background set, so the ua background is ignored.  But we do use the -moz-fieldtext color (in this case also  black).

We should probably also force the foreground color when we force the background.... Or something.

Comment 10

[Yehuda]

(In reply to comment #9)
> *** Bug 423954 has been marked as a duplicate of this bug. ***
> 

Some background material from bug 423954, in order to make the X in "  	 wanted1.9.0.x" to be as low as possible:

This bug is not about teenage boys who like to see everything in black, or teenage girls who like to see everything in pink. It is about giving people with sight impairment accessibility to the internet. See http://www.webaim.org/articles/visual/lowvision.php#fontsandbackground for a detailed explanation.

On top of that, Federal US lows (section 508) require that software programs and operation systems support this kind of user selectable colors. This means that Firefox 3.0.0 cannot be installed in any US Federal agency (I know that Firefox penetration in those places is low, but it might make it even lower). I guess that other countries have similar rules or regulations.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I tried giving this a shot, but the problem is that we resolve the color and background structs separately.  So we have no way to know that we clobbered the background when we go to get the color...

Maybe we should just set explicitly the color to default whenever there's an author-winning color and be done with it?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to do that

This reintroduces bug 58048 for only those form controls which the author styles.  Right now we have it for their backgrounds but not their colors, which can easily read to unreadable combinations.  So this is an improvement, though not much of one.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think what we should do here, ideally, is make author setting of background-color set the background-color to the background-color the nearest ancestor of the element (including itself as an ancestor) that has a non-transparent background color from the user or user agent style sheets.

I think that's hard, although I'd like to think about it a little more...

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What about the following alternative:

Make nsPresContext::HasAuthorSpecifiedRules begin with:

  if (!UseDocumentColors())
    return PR_FALSE;

Does that help?

Comment 17

[Boris Zbarsky [:bzbarsky]]

That would mean not dropping native theming when authors change the border style or width, or padding, if they're prohibited from changing colors.  The width and padding thing in particular could cause issues, I think.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What about removing the background bit from the bitmask in that case?

Comment 19

[Boris Zbarsky [:bzbarsky]]

That sounds much more promising.  Lemme sleep on it and test the effect tomorrow.

Comment 20

[Boris Zbarsky [:bzbarsky]]

That doesn't work, because the testcase also styles the border.  So we drop native theming anyway, and then follow the codepath in comment 7.

Comment 22

[Tchalvak]

I'm having this issue as well, from the description of 't all, sounds like a perfect acessment of the problem is under way (even if a final solution isn't at hand yet).  My system theme is respected for the dropdown chrome, but not for the dropdown text, resulting in dark on dark dropdown boxes.

A simple example image is loaded downstream in launchpad, viewable here:
http://launchpadlibrarian.net/20391950/darkDropdownsHaveDarkText.png

With the bug:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/220263

Comment 23

[Tchalvak]

Attachment: When using a dark system theme, Dark dropdowns have dark text, as per the images and bug filed on launchpad.

Comment 24

[Tchalvak]

Just to add to the assessment, it looks like a lot of the chrome elements interpret the system settings just fine, for example the chrome of the "Commit" button below the Additional Comments window that I'm writing in now shows just fine (with white text on the system-colored dark chrome), so perhaps duplicating whatever behavior the normal submit button chrome is using would fix the problem with the dropdowns.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Tchalvak, we know exactly why _this_ bug is happening.  If you're seeing something other than this bug, please file a separate bug report.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 336718 [details] [diff] [review]
Patch to do that

OK, r+sr=dbaron.  Sorry for thinking about it for so long.

But please file a followup bug on fixing form controls.

(I'm a little surprised we don't have a test for this... any chance you could add one?)

Comment 27

[Boris Zbarsky [:bzbarsky]]

> But please file a followup bug on fixing form controls.

Fixing what, exactly?  The bug 58048 regression?

I'll see what I can do as far as a testcase goes.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/f87b46d44d22 on m-c.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #27)
> Fixing what, exactly?  The bug 58048 regression?

Yes.


It's also worth getting this into 1.9.1.

Comment 30

[Boris Zbarsky [:bzbarsky]]

I filed bug 471345 (and requested branch approvals for this patch).

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmmm.  I think bug 471520 is probably a bad enough regression that we should back this out.  I should have thought of that...


I'm also actually not so sure comment 17 is that bad.  The only thing that it would be a problem for is border (we don't drop native theming for padding or width), and I'm not sure that's such a big problem.

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 336718 [details] [diff] [review]
Patch to do that

(As dbaron mentions, we can't take this on branch until the regression in bug 471520 is handled.)

Comment 33

[Boris Zbarsky [:bzbarsky]]

I backed out attachment 336718 [details] [diff] [review].

I guess if we're not worried about border width changes messing up layout (which I think I can live with), we can do comment 16.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: Per comment 16

I did test that this works fine.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 355502 [details] [diff] [review]
Per comment 16

r+sr=dbaron

Comment 36

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/fc12d81653dd

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 355502 [details] [diff] [review]
Per comment 16

This should be a much safer fix than the other that we tried.

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 355502 [details] [diff] [review]
Per comment 16

a191=beltzner

Comment 39

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0a9f0a5ee334

Comment 40

[Samuel Sidler (old account; do not CC)]

Boris, does your patch apply to 1.9.0?

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 355502 [details] [diff] [review]
Per comment 16

Yes.  I suppose we should take it there too.

Comment 42

[Daniel Veditz [:dveditz]]

Comment on attachment 355502 [details] [diff] [review]
Per comment 16

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.9.0 branch.

Comment 44

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.7 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7pre) Gecko/2009021705 GranParadiso/3.0.7pre (.NET CLR 3.5.30729).