825999 - REGRESSION: overflow: hidden; on <button>s hides text but doesn't change the button's overflow area or that of its parents

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Art McBain]

Attachment: Test case for Firefox button overflow regression

Bug ID for previous issue with similar effect (resolved in 2010): 541382 (I don't know how to / can't re-open issues).

Regression of overflow handling on <button>s with a fixed width and overflow: hidden. Such buttons now behave as if their overflow was not hidden except that the overflow is not painted.

I noticed this on an existing site I wrote where such a button was nested in a sidebar. The site was designed to fill the viewport if the viewport was less than a certain width. Thus it never had a horizontal scrollbar for the page until this regression.

This regression causes space to appear to the right side of the sidebar with nothing in it and thus also cause a horizontal scrollbar for the page.

Attached is a test-case, using outline, that shows the effective (layout-affecting) width of a fixed width button with overflow compared to a normal div with the same styles. If you view this test-case in Firefox the effective width of the button is much larger than the div, which was not the case in prior versions of Firefox. If you view this in Chrome or Opera, no extra space is reserved for the overflowing text as expected. In all browsers, the overflowing text is not actually painted (as expected).

Comment 1

[Art McBain]

Hopefully this'll make it a link this time: bug 541382

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  Looks like this worked right in Firefox 11 but is already broken in Firefox 12.

Looks like the overflow area for the button includes the text (hence the effects on ancestor scrollbars and outlines), which isn't right.

Comment 3

[Matthias Versen [:Matti]]

Last good nightly: 2012-01-19
First bad nightly: 2012-01-20

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=58e933465c36&tochange=5c2bc94d359c

Bug 524925 ?

Comment 4

[Alice0775 White]

Last Good: 73eaf1199ff0
First Bad: a915d5820eb8

Triggered by:
a915d5820eb8	Mats Palmgren — Bug 524925 - Consolidate overflow clipping checks to nsFrame::ApplyOverflowClipping(); and fix some code style nits. part=5/6 r=roc

Comment 5

[Boris Zbarsky [:bzbarsky]]

Mats, could you take a look please?

At first glance, it looks like nsIFrame::FinishAndStoreOverflow used to call IsTableClip() for all frames but now we no longer do the equivalent of that or something?

Comment 6

[Boris Zbarsky [:bzbarsky]]

roc, any idea what's going on here?

Comment 7

[Mounir Lamouri (:mounir)]

Using NEEDSINFO flag per comment 6.

Comment 8

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

Not off the top of my head.

Comment 9

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #5)
> At first glance, it looks like nsIFrame::FinishAndStoreOverflow used to call
> IsTableClip() for all frames but now we no longer do the equivalent of that
> or something?

Exactly so.  We now only do the overflow:hidden clipping for some frame types:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.h#594
adding "IsFrameOfType(nsIFrame::eReplacedContainsBlock)" there seems to fix it:
https://hg.mozilla.org/try/rev/e0941f547a54
(iirc, we only handle the non-scrollframe case here)

I'm going to investigate more to make sure I don't miss any cases this time...
and write regression tests since we apparently don't have any covering this.

Comment 10

[Mats Palmgren (inactive)]

Attachment: fix+test

https://tbpl.mozilla.org/?tree=Try&rev=2058283350fc

Comment 11

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b6aff97358

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0b6aff97358