Extra scrollbar on overflow:auto blog post

RESOLVED FIXED in Firefox 12

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: Mats Palmgren (vacation - back in August))

Tracking

({regression, testcase})

Trunk
mozilla14
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

(Whiteboard: [qa+], URL)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
Keywords: regressionwindow-wanted
OS: Mac OS X → All
Hardware: x86_64 → All
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.
Blocks: 665597
Created attachment 594551 [details]
Reduced testcase

Looks like it's an issue with lists with negative margins and overflow:auto.
Keywords: testcase-wanted
Created attachment 594552 [details]
Reference testcase

Hopefully enough for reftests.
Flags: in-testsuite?

Comment 5

6 years ago
Created attachment 594556 [details]
another html
(Reporter)

Updated

6 years ago
Keywords: testcase

Updated

6 years ago
Attachment #594556 - Attachment is obsolete: true
Assignee: nobody → matspal

Updated

6 years ago
Duplicate of this bug: 725871
Novel regression in firefox12, with impact on a pretty standard piece of CSS -> tracking-firefox12+
tracking-firefox12: ? → +
Roc/Mats - should we be backing out the offending patch, or do you have an alternate remedy in mind?

Updated

6 years ago
tracking-firefox13: ? → +
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).

Updated

6 years ago
Duplicate of this bug: 739622

Comment 13

6 years ago
(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.

Comment 15

6 years ago
I did not back out any of bug 524925's six changesets.
Created attachment 609920 [details]
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.
Created attachment 611821 [details] [diff] [review]
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)
Attachment #611821 - Flags: review?(roc)
Created attachment 611825 [details] [diff] [review]
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.
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+
Attachment #611825 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b18609e4bc57
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc7afdbc31ff
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/b18609e4bc57
https://hg.mozilla.org/mozilla-central/rev/cc7afdbc31ff
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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+
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
status-firefox12: affected → fixed
status-firefox13: affected → fixed
(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.
Ah, it failed to apply for a different reason.  Pushed http://hg.mozilla.org/releases/mozilla-aurora/rev/6456a42e0c97
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
status-firefox13: fixed → verified
You need to log in before you can comment on or make changes to this bug.