Closed Bug 894629 Opened 10 years ago Closed 10 years ago

Bad transition effect on google play website

Categories

(Core :: Layout, defect)

19 Branch
x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox22 --- affected
firefox23 - affected
firefox24 - affected
firefox25 - fixed

People

(Reporter: Fanolian+BMO, Assigned: coyotebush)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

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.
Confirmed on Mac OS X 10.8.4 with today's nightly.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: General → Untriaged
Product: Core → Firefox
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)
Blocks: 157681
Component: Untriaged → Layout
Keywords: regression
OS: Windows 8 → All
Product: Firefox → Core
Version: Trunk → 19 Branch
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.
Jet, can you please find an owner for this?  This looks *very* broken in Firefox :(
Assignee: nobody → bugs
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.
Attached file 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.
Assignee: bugs → cford
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
Attachment #778733 - Flags: review?(dbaron)
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
Attachment #778759 - Flags: review?(dbaron)
Attachment #778733 - Attachment is obsolete: true
Attachment #778733 - Flags: review?(dbaron)
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
Attachment #778759 - Flags: review?(dbaron) → review+
Also, if you don't mind, could you wait to land this until after bug 896138 lands?
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
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.
(Landed on mozilla-inbound, that is. It should get to mozilla-central soon.)
Attachment #778759 - Attachment is obsolete: true
Attachment #779249 - Flags: review+
Keywords: checkin-needed
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/1da1653dd884
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.