CSS text color change not applied to elements in tables

VERIFIED FIXED in mozilla7

Status

()

Core
CSS Parsing and Computation
P1
normal
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: James Babcock, Assigned: bz)

Tracking

({regression, testcase})

Trunk
mozilla7
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5-, firefox6-)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 

Below is a simplified version of luminous.elcenia.com (which might have a workaround applied now) that would toggle between black-on-white and white-on-black by setting the CSS "background-color" and "color" attributes on the body tag. The background-color would toggle correctly, but the color would remain unchanged, making the text black-on-black or white-on-black until something forced a full repaint, such as zooming the text.

Reproducible: Always

Steps to Reproduce:
<html>
<script src="http://ajax.aspnetcdn.com/ajax/jQuery/jquery-1.6.min.js"></script>
<script>
function changeColor()
{
	$("body").css({ "color": "red" });
}
</script>
<body style="background-color: white; color: black">
	<table>
		<tr><td>
		This text should turn red when you click the button
		</tr></td>
	</table>
	<button onclick="changeColor()">Change color</button>
</body>
</html>

Actual Results:  
Clicking the "change color" button has no effect until something forces a full repaint, such as zooming with ctrl+scrollwheel. (Note that Firebug will sometimes force a repaint if its console is open, too.)

Expected Results:  
Clicking the button should immediately change the text color

The use of a table-based layout seems significant; if the <table> wrapping the text is removed then it works.

Comment 1

6 years ago
Created attachment 530892 [details]
sample html

Regression window(cached m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/88fa0b783306
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100618 Minefield/3.7a6pre ID:20100618085618
Fails:
http://hg.mozilla.org/mozilla-central/rev/d4156799e66a
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100617 Minefield/3.7a6pre ID:20100618100908
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=88fa0b783306&tochange=d4156799e66a

In local build:
build from 8d309d7e0746
build from dee1e84a95aa : fails
build from d594bc58ca6b : works
build from 9ec74c8e5690
Triggerd by:
dee1e84a95aa	Boris Zbarsky — Bug 494117 part 2. Don't force selector matching on the whole subtree rooted at an element when the element's style changes. r=dbaron

Updated

6 years ago
Blocks: 494117
Status: UNCONFIRMED → NEW
tracking-firefox5: --- → ?
tracking-firefox6: --- → ?
Ever confirmed: true
Keywords: regression
Version: unspecified → 2.0 Branch

Comment 2

6 years ago
Oops,
In local build:
build from 8d309d7e0746 : fails
build from dee1e84a95aa : fails
build from d594bc58ca6b : works
build from 9ec74c8e5690 : works
Keywords: testcase
Ah, the same wonderful code as bug 645768 deals with.  Marking dependent for now, but I think this may be a pretty different problem.  I'll look into it more tomorrow.
Depends on: 645768
Assignee: nobody → bzbarsky
Priority: -- → P1
Version: 2.0 Branch → Trunk
So the fundamental issue here is that when the body's _computed_ color changes that changes the rule identity of nsHTMLStyleSheet::mDocumentColorRule, which means that all tables need a style reresolve.

That's pretty messed up, and is the underlying cause of this bug; the failure to get the computed color to start with is the cause of bug 645768.

I can probably fix this bug with sufficient hackery by detecting quirks-mode changes in body computed color and triggering a full reresolve on all descendants.

The other option is to rename mDocumentColorRule to mTableQuirkColorRule and have that rule always do the same thing: set the specified color to a new enumerated value that means "inherit from body".  Then in rulenode we'd need to find the right thing to inherit.  One simple way to do that would be to add a new member to nsStyleColor that's the body color; we could have yet another rule in nsHTMLStyleSheet that causes that to be set to something other than "inherit" for <body>.  I'm leaning toward that plan, since it will fix both this bug and bug 645768 in a non-hacky way.. except for bloating nsStyleColor, of course.

David, any objections?
Blocks: 645768
No longer depends on: 645768
Or I could just store the body color on the prescontext.  I'll probably do that.

Comment 6

6 years ago
Did we break the web badly here and how scary would the proposed fix be?
We broke the specific case of a page that:

1)  Is in quirks mode.
2)  Dynamically changes the color of the body.
3)  Expects tables to inherit the changed color (i.e. has no color styles on
    those tables).

