Closed Bug 94688 Opened 23 years ago Closed 21 years ago

black text instead of white

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: zheka, Assigned: MatsPalmgren_bugz)

References

(
URL
)

Details

(Keywords: regression, testcase)

Attachments

(5 files, 4 obsolete files)

The page without the first line.
3.26 KB, text/html
Details
Testcase (does not work)
414 bytes, text/html
Details
Testcase #2
347 bytes, text/html
Details
Testcase #4
3.04 KB, text/html
Details
Patch rev. 3
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
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
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.

    
    

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

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase #2
        — 
      

    


  

    
    

    
  
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.

    
  
Target Milestone: --- → mozilla0.9.4

    
  
Priority: -- → P2

    
    

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

    
  
Target Milestone: mozilla0.9.4 → mozilla0.9.5

    
  
QA Contact: bsharma → moied

    
  
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

    
    

    
  
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

    
    

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

    
    

    
  
Verified WFM with build ID 20020131 on win2k
Status: RESOLVED → VERIFIED

    
    

    
  
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 → ---

    
    

    
  
Regression.
Assignee: attinasi → other
Status: REOPENED → NEW
QA Contact: moied → ian

    
    

    
  
*** Bug 207505 has been marked as a duplicate of this bug. ***

    
  

        Attachment #45582 -
        Attachment description: Testcase → Testcase (does not work)

    
    

    
  
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

    
    

    
  
david why when changing text size this is showed properly?

    
    

    
  

      
      
      
      

        Attached file
          
        Testacse #3 (from bug 207505) (obsolete)
        — 
      

    


  
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.

    
    

    
  
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.

    
  

        Attachment #124475 -
        Attachment is obsolete: true

    
    

    
  
This has something to do with the HTMLDocumentColorRule, I think.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rev. 1 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

    
  

        Attachment #124868 -
        Flags: review?(dbaron)

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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rev. 2 (obsolete)
        — 
          — Splinter Review
      

    


  
Second attempt, it is close to the suggestions in comment 26

        Attachment #124868 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase  #4
        — 
      

    


  

    
  

        Attachment #124960 -
        Flags: review?(dbaron)

    
    

    
  
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+

    
    

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


    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rev. 3
        — 
          — Splinter Review
      

    


  
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

    
  

        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+

    
    

    
  
I don't have cvs access.

    
    

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

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase #5 (obsolete)
        — 
      

    


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

    
  

        Attachment #125042 -
        Attachment is obsolete: true

    
    

    
  
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

    
  

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

    
    

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: