894629 - Bad transition effect on google play website

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Fanolian]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:25.0) Gecko/20130716 Firefox/25.0 (Nightly/Aurora)
Build ID: 20130716030202

Steps to reproduce:

1. Go to https://play.google.com/store/apps/details?id=com.google.android.apps.maps
2. Scroll the app screenshots by pressing the long button on the right. Do not move the mouse while/after pressing.


Actual results:

The screenshots may not display at the center until moving the mouse out of the scroll button.


Expected results:

Screenshots should display properly.

Comment 1

[Jose Fandos]

Confirmed on Mac OS X 10.8.4 with today's nightly.

Comment 2

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/f5a441d6929f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120605210720
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/df6702c41ddd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120605215419
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f5a441d6929f&tochange=df6702c41ddd

Rregressed by: Bug 157681.
(Bug 157681 was disabled in Firefox16-18 by backed it out due to Bug 775350)

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

We could look at a low risk uplift nomination for Aurora but this has been present since FF19 so we wouldn't push a fix this late in FF23 cycle.

Comment 4

[(no longer active)]

Jet, can you please find an owner for this?  This looks *very* broken in Firefox :(

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The easiest first step for debugging this is probably testing which part of bug 157681 is the cause of the regression (which does require knowing how to separate or disable parts of the patch), as I did for bug 845837.

Comment 6

[Corey Ford [:coyotebush]]

Attachment: Reduced test case

Here's a reduced version of what's happening on the Google Play website. Notice that after the JS function runs, the green block is not centered, but resizing the window fixes that.

I'd guess that the margin/padding on the overflow:hidden element aren't being accounted for quite correctly in nsHTMLReflowState::ComputeRelativeOffsets (with a percentage offset). I'll look into that.

Comment 7

[Corey Ford [:coyotebush]]

Attachment: Reference containing block's content box for relatively positioned elements in nsCSSFrameConstructor::RecomputePosition. r=

Turns out RecomputePosition was using the containing block's border box (via GetSize) to determine a basis for those percentage offsets. It should be using the content box size instead (the padding-box rule from section 10.2 only applies to absolutely positioned elements).

The position-dynamic-changes reftests from bug 157681 still pass for me, and a try push:
https://tbpl.mozilla.org/?tree=Try&rev=85a4b6fa2278

Comment 8

[Corey Ford [:coyotebush]]

Attachment: Reference containing block's content box for relatively positioned elements in nsCSSFrameConstructor::RecomputePosition. (with test)

And now with a reftest.

New try run to test the test (maybe this needs some fuzziness like the others): https://tbpl.mozilla.org/?tree=Try&rev=18bcc6620697

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 778759 [details] [diff] [review]
Reference containing block's content box for relatively positioned elements in nsCSSFrameConstructor::RecomputePosition. (with test)

I'd perhaps make the variable:
  const nsSize cbSize = cb->GetContentRectRelativeToSelf().Size();
and then adjust the code just below to match, just since you don't actually need the whole rect.

Also, for the test, you should probably use a MozReftestInvalidate listener instead of using setTimeout.

And no need to include animate.js in the test.

r=dbaron with that

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, if you don't mind, could you wait to land this until after bug 896138 lands?

Comment 11

[Corey Ford [:coyotebush]]

The new reftest didn't give any trouble on Try, so I guess I'll leave out the random-if/fuzzy-if that the others have until and unless it does.

https://tbpl.mozilla.org/?tree=Try&rev=3aeda505dc4d

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

To follow up on comment 10:  Bug 896138 has now landed; RecomputePosition now lives in layout/base/RestyleManager.cpp but should be otherwise the same.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Landed on mozilla-inbound, that is. It should get to mozilla-central soon.)

Comment 14

[Corey Ford [:coyotebush]]

Attachment: Reference containing block's content box for relatively positioned elements in RestyleManager::RecomputePosition. (addressed comments, rebased)

Addressed review comments and rebased against latest m-c.

Comment 15

[(no longer active)]

Thanks a lot for the fix, Corey!

https://hg.mozilla.org/integration/mozilla-inbound/rev/1da1653dd884

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1da1653dd884