Closed Bug 41847 Opened 24 years ago Closed 23 years ago

Text zoom causes overlap since em remains same

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.9

People

(Reporter: ian, Unassigned)

References

(
URL
)

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.
IE5 does this correctly.
Keywords: 4xp, css1, testcase
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
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.
Summary: em's dont update when changing font size with mousewheel → em's dont update when changing font size in UI
Reassigning to erik.  Note that this probably should have Component: GFX but 
that doesn't seem to be an option.
Assignee: bryner → erik
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
Keywords: correctness, nsbeta3
As per meeting with ChrisD today, taking QA.
QA Contact: petersen → py8ieh=bugzilla
nsbeta3- per bug meeting w/ ekrock. Not too many people will see this.
Whiteboard: nsbeta3-
*** Bug 61175 has been marked as a duplicate of this bug. ***
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)
*** 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
Target Milestone: M18 → ---
mark as future
Target Milestone: --- → Future
*** Bug 77891 has been marked as a duplicate of this bug. ***
Whiteboard: nsbeta3- → [Hixie-P3] nsbeta3-
erik resign. reassign all his bug to ftang for now.
Assignee: erik → ftang
Status: ASSIGNED → NEW
mark all future new as assigned after move from erik to ftang
Status: NEW → ASSIGNED
*** Bug 89490 has been marked as a duplicate of this bug. ***
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)
Blocks: 74186
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?
Assignee: ftang → attinasi
Status: ASSIGNED → NEW
Target Milestone: Future → ---
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
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. ***
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
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
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 )
Summary: Text zooming causes overlap since line-height ('em') remains same ( font zoom em ) → Text zoom causes overlap since em remains same
Removing PDT grafitti, and nsbeta3. Adding nsbeta1 for triaging nomination.
Keywords: nsbeta3 → nsbeta1
Whiteboard: [Hixie-P3] nsbeta3- → [Hixie-P3]
Not only 'em' is affected, '%' is also. Set for example 'line-height: 120%' and
zoom.
Attached patch fix (without 'line-height' bit) (obsolete) — — Splinter Review 
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.
Attached patch fix (obsolete) — — Splinter Review 
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.
Attachment #67358 - Attachment description: fix → fix (without 'line-height' bit)
Attachment #67358 - Attachment is obsolete: true
Taking.
Assignee: attinasi → dbaron
Status: REOPENED → NEW
Status: NEW → ASSIGNED
OS: Windows 2000 → All
Priority: P3 → P2
Hardware: PC → All
Target Milestone: mozilla1.0 → mozilla0.9.9
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)
Attachment #67359 - Flags: review+
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Assignee: dbaron → nobody
Status: REOPENED → NEW
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.
Status: NEW → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
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.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
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.
Attachment #67359 - Attachment is obsolete: true
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
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
Status: RESOLVED → VERIFIED
Ok Ian, did that. See bug 140599. Thanks.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: