black text instead of white

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
17 years ago
15 years ago

People

(Reporter: zheka844, Assigned: mats)

Tracking

({regression, testcase})

Trunk
x86
All
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

17 years ago
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
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

17 years ago
Created attachment 45434 [details]
The page without the first line.

Comment 3

17 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

17 years ago
Created attachment 45582 [details]
Testcase (does not work)
(Assignee)

Comment 5

17 years ago
Confirming, build 2001-08-09-04 on Windows 98 SE.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase, top100
OS: Linux → All

Comment 6

17 years ago
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

17 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

17 years ago
Created attachment 46352 [details]
Testcase #2
(Assignee)

Comment 9

17 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.

Updated

17 years ago
Target Milestone: --- → mozilla0.9.4

Updated

17 years ago
Priority: -- → P2

Comment 10

17 years ago
This is definitely a layout problem. Reassigning to Marc [ sorry! :-( ].
Assignee: harishd → attinasi

Updated

17 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Updated

17 years ago
QA Contact: bsharma → moied

Updated

17 years ago
Target Milestone: mozilla0.9.5 → mozilla1.0
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

17 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.
All testcases, page now work in last night's CVS, Linux.  WORKSFORME.
Component: Parser → Layout
(Assignee)

Comment 14

17 years ago
WORKSFORME, build 2001-12-20-03 (trunk) on Windows 98 SE.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → WORKSFORME

Comment 15

17 years ago
Verified WFM with build ID 20020131 on win2k
Status: RESOLVED → VERIFIED
(Assignee)

Comment 16

15 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

15 years ago
Regression.
Assignee: attinasi → other
Status: REOPENED → NEW
QA Contact: moied → ian
(Assignee)

Comment 18

15 years ago
*** Bug 207505 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

15 years ago
Attachment #45582 - Attachment description: Testcase → Testcase (does not work)
(Assignee)

Comment 19

15 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

15 years ago
david why when changing text size this is showed properly?
(Assignee)

Comment 22

15 years ago
Created attachment 124475 [details]
Testacse #3 (from bug 207505)

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

15 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

15 years ago
Attachment #124475 - Attachment is obsolete: true
This has something to do with the HTMLDocumentColorRule, I think.
(Assignee)

Comment 25

15 years ago
Created attachment 124868 [details] [diff] [review]
Patch rev. 1

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

15 years ago
Attachment #124868 - Flags: review?(dbaron)
(Assignee)

Updated

15 years ago
Blocks: 90194
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

15 years ago
Created attachment 124960 [details] [diff] [review]
Patch rev. 2

Second attempt, it is close to the suggestions in comment 26
Attachment #124868 - Attachment is obsolete: true
(Assignee)

Comment 28

15 years ago
Created attachment 124961 [details]
Testcase  #4
(Assignee)

Updated

15 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

15 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

15 years ago
Created attachment 125042 [details] [diff] [review]
Patch rev. 3

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

15 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

15 years ago
I don't have cvs access.
(Assignee)

Comment 34

15 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

15 years ago
Created attachment 125061 [details]
Testcase #5

With Patch rev. 3 the text in the table is red (should be black).
(Assignee)

Updated

15 years ago
Attachment #125042 - Attachment is obsolete: true
(Assignee)

Comment 36

15 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

15 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
Last Resolved: 17 years ago15 years ago
Resolution: --- → FIXED
*** Bug 207505 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 40

15 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.