Closed
Bug 41847
Opened 25 years ago
Closed 23 years ago
Text zoom causes overlap since em remains same
Categories
(Core :: Layout, defect, P2)
Core
Layout
Tracking
()
VERIFIED
FIXED
mozilla0.9.9
People
(Reporter: ian, Unassigned)
References
()
Details
(Keywords: css1, testcase, Whiteboard: [Hixie-P3])
Attachments
(2 obsolete files)
Changing the font size using the mouse wheel (control+mousewheel by default) does not cause the "em" unit as used for lengths to be updated. For example, if you increase the font size a lot using the mousewheel, such that the font-size 'medium' is about 100px, then the following code: <div style=" height: 1em; width: 1em; font-size: medium; border: solid; "> X </div> ...will not generate a 100px by 100px box, but a small, 12px by 12px box with a very large 100px X inside it. This is very, very wrong. See this test case: http://www.bath.ac.uk/%7Epy8ieh/m/font-size-changes-and-ems.html ...for more examples.
Comment 2•25 years ago
|
||
if this is only an issue with mousewheel resizing and not reduce/enlarge text size (from the view menu), then it's bryner's bug. But he just used erik's resizing code I think, so I don't know why it'd be specific to mousewheel. sending to bryner to examine this, anyways.
Assignee: clayton → bryner
Reporter | ||
Comment 4•25 years ago
|
||
You're right, it also happens from the view menu's Enlarge/Reduce items. Erik, it looks like this is yours.
Summary: em's dont update when changing font size with mousewheel → em's dont update when changing font size in UI
Comment 5•25 years ago
|
||
Reassigning to erik. Note that this probably should have Component: GFX but that doesn't seem to be an option.
Assignee: bryner → erik
Comment 6•25 years ago
|
||
My guess is that the layout engine is using the specified font size instead of the actual font size.
Status: NEW → ASSIGNED
Target Milestone: --- → M18
Reporter | ||
Updated•25 years ago
|
Keywords: correctness,
nsbeta3
Reporter | ||
Comment 7•25 years ago
|
||
As per meeting with ChrisD today, taking QA.
QA Contact: petersen → py8ieh=bugzilla
Comment 8•24 years ago
|
||
nsbeta3- per bug meeting w/ ekrock. Not too many people will see this.
Whiteboard: nsbeta3-
Updated•24 years ago
|
Summary: em's dont update when changing font size in UI → em's dont update when changing font size in UI (View | Text Size or mousewheel)
Updated•24 years ago
|
Keywords: mozilla0.9
![]() |
||
Updated•24 years ago
|
Keywords: mozilla0.9.1
Reporter | ||
Updated•24 years ago
|
Whiteboard: nsbeta3- → [Hixie-P3] nsbeta3-
Comment 15•24 years ago
|
||
erik resign. reassign all his bug to ftang for now.
Assignee: erik → ftang
Status: ASSIGNED → NEW
Comment 16•24 years ago
|
||
mark all future new as assigned after move from erik to ftang
Status: NEW → ASSIGNED
Reporter | ||
Updated•23 years ago
|
Summary: em's dont update when changing font size in UI (View | Text Size or mousewheel) → em's dont update when changing font size in UI (View | Text Size or mousewheel) (font-size should be actual font-size not computed font-size)
Reporter | ||
Comment 18•23 years ago
|
||
Note the following comments in bug 41847, which should explain how to fix this bug quite easily: > After changing the code to also get the CSS metrics from the adjusted size, > all pass, except 'em'. That's because nsFont::size is used all over the place > as an approximation of 'em'! People (me included of course :-) should really > be using the GetEmHeight() API. (BTW, that's where bug 41847 comes from... > where is the effect of the zoom if nsFont::size is being used?!)
Comment 20•23 years ago
|
||
marc- can you take this one?
Assignee: ftang → attinasi
Status: ASSIGNED → NEW
Target Milestone: Future → ---
Comment 21•23 years ago
|
||
Accepting. I'll look into it when priority allows. BTW: we could just do a reframe in the meantime when the default font size is changed - overkill? sure, but quick and easy - I won;t be getting to this for a while I tihnk (putting up egg-shields now).
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
Comment 22•23 years ago
|
||
I don't think a reframe is the issue. I think the problem is that this is implemented at too low a level -- so that the style system doesn't even know it's happening, and only the font (and text??) code does.
Comment 23•23 years ago
|
||
The reason why the Style System doesn't know that it is happening is because it computes 'em' as 'em = nsFont::size' (c.f. the code in nsRuleNode::SetCoord() for example). Other examples like that are in the tree. The problem is not much different from 'ex'. But this latter one works OK because 'ex' is rigthly computed as 'ex = GetXHeight()'. So, the way to go about fixing this is to switch to 'em = GetEmHeight()' across the tree.
Updated•23 years ago
|
Summary: em's dont update when changing font size in UI (View | Text Size or mousewheel) (font-size should be actual font-size not computed font-size) → Text zooming causes overlap since line-height ('em') remains same
Comment 25•23 years ago
|
||
I reopened bug 95267 and I'll close this one as dup. The other bug contains more info. *** This bug has been marked as a duplicate of 95267 ***
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → DUPLICATE
Reporter | ||
Comment 26•23 years ago
|
||
Uh. The two bugs are not dupes. Reopening. This bug covers the fact that we do not update 'em' when we change font zoom. Bug 95267 covers the fact that we do not update absolute line-heights when we change the font zoom. They are totally separate bugs with totally separate fixes.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Text zooming causes overlap since line-height ('em') remains same → Text zooming causes overlap since line-height ('em') remains same ( font zoom em )
Reporter | ||
Updated•23 years ago
|
Summary: Text zooming causes overlap since line-height ('em') remains same ( font zoom em ) → Text zoom causes overlap since em remains same
Comment 27•23 years ago
|
||
Removing PDT grafitti, and nsbeta3. Adding nsbeta1 for triaging nomination.
Comment 28•23 years ago
|
||
Not only 'em' is affected, '%' is also. Set for example 'line-height: 120%' and zoom.
Comment 29•23 years ago
|
||
This fixes the bug by moving the handling of zoom from the font code into the style system. It also special-cases pixel and absolute-length 'line-height' values and scales them as well.
Comment 30•23 years ago
|
||
This fixes the bug by moving the handling of zoom from the font code into the style system. It also special-cases pixel and absolute-length 'line-height' values and scales them as well.
Updated•23 years ago
|
Attachment #67358 -
Attachment description: fix → fix (without 'line-height' bit)
Attachment #67358 -
Attachment is obsolete: true
Updated•23 years ago
|
Status: NEW → ASSIGNED
OS: Windows 2000 → All
Priority: P3 → P2
Hardware: PC → All
Target Milestone: mozilla1.0 → mozilla0.9.9
Comment 32•23 years ago
|
||
There were some peculiarities regarding the order with which to apply the zoom and font-size-adjust. You might want to double check with Hixie that the patch doesn't regress that.
Reporter | ||
Comment 33•23 years ago
|
||
dbaron: why is your fix correct rather than the fix suggested in comment 23?
Reporter | ||
Comment 36•23 years ago
|
||
Do we still need to change em = nsFont::size to em = GetEmHeight() throughout the tree though? If just for consistency's sake?
Comment 37•23 years ago
|
||
It sounds like this is a fix for bug 95267 as well (line-height not getting scaled). This is great work DBaron! I'll look over the patch as well.
Comment 38•23 years ago
|
||
Comment on attachment 67359 [details] [diff] [review] fix The change to nsStyleStruct.cpp seems unnecessary, but looks ra tional anyway. Also, why don't we need to flush the font cache now in SetZoom/SetTextZoom? r=attinasi (assuming the FontCache issue is understood to be OK)
Attachment #67359 -
Flags: review+
Comment 39•23 years ago
|
||
We don't need to flush the font cache because the implementation of text zoom no longer changes the mapping from nsFont to native font when the zoom is changed. The implementation now causes the nsFont requested to be different. I still need to look into the font-size-adjust issue, though.
Comment 40•23 years ago
|
||
rbs: Does font-size-adjust work at all on Unix? I can't seem to get a testcase to work (in a build without my changes). What is the issue that you were worried about?
Comment 41•23 years ago
|
||
I don't think there should be problems with font-size-adjust, though, since I'm zooming both the nsStyleFont::mFont::mSize and the nsStyleFont::mSize.
Comment 42•23 years ago
|
||
font-size-adjust isn't implemented on Unix, only on Windows. The issue I was referring to is the following: http://bugzilla.mozilla.org/show_bug.cgi?id=74186#c66 (There are some associated testcases there.)
Comment 43•23 years ago
|
||
Fix checked in 2002-02-16 08:24 PDT.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 44•23 years ago
|
||
Great! Thanks!! But,BuildID:2002021706/Linux has problem. When ex resized lower 100%,width and height is 0px. testcase: http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=360
Reporter | ||
Comment 45•23 years ago
|
||
Reopening. The original bug I reported isn't fixed. See: http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/font-size-changes-and-ems.html The problem is described in: http://bugzilla.mozilla.org/show_bug.cgi?id=41847#c0 I still think the correct fix is described in: http://bugzilla.mozilla.org/show_bug.cgi?id=41847#c23
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 46•23 years ago
|
||
I don't know what build you're using, Ian, but that testcase works fine for me. Do we really want to base 'em' units on the actual font size, based on calls into the font metrics code?
Comment 48•23 years ago
|
||
http://www.w3.org/TR/REC-CSS2/syndata.html#em-width says it should be based on the computed font size, not the actual font size.
Comment 50•23 years ago
|
||
Marking fixed again. Or is it broken on platforms where 'font-size-adjust' works? What platform are you testing on? See also bug 125963.
Status: NEW → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 51•23 years ago
|
||
I happen to have a latest Win32 build. Indeed, hixie's testcase in the URL appears fixed. However, in bug 74186 comment #71, I wrote that all the following tests passed except 'em'. Now 'em' is still failing and '%' has regressed. I have annoted them below: [Passed] http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/003.xml normal [Passed] http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/004.xml ratio [Failed] http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/005.xml % [Failed] http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/006.xml em [Passed] http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/007.xml ex [Passed] http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/008.xml px Didn't test the minimum size test (still invalid -- bug 74186 comment #75, but it used to pass too, on a locally corrected copy) http://www.hixie.ch/tests/adhoc/css/fonts/size/minimum/001.xml Protocol wise, I am not sure what the rules are here, but it would seem proper to me to backout before disowning so that it is back to square one, or stand by your fix (i.e., follow-up with some of the effects that may arise).
Reporter | ||
Comment 52•23 years ago
|
||
I'm building again to see if there was some problem when I pulled your changes yesterday. The forms crasher was interfeering quite badly with my ability to test stuff. I'm testing on Linux, using a build with almost all flags enabled. In my opinion, 'em' should be based on actual values, yes, see: http://bugzilla.mozilla.org/show_bug.cgi?id=74186#c66 ...where I describe the life of font-size as: stylesheet cascade | \|/ specified value <-------------------------------------+ | | \|/ | computed value ---------------------------------> inheritance | \|/ minimum font-size calculation | \|/ font-size-adjust | \|/ font zoom | \|/ actual value -------> calculation of 'em', 'ex', '%', ratios / \ | |/_ _\| \|/ font rasterising inline box ---------> inline box model line height
Reporter | ||
Comment 53•23 years ago
|
||
Reopening again. Changing the font zoom does not affect borders, heights, widths, margins, etc, that are set using 'em's. This is presumably because, like you say, we are using the computed value and not the actual value for the 'em' units. (Doing this leads to overlap if line-height is set in 'em's.) The test case: http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/font-size-changes-and-ems.html ...describes the expected behaviour, and we are not doing what the test case describes. However, it appears that the CSS2 spec (still) says that 'em' should be based on the computed value and not the actual value of font-size. I'll bring this up in the WG. (This was discussed back in 1999 in www-style.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 54•23 years ago
|
||
Like I said, that testcase works for me. Really disowning this time, since I don't want it on my list anymore since it's fixed.
Assignee: dbaron → nobody
Status: REOPENED → NEW
Reporter | ||
Comment 55•23 years ago
|
||
dbaron: it works as in the behaviour described in the testcase is what you see (borders grow when you change the zoom) or as in we do what the spec says to the letter (em is based on the unscaled computed value, borders, margins etc don't scale)? Both of those could be described as "working", I'm not sure to which you are referring. If it's the first, then there is an odd problem here (probably on my side), since my CVS tip build has the second behaviour.
Comment 56•23 years ago
|
||
If someone is interested in trying out: http://bugzilla.mozilla.org/show_bug.cgi?id=41847#c23 then a way to proceed could be to temporarily turn nsFont.cpp to a class, and make the |size| field private, then do the substitution as the compiler catch users.
Updated•23 years ago
|
Attachment #67359 -
Attachment is obsolete: true
Reporter | ||
Comment 57•23 years ago
|
||
Bafflingly, my build now does indeed work on the test. I have no idea why it didn't before. A problem with our build system not triggering enough rebuilding with dependencies and so on? Marking FIXED again. Sorry for doubting you David. :-) (Incidentally the WG decided today that while the 'em' unit should be defined in terms of the computed value, it is ok for font zoom to affect 'em' units. I'm not entirely sure how that works, but I suggest we don't worry too much about it.)
Status: NEW → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 58•23 years ago
|
||
There is some weirdness with zooming. Check out the following page: http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/font-size-changes-and-ems.html Try zooming out until it is very, very small. Now keep zooming out. The text seems to lose its styling. Most of the text ends up looking going back to the default font size. Zooming back in (at a certain point) brings back the expected behavior. Not sure if this is something to worry about? It just looks a bit quirky and messy rather than impeding usability. Should this be reported as a bug, albeit with lower priority? Jake
Comment 60•23 years ago
|
||
Sorry guys, I can't find this bug so much fixed. Yes, the test case from comment #53 works for me as well. But here's another case: http://xren.sourceforge.net/ The height of the fixed DIV at the bottom and the width of the one on the right are given in 'em's. Now when you increase the text zoom step by step, the DIVs sizes do not change until you reach the 180% zoom step (or 40% if you decrease the size) with 1.0RC1 (Build 2002041711). The CSS used is http://xren.sourceforge.net/normal-layout.css, the DIVs in question are div.primnav and div.secnav. Interestingly, the right margin of the main content div (div.content, which has margin-right:13.5em set so that the right menu won't overlap the text) *does* scale according to the the text zoom. Do you see the point or should I prepare a special test case with all the uninteresting stuff removed?
Reporter | ||
Comment 61•23 years ago
|
||
please prepare a special test case and file a new bug -- thanks VERIFIED FIXED on most test cases
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•