Closed
Bug 94688
Opened 23 years ago
Closed 21 years ago
black text instead of white
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: zheka, Assigned: MatsPalmgren_bugz)
References
()
Details
(Keywords: regression, testcase)
Attachments
(5 files, 4 obsolete files)
3.26 KB,
text/html
|
Details | |
414 bytes,
text/html
|
Details | |
347 bytes,
text/html
|
Details | |
3.04 KB,
text/html
|
Details | |
5.28 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
From Bugzilla Helper: User-Agent: Mozilla/5.0 (X11; U; Linux i586; en-US; rv:0.9.3) Gecko/20010810 BuildID: 0.9.3 Mozilla displays text 'black on blue' instead 'white on blue'. Seems like it does not interpretes TEXT="#FFFFFF" in the <body> section Reproducible: Always Steps to Reproduce: 1.load URL 2. 3. Actual Results: white on blue text Expected Results: black on blue
Comment 1•23 years ago
|
||
The crap inserted before <html> cause automatic creation of a <body> tag, most likely. Then the second body tag (the one from the page) is ignored. Over to parser.
Assignee: asa → harishd
Component: Browser-General → Parser
QA Contact: doronr → bsharma
Comment 2•23 years ago
|
||
Comment 3•23 years ago
|
||
The javascript code in question doesn't look like it creates a body tag, but it does look like it breaks the hell out of the user's web page. It's before the frst html, and creates what looks like tables in Div's. Somewhere I think we're getting confused by the non compliant code and trashing the colors.
Assignee | ||
Comment 4•23 years ago
|
||
Assignee | ||
Comment 5•23 years ago
|
||
Confirming, build 2001-08-09-04 on Windows 98 SE.
Content model for testcase id 45582: docshell=011CADD0 html@0221FD60 refcount=8< head@0221BFC0 refcount=2< title@02CCE7C0 refcount=2< Text@02CCE760 refcount=2<Testcase for bug 94688> > > body@022146E0 bgcolor=#cccccc text=#ffffff refcount=3< table@02CCC5F0 border=1 refcount=6< Text@02CCC400 refcount=2<\n > tbody@02CCC310 refcount=3< tr@02CCC220 refcount=3< td@02CCC130 refcount=4< Text@02CCC0D0 refcount=3<TABLE before HTML> > > Text@02CCC030 refcount=2<\n> > > Text@02CCDE60 refcount=3<\n\n\n\n\n> table@02CCFF00 border=1 refcount=6< Text@02CCFEA0 refcount=2<\n > tbody@02CCFD90 refcount=3< tr@02CCFD30 refcount=3< td@02CCFCD0 refcount=4< Text@02CCFC70 refcount=3<TABLE in BODY - This text should be white.> > > Text@02CCFBB0 refcount=2<\n> > > Text@02CCC6E0 refcount=3<\n\n> p@02CCF9F0 refcount=3< Text@02CCF990 refcount=3<P in BODY - This text should be white.> > Text@02CCF810 refcount=3<\n\n> table@02CCF7B0 border=1 refcount=6< Text@02CCF6E0 refcount=2<\n > tbody@02CCF650 refcount=3< tr@02CCF5F0 refcount=3< td@02CCF590 refcount=4< Text@02CCF530 refcount=3<TABLE in BODY - This text should be white.> > > Text@02CCF470 refcount=2<\n> > > Text@02CCF870 refcount=3<\n\n\n> > > I'm begining to wonder if this is a layout bug. CCing Marc.
Comment 7•23 years ago
|
||
Note that this works fine in Standard mode, but not Quirks mode. My guess is it is the 'block font style inheritance into tables' problem again.
Assignee | ||
Comment 8•23 years ago
|
||
Assignee | ||
Comment 9•23 years ago
|
||
This testcase (attachment 46352 [details]) is the same as the last one except that I
removed the misplaced TABLE (before HTML). Now the color is inherited.
My guess is that when Parser moves the misplaced TABLE inside BODY the style
structures is messed up in some way.
Comment 10•23 years ago
|
||
This is definitely a layout problem. Reassigning to Marc [ sorry! :-( ].
Assignee: harishd → attinasi
Updated•23 years ago
|
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Updated•23 years ago
|
Target Milestone: mozilla0.9.5 → mozilla1.0
Comment 11•23 years ago
|
||
Removing top100 keyword. geocities is top100 site but the particular user's web page referenced in this bug does not meet top100 criteria.
Keywords: top100
Target Milestone: mozilla1.0 → mozilla1.0.1
Assignee | ||
Comment 12•23 years ago
|
||
The bug is triggered by auto-generated content by the Geocities.com server. It will affect all users that use <body text="..."> to change the text color.
Comment 13•23 years ago
|
||
All testcases, page now work in last night's CVS, Linux. WORKSFORME.
Component: Parser → Layout
Assignee | ||
Comment 14•23 years ago
|
||
WORKSFORME, build 2001-12-20-03 (trunk) on Windows 98 SE.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → WORKSFORME
Assignee | ||
Comment 16•21 years ago
|
||
This worked fine in Mozilla 1.0.2 but has now regressed.
Status: VERIFIED → REOPENED
Keywords: regression
Priority: P2 → --
Resolution: WORKSFORME → ---
Target Milestone: mozilla1.0.1 → ---
Assignee | ||
Comment 17•21 years ago
|
||
Regression.
Assignee: attinasi → other
Status: REOPENED → NEW
QA Contact: moied → ian
Assignee | ||
Comment 18•21 years ago
|
||
*** Bug 207505 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•21 years ago
|
Attachment #45582 -
Attachment description: Testcase → Testcase (does not work)
Assignee | ||
Comment 19•21 years ago
|
||
It works in Mozilla 1.3.1 but is broken in 1.4a
Probably something in nsHTMLStyleSheet.cpp
Assignee: other → dbaron
Component: Layout → Style System
Comment 21•21 years ago
|
||
david why when changing text size this is showed properly?
Assignee | ||
Comment 22•21 years ago
|
||
From bug 207505: Using "View - Text Zoom" makes the text appear. Also, note the "empty" style sheet inclusion in the testcase: <link rel="stylesheet" href="" type="text/css"> Remove it and the bug will not occur.
Assignee | ||
Comment 23•21 years ago
|
||
I've tracked down this regression to: rev. 3.210 content/base/src/nsStyleContext.cpp : URL+Testcase(1st) works rev. 3.211 content/base/src/nsStyleContext.cpp : URL+Testcase(1st) is broken That is the patch for bug 196603: 03/15/2003 16:20 dbaron%dbaron.org "Ensure that CalcStyleDifference populates the new context with all of the structs present on the old context to ensure that the PeekStyleData optimization is valid. Avoid copy-and-pasted code. b=196603 r+sr=roc" Bug 207505 looks like a separate problem after all (although very similar in nature) so I'm reopening that bug.
Assignee | ||
Updated•21 years ago
|
Attachment #124475 -
Attachment is obsolete: true
This has something to do with the HTMLDocumentColorRule, I think.
Blocks: 207505
Assignee | ||
Comment 25•21 years ago
|
||
Yes, HTMLDocumentColorRule is the culprit... I have a fix that removes it and instead uses the pattern that the link/vlink/alink BODY attributes uses to update the stylesheet. It fixes this bug, bug 207505 and bug 90194. I have specifically checked that bug 116161 and all its depending bugs did not regress and that it passes the block regression tests. BTW, it was the removal of the "same rulenode optimization" in CalcStyleDifference() in bug 196603 that exposed this bug.
Assignee | ||
Updated•21 years ago
|
Attachment #124868 -
Flags: review?(dbaron)
This is a change in behavior -- the old code worked no matter how the color on BODY was set, but the new code only works when it's set by the TEXT attribute. That's not necessarily a bad change -- it makes the quirk happen in slightly fewer cases. Are those cases relevant to the web? It's worth noting that making mDocumentColorRule and m{Link,Visited,Active}Rule work the same way isn't necessarily logical -- they serve rather different purposes. The mDocumentColorRule is used to implement the quirk that TABLE elements automatically acquire the BODY's background. The other three are used to implement something standard -- the LINK, VLINK, and ALINK attributes, which can't be expressed using normal attribute mapping since the style rule resulting from the attributes applies to an element different from the element that the attribute is on. (I should probably add comments explaning that in the code...) A fix that would probably fix this bug without changing the behavior would be to add a ClearDocumentColorRule that's called when you call SetDocumentColor (perhaps even unconditionally rather than based on the "text" attribute) -- which would then set a boolean on the HTMLDocumentColorRule that would cause the color to be redetermined and a new rule created *if needed* before using the rule. (This would require moving some logic from HTMLDocumentColorRule to HTMLStyleSheetImpl.) (The fact that any of these have anything to do with scripting stuff is just annoying.)
Assignee | ||
Comment 27•21 years ago
|
||
Second attempt, it is close to the suggestions in comment 26
Attachment #124868 -
Attachment is obsolete: true
Assignee | ||
Comment 28•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #124960 -
Flags: review?(dbaron)
Attachment #124868 -
Flags: review?(dbaron) → review-
Comment on attachment 124960 [details] [diff] [review] Patch rev. 2 This could be a little slow, but we'll find that out if it affects the page load tests. I'd prefer if you change the nscolor& to nscolor*, simply because I don't like references for out parameters. (Also, perhaps the big chunk in RulesMatching should really be in a separate member function (EnsureDocumentColorRule?).)
Attachment #124960 -
Flags: superreview+
Attachment #124960 -
Flags: review?(dbaron)
Attachment #124960 -
Flags: review+
Assignee | ||
Comment 30•21 years ago
|
||
> This could be a little slow...
Perhaps, hopefully it will be negligible though.
If there is a problem, we should consider Patch rev.1 which surely will not have
a performance problem. However, as you pointed out, it doesn't cover all
possible methods of color change - but is that relevant?
I don't think so. After all, this is a quirk.
It fixes the 3 open bugs we have on this issue.
Assignee | ||
Comment 31•21 years ago
|
||
Changed the reference to a pointer, diff against Patch rev. 2: < +static nsresult GetBodyColor(nsIPresContext* aPresContext, nscolor& aColor) --- > +static nsresult GetBodyColor(nsIPresContext* aPresContext, nscolor* aColor) 122c122 < + aColor = bodyFrame->GetStyleColor()->mColor; --- > + *aColor = bodyFrame->GetStyleColor()->mColor; 137c137 < + nsresult rv = GetBodyColor(ruleWalker->GetCurrentNode()->PresContext(), bodyColor); --- > + nsresult rv = GetBodyColor(ruleWalker->GetCurrentNode()->PresContext(), &bodyColor);
Attachment #124960 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #125042 -
Flags: review?(dbaron)
Comment on attachment 125042 [details] [diff] [review] Patch rev. 3 Do you have cvs access or do you need someone to check this in for you?
Attachment #125042 -
Flags: superreview+
Attachment #125042 -
Flags: review?(dbaron)
Attachment #125042 -
Flags: review+
Assignee | ||
Comment 33•21 years ago
|
||
I don't have cvs access.
Assignee | ||
Comment 34•21 years ago
|
||
Hang on to that cvs commit a bit. By pure coincidence I have found a regression with Patch rev. 3 which the block regression tests did not cover. Gimme a couple of minutes to analyse this further...
Assignee | ||
Comment 35•21 years ago
|
||
With Patch rev. 3 the text in the table is red (should be black).
Assignee | ||
Updated•21 years ago
|
Attachment #125042 -
Attachment is obsolete: true
Assignee | ||
Comment 36•21 years ago
|
||
Comment on attachment 125042 [details] [diff] [review] Patch rev. 3 Nah, I was mistaken. After recompiling everything it works just fine. Sorry for the false alarm - Patch rev.3 is good to go.
Attachment #125042 -
Attachment is obsolete: false
Assignee | ||
Updated•21 years ago
|
Attachment #125061 -
Attachment is obsolete: true
->Mats
Assignee: dbaron → mats.palmgren
Fix checked in to trunk, 2003-06-08 12:52 -0700.
Status: NEW → RESOLVED
Closed: 23 years ago → 21 years ago
Resolution: --- → FIXED
*** Bug 207505 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 40•21 years ago
|
||
*** Bug 157473 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•