Closed Bug 655549 Opened 14 years ago Closed 14 years ago

CSS text color change not applied to elements in tables

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla7
Tracking Flags:
Tracking Status
firefox5 - ---
firefox6 - ---

People

(Reporter: jimrandomh, Assigned: bzbarsky)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files, 1 obsolete file)

sample html
359 bytes, text/html
Details
With the comments addressed
16.12 KB, patch
Details | Diff | Splinter Review
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.
Attached file 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
Blocks: 494117
Status: UNCONFIRMED → NEW
tracking-firefox5: --- → ?
tracking-firefox6: --- → ?
Ever confirmed: true
Keywords: regression
Version: unspecified → 2.0 Branch
Oops, In local build: build from 8d309d7e0746 : fails build from dee1e84a95aa : fails build from d594bc58ca6b : works build from 9ec74c8e5690 : works
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.
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.
Release drivers are not going to track this for Firefox 5.
tracking-firefox5: ? → -
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]
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
Closed: 14 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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: