458231 - Frame overflow plays havoc with size of <label> or <description>

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[[e-mail]]

When a <label> or <description> contains HTML content which paints outside of its boundaries (which I believe you call overflow, not to be confused with the CSS overflow property), then they miscompute their dimensions.

A simple test case that applies a 40px outline makes the label's height 40px more than it should be. The widths I see make no sense to me at all, although strangely when opening the test case as chrome only the window's height is affected; the window's width does not change (but it still paints incorrectly.)

When I try to apply a 40px text shadow, the height is now 80px more than the expected value; the widths are at least consistently strange...

Comment 1

[[e-mail]]

Attachment: Test case

Comment 2

[[e-mail]]

OK, so I debugged this a bit, and it turns out that <label> works by asking its area frame for its overflow area, and sizing itself based on that, because for some unknown reason its area frame's desired size is only the height of the first line of text (although I stepped into the reflow for the <div> which did know the height, which is not unreasonable since the border is drawn correctly.)

The code in question is nsFrame::BoxReflow just after the call to Reflow.

Comment 3

[[e-mail]]

Attachment: Proposed patch

By allowing the label unconstrained height during reflow, we don't need to worry about fixing up the height after reflow, so that code just dies.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Much as I'd like to do this, this is a major change to the behaviour of XUL layout that I'm afraid to make without considerable thought.

Comment 5

[Dão Gottwald [:dao]]

see also bug 69710 comment 17

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #4)
> Much as I'd like to do this, this is a major change to the behaviour of XUL
> layout that I'm afraid to make without considerable thought.

That says blocking- to me ...

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 341754 [details] [diff] [review]
Proposed patch

Let's do this for 1.9.2.

Comment 8

[[e-mail]]

Pushed changeset 176f8cd08078 to mozilla-central.

Comment 9

[Phil Ringnalda (:philor)]

Backed out in changeset 859496f90e66 - you failed http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/386470-1a.html?force=1 twice on all three platforms, putting some space above the lime div (e.g. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1235865020.1235870023.12914.gz#err0)

Comment 10

[[e-mail]]

(In reply to comment #9)
> Backed out in changeset 859496f90e66 - you failed
> twice on all three platforms, putting some space above the lime div
Weird. Especially as I can't seem to create a reduced test case :-(

Comment 11

[[e-mail]]

Apparently this patch changes the computed baseline; the amount of offset depends on the font height.

Or is this something to do with overflow, i.e. I need to check for something that wants to overflow and therefore should have their size constrained?

Comment 12

[[e-mail]]

On second thoughts it doesn't seem to be overflow; removing the overflow style doesn't seem to help.

Comment 13

[[e-mail]]

Attachment: Fixes failing test

Test 386470-1a fails without this extra change.

Comment 14

[[e-mail]]

Pushed changeset 6eb0ddec35a0 to mozilla-central.

Comment 15

[Ben Bucksch (:BenB)]

From what I read and see in tests, this was not applied to 1.9.1, so fixing Target Milestone field.

This causes bug 526918 in Thunderbird 3.0 based on 1.9.1. I'll attaching the backported patch.

Comment 16

[Ben Bucksch (:BenB)]

Attachment: Backported to 1.9.1

Comment 17

[[e-mail]]

Should this land on 1.9.1 you would also want to back out the workarounds for the XP Classic native theme disabled checkboxes or radio buttons.