Closed
Bug 163785
Opened 23 years ago
Closed 23 years ago
minimum font-size doesn't work when no font props specified
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla1.2alpha
People
(Reporter: rbs, Assigned: rbs)
References
Details
Attachments
(3 files, 1 obsolete file)
|
217 bytes,
text/html
|
Details | |
|
230 bytes,
text/html
|
Details | |
|
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
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.
|
Assignee | |
Comment 10•23 years ago
|
||
Attachment #96264 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 11•23 years ago
|
||
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
|
|
Assignee | |
Comment 12•23 years ago
|
||
Trying other reviewers (cbiesinger who fixed bug 110342 and bz for sr).
(I got an auto-reply upon attempting dbaron's private e-mail).
|
||
Comment 13•23 years ago
|
||
+ 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.
|
|
Assignee | |
Comment 14•23 years ago
|
||
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).
|
||
Comment 15•23 years ago
|
||
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 :) )
|
|
||
Updated•23 years ago
|
Attachment #96375 -
Flags: superreview+
|
|
||
Comment 16•23 years ago
|
||
Comment on attachment 96375 [details] [diff] [review]
patch - take 2
sr=bzbarsky
|
|
Assignee | |
Comment 17•23 years ago
|
||
Trying heikki for sr, assuming bz may trade his sr for an r.
|
|
Assignee | |
Comment 18•23 years ago
|
||
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+
|
|
Assignee | |
Comment 20•23 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 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.)
|
|
Assignee | |
Comment 23•23 years ago
|
||
Updated.
|
|
Assignee | |
Comment 24•23 years ago
|
||
It didn't affect the Tp though.
You need to log in
before you can comment on or make changes to this bug.
Description
•