Closed Bug 1024872 Opened 8 years ago Closed 6 years ago

Ad pans in the wrong direction

Categories

(Web Compatibility :: Mobile, defect)

Other
Android
defect
Not set
normal

Tracking

(firefox31 affected, firefox32 affected, firefox33 affected, firefox34 affected, fennec-)

RESOLVED WORKSFORME
Tracking Status
firefox31 --- affected
firefox32 --- affected
firefox33 --- affected
firefox34 --- affected
fennec - ---

People

(Reporter: blassey, Unassigned, Mentored)

References

()

Details

(Keywords: regression, Whiteboard: [country-all] [css] [contactready])

Attachments

(1 obsolete file)

When you start panning the ad goes in the opposite direction of the rest of content
The ad seems to shift as the url bar goes on and offscreen. Likely related to bug 1017427.
Keywords: qawanted
tracking-fennec: ? → 33+
Milan, looks like a regression, got an assignee?
Flags: needinfo?(milan)
Let's see what the regression range is.
Brad, can you try backing out the patch from bug 1017427 and see if that takes care of this?
Flags: needinfo?(milan) → needinfo?(blassey.bugs)
(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)
Flags: needinfo?(blassey.bugs)
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
(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.
Roc, Chris is pointing the finger at you.
Assignee: nobody → roc
Verified Chris's assumptions.

The first bad revision is:
changeset:   156091:c7d20f37b046
user:        Robert O'Callahan <robert@ocallahan.org>
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
Blocks: 919144
Roc, are you going to be able to look at this? if not, can you suggest another assignee?
Flags: needinfo?(roc)
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 :-)
Flags: needinfo?(roc)
Botond, hot potato to you
Assignee: roc → 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.
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
I can repro with the Miami Herald article - thanks!
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.
Attached patch Fix attempt (obsolete) — Splinter Review
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?
Attachment #8487584 - Flags: feedback?(chrislord.net)
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
Attachment #8487584 - Flags: feedback?(chrislord.net)
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.
Component: Graphics, Panning and Zooming → Mobile
Product: Firefox for Android → Tech Evangelism
Attachment #8487584 - Attachment is obsolete: true
Assignee: botond → nobody
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.
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.
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?
Mentor: hsteen
Whiteboard: [country-all] [css] [contactready]
(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.
tracking-fennec: 33+ → -
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
Product: Tech Evangelism → Web Compatibility
You need to log in before you can comment on or make changes to this bug.