Extra scrollbar on overflow:auto blog post

RESOLVED FIXED in Firefox 12

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jruderman, Assigned: mats)

Tracking

({regression, testcase})

Trunk
mozilla14
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox11 unaffected, firefox12+ fixed, firefox13+ verified)

Details

(Whiteboard: [qa+], )

Attachments

(5 attachments, 1 obsolete attachment)

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.
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.
OS: Mac OS X → All
Hardware: x86_64 → All
Posted file Reduced testcase
Looks like it's an issue with lists with negative margins and overflow:auto.
Posted file Reference testcase
Hopefully enough for reftests.
Flags: in-testsuite?
Posted file another html (obsolete) —
Keywords: testcase
Attachment #594556 - Attachment is obsolete: true
Duplicate of this bug: 725871
Novel regression in firefox12, with impact on a pretty standard piece of CSS -> tracking-firefox12+
Roc/Mats - should we be backing out the offending patch, or do you have an alternate remedy in mind?
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?
(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.
Backing out 665597 is very hard. I don't think fixing this bug is hard. Hopefully Mats can do this (soon).
Duplicate of this bug: 739622
(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.
We'd also have to back out bug 524925, which would cause performance regressions.
I did not back out any of bug 524925's six changesets.
Posted file Simpler testcase
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.
(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.
Posted patch fix+testsSplinter Review
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)
Attachment #611821 - Flags: review?(roc)
This just decrements the assertion counts for some tests that have
huge margins and triggered bug 575011 with the old code.
Attachment #611825 - Flags: review?(roc)
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.
Attachment #611821 - Flags: review?(roc) → review+
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
Attachment #611821 - Flags: approval-mozilla-beta?
Attachment #611821 - Flags: approval-mozilla-aurora?
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.
Attachment #611821 - Flags: approval-mozilla-beta?
Attachment #611821 - Flags: approval-mozilla-beta+
Attachment #611821 - Flags: approval-mozilla-aurora?
Attachment #611821 - Flags: approval-mozilla-aurora+
(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?
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.
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
Hmm...  It didn't apply for me, but maybe I messed it up somehow.  I'll take a look.
Whiteboard: [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
You need to log in before you can comment on or make changes to this bug.