Bad transition effect on google play website

RESOLVED FIXED in Firefox 25

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Fanolian, Assigned: coyotebush)

Tracking

({regression})

19 Branch
mozilla25
x86_64
All
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox22 affected, firefox23- affected, firefox24- affected, firefox25- fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 years ago
Confirmed on Mac OS X 10.8.4 with today's nightly.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

4 years ago
Component: General → Untriaged
Product: Core → Firefox

Comment 2

4 years ago
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
status-firefox22: --- → affected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
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.
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → affected
tracking-firefox23: ? → -
tracking-firefox24: ? → -
tracking-firefox25: ? → -
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.
(Assignee)

Comment 6

4 years ago
Created attachment 778718 [details]
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)

Updated

4 years ago
Assignee: bugs → cford
(Assignee)

Comment 7

4 years ago
Created attachment 778733 [details] [diff] [review]
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
Attachment #778733 - Flags: review?(dbaron)
(Assignee)

Comment 8

4 years ago
Created attachment 778759 [details] [diff] [review]
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
Attachment #778759 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
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?
(Assignee)

Comment 11

4 years ago
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.)
(Assignee)

Comment 14

4 years ago
Created attachment 779249 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #778759 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #779249 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Thanks a lot for the fix, Corey!

https://hg.mozilla.org/integration/mozilla-inbound/rev/1da1653dd884
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/1da1653dd884
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

4 years ago
status-firefox25: affected → fixed
You need to log in before you can comment on or make changes to this bug.