sticky-ad IOS scrolling issue fix #3700

zhouyx · 2016-06-21T22:34:57Z

fix the issue that amp-sticky-ad disappear before it shows amp-ad.
But ad still only displays after scrolling is fully stopped.
#3599

zhouyx · 2016-06-21T22:35:27Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.css

@@ -28,6 +28,7 @@ amp-sticky-ad {
 }

 .-amp-sticky-ad-layout {
+  visibility: hidden;


How can we prevent this from being customized?

That also prevent myself from setting it to visible.
Here's my solution:
this.element.classList.add('-amp-sticky-ad-display')
.-amp-sticky-ad-display { visibility: visible !important; }

You have to set visible !important

You'll have to set it to visible via a class as well. E.g.

.myclass { visibility: hidden !important; } .myclass.visible { visibility: visible !important; }

dvoytenko · 2016-06-22T05:37:05Z

@zhouyx Could you please explain why the issue is happening now? What causes the blink on iOS?

jridgewell · 2016-06-22T16:04:17Z

But ad still only displays after scrolling is fully stopped.

Demo please. This worked perfectly for me before.

zhouyx · 2016-06-22T17:58:25Z

That probably needs one to scroll really fast.

jridgewell · 2016-06-22T18:14:08Z

Got it. The demo I was playing with earlier was not the current version of <stick-ad>, it had a transform: translateZ(0). That's the part that made the visibility changes paint immediately.

zhouyx · 2016-06-23T01:03:57Z

I don't remember putting transform: translateZ(0) to <amp-sticky-ad> anywhere. But if that solves the problem, can we adapt it?

zhouyx · 2016-06-23T01:25:36Z

Sadly I still haven't found the reason to the blink. Still looking for it, but it makes me really hard to debug because the issue only occurs when fetching amp-ad.js remotely. @dvoytenko

dvoytenko · 2016-06-23T02:28:44Z

@zhouyx To confirm, however, according to the description of this PR it follows that switching from display to visibility solves the problem?

zhouyx · 2016-06-23T04:44:55Z

Yes, switching from display to visibility solves the blink problem

zhouyx · 2016-06-23T21:10:09Z

Discuss with @mkhatib, find one thing. Note to self.
By setting ownership of <amp-ad>, after user switch viewer and come back again w/o closing <amp-sticky-ad>, <amp-ad> layout need to be rescheduled by <amp-sticky-ad>

zhouyx · 2016-06-23T23:32:03Z

PTAL

dvoytenko · 2016-06-24T00:33:46Z

examples/sticky.ads.amp.html

@@ -160,7 +159,7 @@
    </div>
  </header>
  <main role="main">
-  <amp-sticky-ad layout="nodisplay">
+  <amp-sticky-ad layout="container">


No need to specify layout=container - it's default. However, there's an issue here and why I'd like to go back to nodisplay. To start with, it does as as nodisplay. The second reason is that by the time the amp-sticky-ad code is done downloading the element might be actually visible, which could trigger all kind of issues that we don't want. Instead, we need to start with nodisplay, but as soon as amp-sticky-ad is initialized (buildCallback) to remove display:none and instead set visibility:hidden.

Do you mean that before we get amp-sticky-ad css file, it might be treated as a normal div and would be visible.
changed the layout type back to nodisplay

The blink comes back with display change.

Can I see the updated code? The goal is to remove display:none and set visibility:hidden as soon as possible, e.g. in the buildCallback. I can't imagine the blink will still be there in this case.

The current code is updated. Here's what I found, after setting display:flex through css classList, I still find style = "display: none" on the HTML element.

I add toggle(this.element, true) to buildCallback(), unfortunately I still see blink

dvoytenko · 2016-06-24T20:13:25Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.js

@@ -38,21 +39,25 @@ class AmpStickyAd extends AMP.BaseElement {
      dev.warn(TAG, `TAG ${TAG} disabled`);
      return;
    }
-
+    toggle(this.element, true);


Probably should be done after the class is set below.

This is needed because the NODISPLAY layout? Why don't we change it to CONTAINER?

Get this comment from @dvoytenko

No need to specify layout=container - it's default. However, there's an issue here and why I'd like to go back to nodisplay. To start with, it does as as nodisplay. The second reason is that by the time the amp-sticky-ad code is done downloading the element might be actually visible, which could trigger all kind of issues that we don't want. Instead, we need to start with nodisplay, but as soon as amp-sticky-ad is initialized (buildCallback) to remove display:none and instead set visibility:hidden.

dvoytenko · 2016-06-24T20:17:34Z

@zhouyx The code looks fine now. At this point, since we remove display:none as soon as possible, I really do not understand the reason why it'd blink - there's simply no real difference here. One think we need to try and see if it helps: we need to move this.viewport_.addToFixedLayer(this.element); from where it is now to:

Try to put it immediately after toggle(this.element, true) in the buildCallback
Try to put it in the layoutCallback

If both (1) and (2) help - my preference is (2).

… amp-sticky-ad

jridgewell · 2016-06-28T17:43:10Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.css

@@ -29,17 +29,21 @@ amp-sticky-ad {

 .-amp-sticky-ad-layout {
  display: flex;
+  visibility: hidden;


Aren't we forcing this with !important?

you're right, will fix

zhouyx · 2016-06-28T18:52:36Z

PTAL

jridgewell · 2016-06-28T18:55:51Z

LGTM.

zhouyx added the NEEDS REVIEW label Jun 21, 2016

zhouyx assigned jridgewell Jun 21, 2016

zhouyx reviewed Jun 21, 2016
View reviewed changes

zhouyx closed this Jun 22, 2016

zhouyx reopened this Jun 22, 2016

dvoytenko reviewed Jun 24, 2016
View reviewed changes

zhouyx force-pushed the ios-fix branch from 2e14777 to fd9e506 Compare June 24, 2016 01:25

dvoytenko reviewed Jun 24, 2016
View reviewed changes

zhouyx added 5 commits June 27, 2016 13:13

delete toggle, add visibility

0bb8ed4

change support layout type to container

3eee785

lint fix

eea6a80

set owner of amp-ad to sticky-ad

fc7e437

change support layout type back to nodisplay

57c48f0

zhouyx force-pushed the ios-fix branch from 7e8d0f5 to ac3a61d Compare June 27, 2016 21:46

zhouyx added 2 commits June 28, 2016 10:01

set background-color to amp-ad inside amp-sticky-ad, add transform to…

75e815b

… amp-sticky-ad

toggle display in buildCallback

c47195c

zhouyx force-pushed the ios-fix branch from 28370e3 to c47195c Compare June 28, 2016 17:02

jridgewell reviewed Jun 28, 2016
View reviewed changes

zhouyx added 2 commits June 28, 2016 11:49

add visibility:hidden !important

aa2fcd1

use transform and visiblity

e83d7f8

zhouyx force-pushed the ios-fix branch from dbae0a9 to e83d7f8 Compare June 28, 2016 18:51

jridgewell added LGTM and removed NEEDS REVIEW labels Jun 28, 2016

zhouyx merged commit 8d079d5 into ampproject:master Jun 28, 2016

zhouyx deleted the ios-fix branch September 28, 2016 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sticky-ad IOS scrolling issue fix #3700

sticky-ad IOS scrolling issue fix #3700

zhouyx commented Jun 21, 2016 •

edited

zhouyx Jun 21, 2016 •

edited

jridgewell Jun 22, 2016 •

edited

zhouyx Jun 23, 2016 •

edited

jridgewell Jun 23, 2016

dvoytenko Jun 23, 2016

dvoytenko commented Jun 22, 2016

jridgewell commented Jun 22, 2016

zhouyx commented Jun 22, 2016

jridgewell commented Jun 22, 2016

zhouyx commented Jun 23, 2016

zhouyx commented Jun 23, 2016

dvoytenko commented Jun 23, 2016

zhouyx commented Jun 23, 2016

zhouyx commented Jun 23, 2016

zhouyx commented Jun 23, 2016

dvoytenko Jun 24, 2016

zhouyx Jun 24, 2016

zhouyx Jun 24, 2016

dvoytenko Jun 24, 2016

zhouyx Jun 24, 2016

zhouyx Jun 24, 2016

dvoytenko Jun 24, 2016

jridgewell Jun 28, 2016

zhouyx Jun 28, 2016

jridgewell Jun 28, 2016

dvoytenko commented Jun 24, 2016

jridgewell Jun 28, 2016

zhouyx Jun 28, 2016

zhouyx commented Jun 28, 2016

jridgewell commented Jun 28, 2016

sticky-ad IOS scrolling issue fix #3700

sticky-ad IOS scrolling issue fix #3700

Conversation

zhouyx commented Jun 21, 2016 • edited

zhouyx Jun 21, 2016 • edited

Choose a reason for hiding this comment

jridgewell Jun 22, 2016 • edited

Choose a reason for hiding this comment

zhouyx Jun 23, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Jun 22, 2016

jridgewell commented Jun 22, 2016

zhouyx commented Jun 22, 2016

jridgewell commented Jun 22, 2016

zhouyx commented Jun 23, 2016

zhouyx commented Jun 23, 2016

dvoytenko commented Jun 23, 2016

zhouyx commented Jun 23, 2016

zhouyx commented Jun 23, 2016

zhouyx commented Jun 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Jun 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx commented Jun 28, 2016

jridgewell commented Jun 28, 2016

zhouyx commented Jun 21, 2016 •

edited

zhouyx Jun 21, 2016 •

edited

jridgewell Jun 22, 2016 •

edited

zhouyx Jun 23, 2016 •

edited