Last Comment Bug 894629 - Bad transition effect on google play website
: Bad transition effect on google play website
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 19 Branch
: x86_64 All
: -- normal with 1 vote (vote)
: mozilla25
Assigned To: Corey Ford [:coyotebush]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 157681
  Show dependency treegraph
 
Reported: 2013-07-16 14:39 PDT by Fanolian
Modified: 2013-07-23 00:32 PDT (History)
11 users (show)
corey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
-
affected
-
affected
-
fixed


Attachments
Reduced test case (528 bytes, text/html)
2013-07-19 16:31 PDT, Corey Ford [:coyotebush]
no flags Details
Reference containing block's content box for relatively positioned elements in nsCSSFrameConstructor::RecomputePosition. r= (1.58 KB, patch)
2013-07-19 17:07 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Reference containing block's content box for relatively positioned elements in nsCSSFrameConstructor::RecomputePosition. (with test) (3.85 KB, patch)
2013-07-19 18:13 PDT, Corey Ford [:coyotebush]
dbaron: review+
Details | Diff | Splinter Review
Reference containing block's content box for relatively positioned elements in RestyleManager::RecomputePosition. (addressed comments, rebased) (3.46 KB, patch)
2013-07-22 09:31 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review

Description Fanolian 2013-07-16 14:39:39 PDT
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 2013-07-16 15:33:57 PDT
Confirmed on Mac OS X 10.8.4 with today's nightly.
Comment 2 Alice0775 White 2013-07-16 20:14:19 PDT
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 2013-07-17 12:28:36 PDT
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 :Ehsan Akhgari 2013-07-17 14:06:18 PDT
Jet, can you please find an owner for this?  This looks *very* broken in Firefox :(
Comment 5 David Baron :dbaron: ⌚️UTC-8 2013-07-17 15:47:09 PDT
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] 2013-07-19 16:31:29 PDT
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.
Comment 7 Corey Ford [:coyotebush] 2013-07-19 17:07:57 PDT
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
Comment 8 Corey Ford [:coyotebush] 2013-07-19 18:13:45 PDT
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
Comment 9 David Baron :dbaron: ⌚️UTC-8 2013-07-19 18:28:53 PDT
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-8 2013-07-19 22:11:13 PDT
Also, if you don't mind, could you wait to land this until after bug 896138 lands?
Comment 11 Corey Ford [:coyotebush] 2013-07-20 11:18:53 PDT
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-8 2013-07-20 20:58:11 PDT
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-8 2013-07-20 20:58:56 PDT
(Landed on mozilla-inbound, that is. It should get to mozilla-central soon.)
Comment 14 Corey Ford [:coyotebush] 2013-07-22 09:31:50 PDT
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.
Comment 15 :Ehsan Akhgari 2013-07-22 10:57:35 PDT
Thanks a lot for the fix, Corey!

https://hg.mozilla.org/integration/mozilla-inbound/rev/1da1653dd884
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-07-22 18:59:13 PDT
https://hg.mozilla.org/mozilla-central/rev/1da1653dd884

Note You need to log in before you can comment on or make changes to this bug.