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.

IE5 does this correctly.

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.

You're correct... cc'ing erik on this.

You're right, it also happens from the view menu's Enlarge/Reduce items. Erik, it looks like this is yours.

Reassigning to erik. Note that this probably should have Component: GFX but that doesn't seem to be an option.

My guess is that the layout engine is using the specified font size instead of the actual font size.

As per meeting with ChrisD today, taking QA.

nsbeta3- per bug meeting w/ ekrock. Not too many people will see this.

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

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

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

clear out M18

mark as future

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

erik resign. reassign all his bug to ftang for now.

mark all future new as assigned after move from erik to ftang

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

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?!)

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

marc- can you take this one?

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

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.

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.

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

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

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.

Removing PDT grafitti, and nsbeta3. Adding nsbeta1 for triaging nomination.

Not only 'em' is affected, '%' is also. Set for example 'line-height: 120%' and zoom.

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.

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.

Taking.

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.

dbaron: why is your fix correct rather than the fix suggested in comment 23?

That would only fix a small subset of the problems that this patch fixes.

sr=hyatt

Do we still need to change em = nsFont::size to em = GetEmHeight() throughout the tree though? If just for consistency's sake?

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

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.

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?

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.

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

Fix checked in 2002-02-16 08:24 PDT.

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

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

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?

Disowning.

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.

.

Marking fixed again. Or is it broken on platforms where 'font-size-adjust' works? What platform are you testing on? See also bug 125963.

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

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

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

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.

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.

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.

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

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

hoju: that's probably bug 53995.

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?

please prepare a special test case and file a new bug -- thanks VERIFIED FIXED on most test cases

Ok Ian, did that. See bug 140599. Thanks.