1190635 - BuzzFeed's Facebook comments are misaligned, overlap sidebar ads

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: 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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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

Comment 3

[Cameron McCormack (:heycam)]

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?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 5

[Cameron McCormack (:heycam)]

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!

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/458994e3fcc9929233dfd4dfa5b1f244e2c8c732
changeset:  458994e3fcc9929233dfd4dfa5b1f244e2c8c732
user:       Robert O'Callahan <[e-mail]>
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 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I believe this affects Fx40 since we did land 1172239 on beta.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/458994e3fcc9

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked. 

Chris, could you please verify the fix on Nightly/Aurora 8/5/15 build or later? Thanks.

Comment 15

[Carsten Book [:Tomcat]]

i guess this needs rebased for aurora

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: patch for aurora

Rebased to Aurora.

Comment 17

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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 18

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d26b3266a147

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking verified for Beta based on comment 17.

Comment 21

[Chris Peterson [:cpeterson]]

Verified fixed in Nightly 42 and Aurora 41.