The change that broke this shipped in Firefox 4 beta 1 back in June or July of last year.  This is the first report of it.

I wouldn't characterize this as bad breakage in the "stop the presses" sense.  It's definitely a bug we need to fix.

I think the proposed fix will be fairly non-scary; a bigger problem is that I have other things on my plate too and dbaron (who would need to review) is on vacation for the next several weeks.  So getting this done, reviewed, and landed on m-c and aurora in the next week (before the beta merge for Fx5) is likely to be tough.  I'm certainly hoping to get this on m-c before the Fx6 aurora merge.

Comment 8

6 years ago
Release drivers are not going to track this for Firefox 5.
tracking-firefox5: ? → -

Updated

6 years ago
tracking-firefox6: ? → -
Created attachment 534971 [details] [diff] [review]
and bug 645768.  Rejigger the quirk table color rule to work more reliably.
Attachment #534971 - Flags: review?(dbaron)
tracking-firefox5: - → ?
tracking-firefox6: - → ?
Whiteboard: [need review]
Looks like you accidentally re-nom'd (or re-nom'd without justification) --> back to minuses.
tracking-firefox5: ? → -
tracking-firefox6: ? → -
Haven't looked in detail yet, but I think I want the pres context member+API to be *BodyTextColor thar then *BodyColor
OK; that's easy enough to change.
Comment on attachment 534971 [details] [diff] [review]
and bug 645768.  Rejigger the quirk table color rule to work more reliably.

In nsStyleSet::GetContext, I'd rather it be a quirks mode check than an
IsHTML check.  We only need the data for quirks mode, but it seems more
dangerous to add it for all non-XHTML HTML since then we'd be more
likely to use it for something in standards mode and not notice the
XHTML bug.

As I said in comment 11, please rename the pres context
member+API to be *BodyTextColor rather than *BodyColor.  It should also
(per above) have comments about how it's quirks mode only.

Please comment somewhere (maybe in SetColor) that you're handling 
dynamic changes by relying on new style contexts being created down the
tree when the body's color changes (but you don't rely on rerunning rule
matching).

If you think the improvement in user style sheet handling by adding the
UseDocumentColors() check is worthwhile, maybe add a test for that?

In SetColor, could you assert that we're in quirks mode?  Note
that that will also have to merge with zwol's patch for infallible
allocation in the parser.

Also, why not just put the nsStyleSet changes in the one ResolveStyleFor
you need (and thus have fewer conditions)?

r=dbaron with that
Attachment #534971 - Flags: review?(dbaron) → review+
> In nsStyleSet::GetContext, I'd rather it be a quirks mode check

Done.

> please rename the pres context member+API

Done.

> It should also (per above) have comments about how it's quirks mode only

Done.

> Please comment somewhere (maybe in SetColor)

Done:

    // We just grab the color from the prescontext, and rely on the fact that
    // if the body color ever changes all its descendants will get new style
    // contexts (but NOT necessarily new rulenodes).

> If you think the improvement in user style sheet handling by adding the
> UseDocumentColors() check is worthwhile, maybe add a test for that?

Hmm.  I don't think we actually want that UseDocumentColors() check, given that.  I'll take it out.  Good catch.

> In SetColor, could you assert that we're in quirks mode? 

Done.

> Also, why not just put the nsStyleSet changes in the one ResolveStyleFor

Because a color change on <html> wouldn't hit ResolveStyleFor on the <body>....
Whiteboard: [need review] → [need landing]
Created attachment 535803 [details] [diff] [review]
With the comments addressed
Attachment #534971 - Attachment is obsolete: true
http://hg.mozilla.org/projects/cedar/rev/9439ffe089a1
Flags: in-testsuite+
Whiteboard: [need landing] → fixed-in-cedar
Target Milestone: --- → mozilla7
Pushed:
http://hg.mozilla.org/mozilla-central/rev/9439ffe089a1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux i686; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Verified fixed on Ubuntu 11.04, Mac OS 10.6, Windows 7, Windows XP using the test case provided in comment 1.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 77287
You need to log in before you can comment on or make changes to this bug.