Last Comment Bug 655549 - CSS text color change not applied to elements in tables
: CSS text color change not applied to elements in tables
Status: VERIFIED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla7
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
: 77287 (view as bug list)
Depends on:
Blocks: 494117 645768
  Show dependency treegraph
 
Reported: 2011-05-07 21:01 PDT by James Babcock
Modified: 2013-12-27 14:19 PST (History)
9 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-


Attachments
sample html (359 bytes, text/html)
2011-05-07 21:29 PDT, Alice0775 White
no flags Details
and bug 645768. Rejigger the quirk table color rule to work more reliably. (15.53 KB, patch)
2011-05-24 19:06 PDT, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Splinter Review
With the comments addressed (16.12 KB, patch)
2011-05-27 19:27 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review

Description James Babcock 2011-05-07 21:01:00 PDT
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 Alice0775 White 2011-05-07 21:29:38 PDT
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
Comment 2 Alice0775 White 2011-05-07 21:33:54 PDT
Oops,
In local build:
build from 8d309d7e0746 : fails
build from dee1e84a95aa : fails
build from d594bc58ca6b : works
build from 9ec74c8e5690 : works
Comment 3 Boris Zbarsky [:bz] 2011-05-08 19:07:39 PDT
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.
Comment 4 Boris Zbarsky [:bz] 2011-05-08 19:19:39 PDT
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?
Comment 5 Boris Zbarsky [:bz] 2011-05-10 09:17:11 PDT
Or I could just store the body color on the prescontext.  I'll probably do that.
Comment 6 Asa Dotzler [:asa] 2011-05-10 15:16:42 PDT
Did we break the web badly here and how scary would the proposed fix be?
Comment 7 Boris Zbarsky [:bz] 2011-05-10 16:05:43 PDT
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 Asa Dotzler [:asa] 2011-05-11 11:54:41 PDT
Release drivers are not going to track this for Firefox 5.
Comment 9 Boris Zbarsky [:bz] 2011-05-24 19:06:19 PDT
Created attachment 534971 [details] [diff] [review]
and bug 645768.  Rejigger the quirk table color rule to work more reliably.
Comment 10 Johnathan Nightingale [:johnath] 2011-05-25 11:45:43 PDT
Looks like you accidentally re-nom'd (or re-nom'd without justification) --> back to minuses.
Comment 11 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-05-25 13:45:43 PDT
Haven't looked in detail yet, but I think I want the pres context member+API to be *BodyTextColor thar then *BodyColor
Comment 12 Boris Zbarsky [:bz] 2011-05-25 18:15:19 PDT
OK; that's easy enough to change.
Comment 13 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-05-27 15:10:24 PDT
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
Comment 14 Boris Zbarsky [:bz] 2011-05-27 19:27:01 PDT
> 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>....
Comment 15 Boris Zbarsky [:bz] 2011-05-27 19:27:30 PDT
Created attachment 535803 [details] [diff] [review]
With the comments addressed
Comment 16 Boris Zbarsky [:bz] 2011-06-01 09:06:41 PDT
http://hg.mozilla.org/projects/cedar/rev/9439ffe089a1
Comment 17 Mounir Lamouri (:mounir) 2011-06-02 04:18:40 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/9439ffe089a1
Comment 18 Virgil Dicu [:virgil] [QA] 2011-08-26 04:44:21 PDT
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.
Comment 19 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-11-15 13:26:17 PST
*** Bug 77287 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.