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)
:
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 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 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 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 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 Ryan VanderMeulen [:RyanVM] 2012-02-05 08:52:39 PST
Created attachment 594552 [details]
Reference testcase

Hopefully enough for reftests.
Comment 5 Alice0775 White 2012-02-05 09:24:52 PST
Created attachment 594556 [details]
another html
Comment 6 Josh Gunderson 2012-02-10 13:24:27 PST
*** Bug 725871 has been marked as a duplicate of this bug. ***
Comment 7 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 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 Robert O'Callahan (:roc) (Exited; 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 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 Robert O'Callahan (:roc) (Exited; 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 Stefan 2012-03-27 08:51:20 PDT
*** Bug 739622 has been marked as a duplicate of this bug. ***
Comment 13 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 Robert O'Callahan (:roc) (Exited; 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 Stefan 2012-03-27 15:21:21 PDT
I did not back out any of bug 524925's six changesets.
Comment 16 Mats Palmgren (:mats) 2012-03-27 15:34:30 PDT
Created attachment 609920 [details]
Simpler testcase
Comment 17 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 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 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 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 Robert O'Callahan (:roc) (Exited; 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 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 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 :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 Boris Zbarsky [:bz] 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 :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 Boris Zbarsky [:bz] 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 Boris Zbarsky [:bz] 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 Simona B [:simonab ] -PTO- back Sept 5th 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.