Last Comment Bug 724352 - Extra scrollbar on overflow:auto blog post
: Extra scrollbar on overflow:auto blog post
Status: RESOLVED FIXED
[qa+]
: regression, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
-- normal with 1 vote (vote)
: mozilla14
Assigned To: Mats Palmgren (:mats)
:
: Jet Villegas (:jet)
Mentors:
http://blog.wolfram.com/2009/05/14/7-...
: 725871 739622 (view as bug list)
Depends on:
Blocks: 665597
  Show dependency treegraph
 
Reported: 2012-02-05 02:24 PST by Jesse Ruderman
Modified: 2012-05-10 08:44 PDT (History)
21 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
+
verified


Attachments
Reduced testcase (251 bytes, text/html)
2012-02-05 08:51 PST, Ryan VanderMeulen [:RyanVM]
no flags Details
Reference testcase (122 bytes, text/html)
2012-02-05 08:52 PST, Ryan VanderMeulen [:RyanVM]
no flags Details
another html (241 bytes, text/html)
2012-02-05 09:24 PST, Alice0775 White
no flags Details
Simpler testcase (255 bytes, text/html)
2012-03-27 15:34 PDT, Mats Palmgren (:mats)
no flags Details
fix+tests (8.29 KB, patch)
2012-04-03 08:20 PDT, Mats Palmgren (:mats)
roc: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
decrement expected assertion counts (2.35 KB, patch)
2012-04-03 08:23 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2012-02-05 02:24:16 PST
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 User image Ryan VanderMeulen [:RyanVM] 2012-02-05 08:08:58 PST
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 User image Ryan VanderMeulen [:RyanVM] 2012-02-05 08:15:37 PST
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 User image Ryan VanderMeulen [:RyanVM] 2012-02-05 08:51:56 PST
Created attachment 594551 [details]
Reduced testcase

Looks like it's an issue with lists with negative margins and overflow:auto.
Comment 4 User image Ryan VanderMeulen [:RyanVM] 2012-02-05 08:52:39 PST
Created attachment 594552 [details]
Reference testcase

Hopefully enough for reftests.
Comment 5 User image Alice0775 White 2012-02-05 09:24:52 PST
Created attachment 594556 [details]
another html
Comment 6 User image Josh Gunderson 2012-02-10 13:24:27 PST
*** Bug 725871 has been marked as a duplicate of this bug. ***
Comment 7 User image Johnathan Nightingale [:johnath] 2012-02-22 07:40:40 PST
Novel regression in firefox12, with impact on a pretty standard piece of CSS -> tracking-firefox12+
Comment 8 User image Johnathan Nightingale [:johnath] 2012-02-22 07:41:07 PST
Roc/Mats - should we be backing out the offending patch, or do you have an alternate remedy in mind?
Comment 9 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-28 04:19:21 PST
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 User image Alex Keybl [:akeybl] 2012-03-19 16:06:55 PDT
(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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2012-03-19 16:08:40 PDT
Backing out 665597 is very hard. I don't think fixing this bug is hard. Hopefully Mats can do this (soon).
Comment 12 User image Stefan 2012-03-27 08:51:20 PDT
*** Bug 739622 has been marked as a duplicate of this bug. ***
Comment 13 User image Stefan 2012-03-27 11:21:13 PDT
(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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2012-03-27 15:12:53 PDT
We'd also have to back out bug 524925, which would cause performance regressions.
Comment 15 User image Stefan 2012-03-27 15:21:21 PDT
I did not back out any of bug 524925's six changesets.
Comment 16 User image Mats Palmgren (:mats) 2012-03-27 15:34:30 PDT
Created attachment 609920 [details]
Simpler testcase
Comment 17 User image Mats Palmgren (:mats) 2012-03-27 15:47:43 PDT
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 User image Alex Keybl [:akeybl] 2012-03-29 17:30:38 PDT
(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 User image Mats Palmgren (:mats) 2012-04-03 08:20:16 PDT
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)
Comment 20 User image Mats Palmgren (:mats) 2012-04-03 08:23:21 PDT
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.
Comment 21 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-03 13:34:00 PDT
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 24 User image Mats Palmgren (:mats) 2012-04-07 15:20:37 PDT
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 User image Alex Keybl [:akeybl] 2012-04-09 09:45:43 PDT
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 27 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-10 15:16:25 PDT
(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 User image Boris Zbarsky [:bz] (still a bit busy) 2012-04-10 16:51:08 PDT
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 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-10 16:54:19 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2012-04-10 17:05:14 PDT
Hmm...  It didn't apply for me, but maybe I messed it up somehow.  I'll take a look.
Comment 31 User image Boris Zbarsky [:bz] (still a bit busy) 2012-04-10 17:07:50 PDT
Ah, it failed to apply for a different reason.  Pushed http://hg.mozilla.org/releases/mozilla-aurora/rev/6456a42e0c97
Comment 32 User image Simona B [:simonab ] 2012-05-10 08:44:07 PDT
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

Note You need to log in before you can comment on or make changes to this bug.