Closed Bug 437366 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: phiw2, Assigned: bzbarsky)

References

Details

(4 keywords)

Attachments

(5 files)

Attached file 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)
Flags: blocking1.9.0.1?
Attached image 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
Severity: normal → major
OS: Mac OS X → All
Hardware: PC → All
Duplicate of this bug: 437392
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...
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.
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.
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.
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.
Blocks: 58048
Duplicate of this bug: 440319
Duplicate of this bug: 423954
Flags: wanted1.9.0.x?
Flags: blocking1.9.1?
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1-
(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.
Keywords: access
Duplicate of this bug: 443612
Duplicate of this bug: 450098
Flags: wanted1.9.0.x? → wanted1.9.0.x+
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?
Attached patch Patch to do thatSplinter Review
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.
Attachment #336718 - Flags: superreview?(dbaron)
Attachment #336718 - Flags: review?(dbaron)
Summary: styled form controls (colour property) not reacting correctly when 'Allow pages to choose their own colors...' is unchecked. → styled form controls (color property) not reacting correctly when 'Allow pages to choose their own colors...' is unchecked.
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...
Flags: blocking1.9.1? → wanted1.9.1+
What about the following alternative:

Make nsPresContext::HasAuthorSpecifiedRules begin with:

  if (!UseDocumentColors())
    return PR_FALSE;

Does that help?
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.
What about removing the background bit from the bitmask in that case?
That sounds much more promising.  Lemme sleep on it and test the effect tomorrow.
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.
Duplicate of this bug: 464262
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
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.
Tchalvak, we know exactly why _this_ bug is happening.  If you're seeing something other than this bug, please file a separate bug report.
Attachment #336718 - Flags: superreview?(dbaron)
Attachment #336718 - Flags: superreview+
Attachment #336718 - Flags: review?(dbaron)
Attachment #336718 - Flags: review+
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?)
> 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.
Pushed http://hg.mozilla.org/mozilla-central/rev/f87b46d44d22 on m-c.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #336718 - Flags: approval1.9.1?
Attachment #336718 - Flags: approval1.9.0.6?
(In reply to comment #27)
> Fixing what, exactly?  The bug 58048 regression?

Yes.


It's also worth getting this into 1.9.1.
Depends on: 471345
I filed bug 471345 (and requested branch approvals for this patch).
Depends on: 471520
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 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.)
Attachment #336718 - Flags: approval1.9.1?
Attachment #336718 - Flags: approval1.9.1-
Attachment #336718 - Flags: approval1.9.0.6?
Attachment #336718 - Flags: approval1.9.0.6-
Whiteboard: [needs bug 471520 fixed before taking on branches]
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Per comment 16Splinter Review
I did test that this works fine.
Assignee: nobody → bzbarsky
Status: REOPENED → ASSIGNED
Attachment #355502 - Flags: superreview?
Attachment #355502 - Flags: review?
Attachment #355502 - Flags: superreview?(dbaron)
Attachment #355502 - Flags: superreview?
Attachment #355502 - Flags: review?(dbaron)
Attachment #355502 - Flags: review?
Comment on attachment 355502 [details] [diff] [review]
Per comment 16

r+sr=dbaron
Attachment #355502 - Flags: superreview?(dbaron)
Attachment #355502 - Flags: superreview+
Attachment #355502 - Flags: review?(dbaron)
Attachment #355502 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Whiteboard: [needs bug 471520 fixed before taking on branches]
Comment on attachment 355502 [details] [diff] [review]
Per comment 16

This should be a much safer fix than the other that we tried.
Attachment #355502 - Flags: approval1.9.1?
Comment on attachment 355502 [details] [diff] [review]
Per comment 16

a191=beltzner
Attachment #355502 - Flags: approval1.9.1? → approval1.9.1+
Boris, does your patch apply to 1.9.0?
Comment on attachment 355502 [details] [diff] [review]
Per comment 16

Yes.  I suppose we should take it there too.
Attachment #355502 - Flags: approval1.9.0.7?
Attachment #355502 - Flags: approval1.9.0.7? → approval1.9.0.7+
Comment on attachment 355502 [details] [diff] [review]
Per comment 16

Approved for 1.9.0.7, a=dveditz for release-drivers.
Fixed on 1.9.0 branch.
Keywords: fixed1.9.0.7
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).
Depends on: 495798
You need to log in before you can comment on or make changes to this bug.