251 bytes, text/html
122 bytes, text/html
255 bytes, text/html
8.29 KB, patch
|Details | Diff | Splinter Review|
2.35 KB, patch
|Details | Diff | Splinter Review|
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
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.
Created attachment 594551 [details] Reduced testcase Looks like it's an issue with lists with negative margins and overflow:auto.
Assignee: nobody → matspal
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?
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).
(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.
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+
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla14
Status: NEW → RESOLVED
Last Resolved: 7 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
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.
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
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.