Closed Bug 1190635 Opened 4 years ago Closed 4 years ago

BuzzFeed's Facebook comments are misaligned, overlap sidebar ads

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox39 --- unaffected
firefox40 + verified
firefox41 + verified
firefox42 --- verified

People

(Reporter: cpeterson, Assigned: roc)

References

()

Details

Attachments

(4 files)

Attached image screenshot.jpg
@ roc, could this bug be a regression from bug 1172239? I bisected the regression to the following inbound pushlog, which implicates bug 1172239:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a12d1b2e6e17&tochange=09878dde0cc8

STR:
1. Load http://www.buzzfeed.com/beachesoffortmyersandsanibel/astonishing-images-that-will-make-you-want-to-l
2. Quickly scroll to the bottom of the (long!) page and then start scrolling back up.

RESULT:
The article's Facebook comments shift from the left side of the page beneath the article text to the right side overlapping the sidebar ads. See the attached screenshot.

[Tracking Requested - why for this release]:

This appears to be a regression in Firefox 41. I can reproduce the bug in Nightly 42 and Aurora 41, but not Beta 40.
Flags: needinfo?(roc)
Attached file reduced testcase
I think this is the bug. Setting the width and height on the red span should cause it to move down below the float, but it doesn't.
Assignee: nobody → roc
Flags: needinfo?(roc)
Bug 1190635. Don't early-return for an mHeight change, since width changes can add extra change hints. r=heycam
Attachment #8642827 - Flags: review?(cam)
Comment on attachment 8642827 [details]
MozReview Request: Bug 1190635. Don't early-return for an mHeight change, since width changes can add extra change hints. r=heycam

https://reviewboard.mozilla.org/r/14967/#review13381

::: layout/style/nsStyleStruct.cpp:1603
(Diff revision 1)
> -    return NS_CombineHint(hint, nsChangeHint_NeedReflow |
> +    NS_UpdateHint(hint, NS_CombineHint(hint, nsChangeHint_NeedReflow |

NS_UpdateHint will already keep the old hint bits set, so no need to include hint in the NS_CombineHint call.

::: layout/style/nsStyleStruct.cpp:1620
(Diff revision 1)
>    // If width and height have not changed, but any of the offsets have changed,

This comment still expects us to have returned earlier.  But given this block can add nsChangeHint_AllReflowHints, it's not clear to me why we should return early in the width-differing case.  Does the work done for the reflow bits set in the width-differing case subsume that done for mOffset differences?
Attachment #8642827 - Flags: review?(cam)
Comment on attachment 8642827 [details]
MozReview Request: Bug 1190635. Don't early-return for an mHeight change, since width changes can add extra change hints. r=heycam

Bug 1190635. Don't early-return for an mHeight change, since width changes can add extra change hints. r=heycam

Don't return early for an mWidth change either.
Attachment #8642827 - Flags: review?(cam)
Attachment #8642827 - Flags: review?(cam) → review+
Comment on attachment 8642827 [details]
MozReview Request: Bug 1190635. Don't early-return for an mHeight change, since width changes can add extra change hints. r=heycam

https://reviewboard.mozilla.org/r/14967/#review13383

Ship It!
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/458994e3fcc9929233dfd4dfa5b1f244e2c8c732
changeset:  458994e3fcc9929233dfd4dfa5b1f244e2c8c732
user:       Robert O'Callahan <robert@ocallahan.org>
date:       Tue Aug 04 16:41:50 2015 +1200
description:
Bug 1190635. Don't early-return for an mHeight change, since width changes can add extra change hints. r=heycam

Don't return early for an mWidth change either.
Comment on attachment 8642827 [details]
MozReview Request: Bug 1190635. Don't early-return for an mHeight change, since width changes can add extra change hints. r=heycam

Approval Request Comment
[Feature/regressing bug #]: 1172239
[User impact if declined]: Bad layout on some major sites
[Describe test coverage new/current, TreeHerder]: layout patches are reasonably well tested with reftests, but these are complex features.
[Risks and why]: This particular patch is very low risk. We should definitely take it on Aurora. On beta we should either take this patch, or back out bug 1172239 on beta in case there are further regressions (none known at this time).
[String/UUID change made/needed]: none
Attachment #8642827 - Flags: approval-mozilla-beta?
Attachment #8642827 - Flags: approval-mozilla-aurora?
I believe this affects Fx40 since we did land 1172239 on beta.
I guess that means that we will have to do a second RC...
About bug 1172239, as we released 39 with this bug, I think we should backout and let the fix ride the train from 41.
Robert, in your test coverage, I was unable to tell whether you have manually locally verified the fix or not. This will give me more confidence in approving the Aurora uplift request.
Flags: needinfo?(roc)
https://hg.mozilla.org/mozilla-central/rev/458994e3fcc9
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(In reply to Ritu Kothari (:ritu) from comment #10)
> Robert, in your test coverage, I was unable to tell whether you have
> manually locally verified the fix or not. This will give me more confidence
> in approving the Aurora uplift request.

I have, it does fix Buzzfeed.
Flags: needinfo?(roc)
Comment on attachment 8642827 [details]
MozReview Request: Bug 1190635. Don't early-return for an mHeight change, since width changes can add extra change hints. r=heycam

Let's uplift to Aurora. Dev has verified the fix locally and there are reftests. Approved.
Attachment #8642827 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracked. 

Chris, could you please verify the fix on Nightly/Aurora 8/5/15 build or later? Thanks.
Flags: needinfo?(cpeterson)
i guess this needs rebased for aurora
Flags: needinfo?(roc)
Attached patch patch for auroraSplinter Review
Rebased to Aurora.
Flags: needinfo?(roc)
Attachment #8643513 - Flags: approval-mozilla-aurora+
I can confirm that the backout for bug 1172239 fixed the issue on Firefox 40 RC 2 (verified on Windows 7 x64, Windows 10 x64, Mac OS X 10.10, Ubuntu 14.04 x86).
Comment on attachment 8642827 [details]
MozReview Request: Bug 1190635. Don't early-return for an mHeight change, since width changes can add extra change hints. r=heycam

We backed out the regressing bug from beta. Dropping the old approval request.
Attachment #8642827 - Flags: approval-mozilla-beta?
Marking verified for Beta based on comment 17.
Verified fixed in Nightly 42 and Aurora 41.
Status: RESOLVED → VERIFIED
Flags: needinfo?(cpeterson)
You need to log in before you can comment on or make changes to this bug.