Closed Bug 163785 Opened 22 years ago Closed 22 years ago

minimum font-size doesn't work when no font props specified

Categories

(Core :: CSS Parsing and Computation, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.2alpha

People

(Reporter: rbs, Assigned: rbs)

References

Details

Attachments

(3 files, 1 obsolete file)

testcase
217 bytes, text/html
Details
testcase - includes extra line that doesn't work too
230 bytes, text/html
Details
patch - take 2
2.76 KB, patch
dbaron
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
This is an enigmatic behavior. To reproduce:
- visit http://www.mozilla.org
- click Edit->Preferences->Appearance->Fonts and set the minimum font-size to 24px

EXPECTED RESULTS
  all elements should display at a font size not less than the minimum font size 
ACTUAL RESULTS
  any text that is a direct child of <p> doesn't honor the minimum font-size
Attached file testcase — 

    
    

    
  
Noted that the bug goes away in the testcase if I do:
<body style="font-style:italic">

(or font-family, etc, so that the style struct is init'ed by the Style System.
Seems that the bug comes from too much optimization).

    
    

    
  
Am I correct that it's only possible for this to be an issue if the minimum font
size is larger than the default font size?

    
    

    
  
I think this could be fixed either by making the nsStyleFont constructor that
takes an nsIPresContext* argument fixup the size or making
nsRuleNode::SetDefaultOnRoot do it.
Status: NEW → ASSIGNED
Priority: -- → P4
Summary: minimum font-size is blocked by <p> → minimum font-size doesn't work when no font props specified
Target Milestone: --- → Future

    
    

    
  
Taking & targeting m1.2a, will have a look.
Assignee: dbaron → rbs
Status: ASSIGNED → NEW
OS: Windows 2000 → All
Priority: P4 → --
Hardware: PC → All
Target Milestone: Future → mozilla1.2alpha

    
    

    
  


  

    
    

    
  
Comment on attachment 96264 [details] [diff] [review]
patch to do the fixup in SetDefaultOnRoot()

This doesn't enforce the fact that the pref applies only to content, and not to
chrome.

I'd also slightly prefer if the code were in the nsStyleFont constructor that
takes an nsPresContext, since that would leave SetDefaultOnRoot more
consistent...

    
    

    
  
>This doesn't enforce the fact that the pref applies only to content, and not to
>chrome.

Indeed the patch is going to break if a chrome doesn't set its fonts... The 
patch didn't have any side-effects because the chrome set its fonts, but it is 
fragile to rely on that... Will attach another patch to fix the problem, using a 
little heper function to test the chrome.

>I'd also slightly prefer if the code were in the nsStyleFont constructor that
>takes an nsPresContext, since that would leave SetDefaultOnRoot more
>consistent...

The other side of the coin is that it will move the inconsistency to the ctor. 
Whereas we can keep the genuflections for the zoom & min-size close, and in the 
same file.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch - take 2
        — 
          — Splinter Review
      

    


  

        Attachment #96264 -
        Attachment is obsolete: true

    
    

    
  
hixie/dbaron, r/sr on this little gem?
Status: NEW → ASSIGNED
Summary: minimum font-size doesn't work when no font props specified → [FIX] minimum font-size doesn't work when no font props specified

    
    

    
  
Trying other reviewers (cbiesinger who fixed bug 110342 and bz for sr).
(I got an auto-reply upon attempting dbaron's private e-mail).

    
    

    
  
+      if (!IsChrome(mPresContext)) {
+        nscoord minimumFontSize = 0;
+        mPresContext->GetCachedIntPref(kPresContext_MinimumFontSize,
minimumFontSize);
+        fontData->mFont.size = PR_MAX(fontData->mSize, minimumFontSize);
+      }
+      else {
+        fontData->mFont.size = fontData->mSize;
+      }

Couldn't you just have the first branch of that code and onle get the pref if
!IsChrome(...)?  Then for the chrome case the PR_MAX would always return
fontData->mSize and the code would, I think, be a little more maintainable...

Other than that, looks pretty good.

    
    

    
  
There is a comment in SetFont() which explains more what is happening regarding 
this dichotomy between the "actual size" and the "computed size". It seems to me 
that the else is actually making that clear (taking into account the other bit 
in SetFont).

    
    

    
  
rbs, sorry, I am not qualified to review that code... the bug you cited was
frontend-only; and I know not really anything about the backend (though I intent
to learn that sometime :) )

        Attachment #96375 -
        Flags: superreview+

    
    

    
  
Comment on attachment 96375 [details] [diff] [review]
patch - take 2

sr=bzbarsky

    
    

    
  
Trying heikki for sr, assuming bz may trade his sr for an r.

    
    

    
  
Trying XSLT folks. Is this bug rocket science?!?

    
    

    
  
Comment on attachment 96375 [details] [diff] [review]
patch - take 2

r=dbaron

(I may at some point decide to move this to the nsStyleFont constructor if I
find a reason to exploit the fact that the switch statement in SetDefaultOnRoot
consists of 18 identical-looking cases.)

        Attachment #96375 -
        Flags: review+

    
    

    
  
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Summary: [FIX] minimum font-size doesn't work when no font props specified → minimum font-size doesn't work when no font props specified

    
    

    
  
Could this have caused the page load time regression (about 12ms) on tinderbox?

    
    

    
  
(If it did, I'd hope it could be fixed by adding a minimumFontSize > 0 check
outside the IsChrome() call.)

    
    

    
  
Updated.

    
    

    
  
It didn't affect the Tp though.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: