Closed Bug 103594 Opened 22 years ago Closed 22 years ago

[CASCADE][review]Clean up colors prefs code in nsPresShell

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.8

People

(Reporter: pierre, Assigned: pierre)

References

Details

(Keywords: testcase)

Attachments

(3 files)

patch
2.40 KB, patch
glazou
: review+
attinasi
: superreview+
Details | Diff | Splinter Review
testcase
779 bytes, text/html
Details
image
50.51 KB, image/png
Details 
We should remove the PREFS_USE_OVERRIDE code in nsPresShell and follow what is 
described in the comments: 

//  - OVERRIDE is better for text and bg colors, but bad for link colors,
// so eventually, we should probably have an agent and an override and 
// put the link colors in the agent and the text and bg colors in the override,
Status: NEW → ASSIGNED
Depends on: 43220
Priority: -- → P3
Target Milestone: --- → mozilla0.9.6
In bug 103517, fantasai wrote: "Pref style rules should be inserted at the 
beginning of the User level; right now nsPresShell adds them as UA rules."

*** Bug 103517 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla0.9.6 → mozilla0.9.8
Marc/Daniel: please r/sr
Summary: Clean up colors prefs code in nsPresShell → [review]Clean up colors prefs code in nsPresShell
Attached patch patch — — Splinter Review 

    
    

    
  

      
      
      
      

        Attached file
          
        testcase
        — 
      

    


  
Pierre - does your patch handle this testcase correctly?

    
  
Keywords: testcase
Summary: [review]Clean up colors prefs code in nsPresShell → [CASCADE][review]Clean up colors prefs code in nsPresShell

    
    

    
  
fantasai: the :root{} rule doesn't do anything even if it changed to !important.  
The testcase works fine though when the selector is changed to 'body'.


    
    

    
  
I suspected that would happen, since you're appending the stylesheet instead of 
prepending it. Try replacing
  +        mStyleSet->AppendUserStyleSheet(mPrefStyleSheet);
with
  +        mStyleSet->InsertUserStyleSheetBefore(mPrefStyleSheet,
  +                                        mStyleSet->GetUserStyleSheetAt(0));
I think the testcase should work then.

The reason it works with body is because the body is a child of :root--no matter 
how specific the rule is on :root, it will never override a color on the body.
(If you use body instead of :root and make the <head> and <title> display block, 
you'll see that the prefs color is set on the :root, because the <head> still 
has the prefs color.)

    
  

        Attachment #61968 -
        Flags: superreview+

    
    

    
  
Comment on attachment 61968 [details] [diff] [review]
patch

sr=attinasi

    
    

    
  
fixed

checked in with mStyleSet->InsertUserStyleSheetBefore(mPrefStyleSheet, nsnull)

Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED

    
    

    
  
Did this cause a 10ms jump in page load time?

    
    

    
  
on linux, i tested the testcase attachment 62200 [details] .....
...this is what i saw ...

TEST 1 : 
The text is blue on an aqua background. The entire body has the aqua background.

TEST 2:
The text is red on an aqua background, but, the aqua background is not painted
ont the entire body ....... only text background gets the aqua background ...
remaining page has a white background .

see what i am mean in the attacheded image.




    
    

    
  

      
      
      
      

        Attached image
          
        image 
        — 
      

    


  

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