1024872 - Ad pans in the wrong direction

Status: RESOLVED WORKSFORME

(Web Compatibility :: Site Reports, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

When you start panning the ad goes in the opposite direction of the rest of content

Comment 1

[Kartikaya Gupta (email:[e-mail])]

The ad seems to shift as the url bar goes on and offscreen. Likely related to bug 1017427.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Milan, looks like a regression, got an assignee?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's see what the regression range is.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Brad, can you try backing out the patch from bug 1017427 and see if that takes care of this?

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Milan Sreckovic [:milan] from comment #4)
> Brad, can you try backing out the patch from bug 1017427 and see if that
> takes care of this?

This bug still exists without that patch, however without that patch the ad gets cut off from the bottom as well (so... situation is even uglier)

Comment 6

[Cristina Madaras, QA [:CristinaM]]

Build from 2013-11-18 looks good. This is happening in build from 2014-11-19:
https://www.youtube.com/watch?v=Ub4OE5hW6d0&feature=youtu.be

Regression window:
Last good: beddd6d4bcdf (2013-11-18)
First bad: ba9ecdea3a90 (2014-11-19)

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=beddd6d4bcdf&tochange=ba9ecdea3a90

Comment 7

[Chris Lord [:cwiiis]]

(In reply to cristina.madaras from comment #6)
> Build from 2013-11-18 looks good. This is happening in build from 2014-11-19:
> https://www.youtube.com/watch?v=Ub4OE5hW6d0&feature=youtu.be
> 
> Regression window:
> Last good: beddd6d4bcdf (2013-11-18)
> First bad: ba9ecdea3a90 (2014-11-19)
> 
> Pushlog:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=beddd6d4bcdf&tochange=ba9ecdea3a90

Huge number of commits in this window, but I'd suspect bug 919144. I'd also suspect that if that caused it, it's actually a bug in our AsyncCompositionManager that's been exposed by the layer tree being a little different.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Roc, Chris is pointing the finger at you.

Comment 9

[Kevin Brosnan [Ex-Mozilla]]

Verified Chris's assumptions.

The first bad revision is:
changeset:   156091:c7d20f37b046
user:        Robert O'Callahan <[e-mail]>
date:        Fri Sep 27 18:01:16 2013 +1200
summary:     Bug 919144. Part 7: Make fixed-pos frames with displayports animated geometry roots, and make FrameLayerBuilder responsible for setting fixed-pos layer parameters instead of nsDisplayFixedPosition. r=mattwoodrow

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Roc, are you going to be able to look at this? if not, can you suggest another assignee?

Comment 11

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

Not anytime soon.

(In reply to Chris Lord [:cwiiis] from comment #7)
> Huge number of commits in this window, but I'd suspect bug 919144. I'd also
> suspect that if that caused it, it's actually a bug in our
> AsyncCompositionManager that's been exposed by the layer tree being a little
> different.

If this is true, Botond or Chris himself might be good :-)

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Botond, hot potato to you

Comment 13

[Botond Ballo [:botond]]

I loaded the URL provided (http://techcrunch.com/2014/06/12/hashplex/) and panned around, but I didn't see any problems. The only ad I see on the page is the one in the middle of the article, and it pans fine along with the other content on the page.

If someone who's able to repro this problem could provide some more detailed STR, I would appreciate it.

Comment 14

[Kevin Brosnan [Ex-Mozilla]]

Looks like techcrunch changed their page. The Miami Herald articles [1] reproduce this issue. Load the page in Release or Beta and the ad at the bottom of the screen is motionless as you pan the page. In Aurora or nightly the ad jumps a bit and flickers as you quickly pan on the page. If you hold you finger on the screen you get the behavior that Brad mentions where the ad ends up below the screen when panning down. If you hold your finger when panning up the ad will sometimes detach from the bottom of the screen then after a moment it will reposition the ad back to the bottom of the screen. 

[1] http://www.miamiherald.com/2014/09/10/4339827/gulfstream-to-build-paint-facility.html

Comment 15

[Botond Ballo [:botond]]

I can repro with the Miami Herald article - thanks!

Comment 16

[Botond Ballo [:botond]]

The wrong positioning of the ad is correlated with when the dynamic toolbar is expanding or collapsing. During scroll gestures that don't cause the dynamic toolbar to expand or collapse, the ad stays in place where it should.

Comment 17

[Botond Ballo [:botond]]

Attachment: Fix attempt

The problem appears to be that the GetLayerFixedMarginsOffset() function in AsyncCompositionManager.cpp, which compares the compositor's notion of the fixed-layer margins (which are non-zero when the dynamic toolbar is visible) to content's notion of them, and adjusts the fixed-pos layer's shadow transform by the difference, only looks at the 'top' and 'left' components of the margins.

What I'm seeing is that when the dynamic toolbar is visible, it's the 'bottom' component of the margin that changes, not the top. Modifying the function to also consider the 'bottom' and 'right' components fixes the problem.

Chris, does this seem sensible?

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8487584 [details] [diff] [review]
Fix attempt

This patch isn't quite right. The reason we check 'left >= 0' and 'top >= 0' is explained here [1]. 

With this patch, for margins such as '.top=0, .bottom=72', both the 'top >= 0' and 'bottom >= 0' patches fire. This is OK most of the time, but sometimes we have a situation the compositor margins are '.top=72, .bottom=0' and the content margins are '.top=0, .bottom=72', and we end up making the offset twice as large as it should be.

I don't really understand why the margins are what they are - in particular, why we stometimes have top > 0, and other times bottom > 0. Chris, I'll try to ping you tomorrow to talk about this a bit and try to get a better understanding.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h?rev=e776483c4213#1115

Comment 19

[Botond Ballo [:botond]]

Further investigation revealed that the position-fixed element on this page is styled "top:100%; margin-top:-50px". This is an unusual way to get an element to be fixed the bottom of the viewport. 

A much simpler way, which works correctly, is to style it as "bottom:0".

I talked to :kats and :Cwiiis about this and it seems that correct handling of "top:100%; margin-top:-50px" with our current dynamic toolbar implementation would be very difficult. Therefore, I'm treating this bug as WONTFIX, and moving it to Tech Evangelism to advocate using "bottom:0" instead.

If anyone finds a page with an ad that's reasonably styled (e.g. "bottom:0" if it's fixed to the bottom of the viewport) which doesn't behave correctly, please file a new bug and feel free to assign it to me.

Comment 20

[Hallvord R. M. Steen [:hallvors]]

I disagree that "with our current architecture, this bug is hard to fix" should equal a "WONTFIX". I'm happy to contact the site with recommendations but I think a bug should be kept open on the core side to document this issue.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

I guess that comes down to what we think "WONTFIX" represents. Let me also point out that because of the flawed way they reimplement bottom:0 using top:100%, the ad also floats up off the bottom of the screen when the user zooms. In addition, the page behaves exactly the same way in chrome.

Fixing this bug while keeping asynchronous panning and zooming is almost impossible. At least i can't think of any reasonable way to do it. That is why i consider this bug a WONTFIX on the platform side. If somebody can come up with an elegant way to fix this class of problems then we would probably accept the patch though, so maybe INCOMPLETE would be a better resolution.

If we have a bug that will never be fixed, i don't see a point in leaving it open. A closed bug can serve as documentation just as well, and can be reopened if the need arises.

Comment 22

[Hallvord R. M. Steen [:hallvors]]

Fair enough. I got the impression from the comment talking about doing it "correctly" that we were violating the CSS spec here - if we are, if Chrome has exactly the same bug, and doing it "correctly" per spec is a big job that would harm performance or make things complex, we should complain to the CSS editors and go ahead with WONTFIXing it from a core point of view. Is somebody on this bug liaising with the CSS group?

Comment 23

[Kartikaya Gupta (email:[e-mail])]

(In reply to Hallvord R. M. Steen from comment #22)
> Is somebody on this bug liaising with the CSS group?

Not really. To be honest I'm not sure what we would request of the CSS group. Most of the CSS specs don't really consider the implications of async panning/zooming behaviour, and browser vendors all have had to come up with ad-hoc ways to try to maintain compatibility as much as possible while still reaping the perf advantages.

In a bunch of cases that means violating the CSS spec for transient periods of time while we are in the middle of doing some async operation. Cwiiis pointed out on IRC that checkerboarding is such a case. This bug is just another example of that, as our behaviour is wrong but only transiently while the dynamic toolbar is in the process of sliding off the screen.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

FWIW with the new dynamic toolbar implementation this is less bad. The ad remains in the right position while the toolbar is going and off the page, although it may jump/glitch while we resize the content area afterwards. That's a transient problem though, I'm inclined to close this as WFM.