724352 - Extra scrollbar on overflow:auto blog post

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Nightly (13) and Aurora (12) show an extra scrollbar around the main text of http://blog.wolfram.com/2009/05/14/7-years-of-nksand-its-first-killer-app/. Beta (11) does not.

div.post has "overflow: auto" set, but since its height is (default) auto, I do not expect it to merit a scrollbar.

I've been seeing similar behavior in Google Reader occasionally for the last week or two of using Nightly.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Last good nightly: 2012-01-17
First bad nightly: 2012-01-18

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=34572943a3e4&tochange=f4049f65efc6

Guessing regression from bug 665597.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Bisecting inbound:

Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=797896e68634&tochange=60add44419ef

Bug 665597 comment 80 also mentions the Google Reader issues.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Attachment: Reduced testcase

Looks like it's an issue with lists with negative margins and overflow:auto.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Attachment: Reference testcase

Hopefully enough for reftests.

Comment 5

[Alice0775 White]

Attachment: another html

Comment 7

[Johnathan Nightingale [:johnath]]

Novel regression in firefox12, with impact on a pretty standard piece of CSS -> tracking-firefox12+

Comment 8

[Johnathan Nightingale [:johnath]]

Roc/Mats - should we be backing out the offending patch, or do you have an alternate remedy in mind?

Comment 9

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

I think we should try to fix this.

I think for nsBlockFrames whose bottom margin was carried out, we should not add their bottom margins to their overflow areas. Mats, what do you think?

Comment 10

[Alex Keybl [:akeybl]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> I think we should try to fix this.
> 
> I think for nsBlockFrames whose bottom margin was carried out, we should not
> add their bottom margins to their overflow areas. Mats, what do you think?

Now that FF12 is on beta, we should more strongly consider a backout at this point.

Comment 11

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

Backing out 665597 is very hard. I don't think fixing this bug is hard. Hopefully Mats can do this (soon).

Comment 13

[Stefan]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> Backing out 665597 is very hard. 

Of the seven changesets only two (905ef8691d96 7f8cf8e934a5) require moderate effort.

Comment 14

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

We'd also have to back out bug 524925, which would cause performance regressions.

Comment 15

[Stefan]

I did not back out any of bug 524925's six changesets.

Comment 16

[Mats Palmgren (inactive)]

Attachment: Simpler testcase

Comment 17

[Mats Palmgren (inactive)]

I think the reported problem is less serious than the problems that bug 665597
fixed, so I don't think we should backout.  This margin-collapsing problem was
discussed in bug 665597 and everyone accepted the tradeoff.
That said, I'll take a look at the code and try to come up with a solution.

Comment 18

[Alex Keybl [:akeybl]]

(In reply to Mats Palmgren [:mats] from comment #17)
> I think the reported problem is less serious than the problems that bug
> 665597
> fixed, so I don't think we should backout.  This margin-collapsing problem
> was
> discussed in bug 665597 and everyone accepted the tradeoff.
> That said, I'll take a look at the code and try to come up with a solution.

My concern comes from this note in the description, and the possibility that even more sites may regress:

> I've been seeing similar behavior in Google Reader occasionally for the last
> week or two of using Nightly.

We should really line up a fix for FF12.

Comment 19

[Mats Palmgren (inactive)]

Attachment: fix+tests

We don't need to add vertical margins here after reflow since that
has already been added to the child bounds, ie aBottomEdgeOfChildren
arg to nsBlockFrame::ComputeOverflowAreas, calculated by
nsBlockFrame::ComputeFinalSize here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#1351

(we will need it later though when doing UpdateOverflow correctly,
ie bug 719177, bug 725664, bug 720987)

Comment 20

[Mats Palmgren (inactive)]

Attachment: decrement expected assertion counts

This just decrements the assertion counts for some tests that have
huge margins and triggered bug 575011 with the old code.

Comment 21

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

Comment on attachment 611821 [details] [diff] [review]
fix+tests

Review of attachment 611821 [details] [diff] [review]:
-----------------------------------------------------------------

Kind of a hack, but the comment covers it.

Comment 22

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b18609e4bc57
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc7afdbc31ff

Comment 23

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/b18609e4bc57
https://hg.mozilla.org/mozilla-central/rev/cc7afdbc31ff

Comment 24

[Mats Palmgren (inactive)]

Comment on attachment 611821 [details] [diff] [review]
fix+tests

[Approval Request Comment]
Regression caused by (bug #): bug 665597
User impact if declined: a vertical scrollbar might appear on pages where negative margins are involved in margin-collapsing inside overflow:auto blocks
Testing completed (on m-c, etc.): baked on trunk since 2012-04-06
Risk to taking this patch (and alternatives if risky): low risk
String changes made by this patch: none

Comment 25

[Alex Keybl [:akeybl]]

Comment on attachment 611821 [details] [diff] [review]
fix+tests

[Triage Comment]
Low risk fix for a web regression in FF12. Approved for Aurora and Beta.

Comment 26

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/e33ea234ae51
http://hg.mozilla.org/releases/mozilla-aurora/rev/40e45c33965c

http://hg.mozilla.org/releases/mozilla-beta/rev/a595657d7f30
http://hg.mozilla.org/releases/mozilla-beta/rev/b316ffaf8657

Comment 27

[:Gavin Sharp [email: [e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #26)
> http://hg.mozilla.org/releases/mozilla-aurora/rev/40e45c33965c

philor pointed out on IRC that this seems to be missing one of the hunks from attachment 611825 [details] [diff] [review] (the last). Was that intentional?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Yes.  It's missing the last hunk, which doesn't apply on beta/aurora because it's changing the manifest annotations for tests that aren't present on those branches, as far as I can tell.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

It looks to me like the last chunk applies on aurora (but not beta). philor mentioned something about it causing a new perma-orange on aurora, it looks like that's the case:

https://tbpl.mozilla.org/?tree=Mozilla-Aurora

Comment 30

[Boris Zbarsky [:bzbarsky]]

Hmm...  It didn't apply for me, but maybe I messed it up somehow.  I'll take a look.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Ah, it failed to apply for a different reason.  Pushed http://hg.mozilla.org/releases/mozilla-aurora/rev/6456a42e0c97

Comment 32

[Simona Badau, Desktop QA]

Verified that no extra scrollbars are shown on Firefox 13 beta 3. Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.6 using the test cases attached in Comment 3 and Comment 5.

Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0