Rewrite PositionObserver for better sharing #10813

zhouyx · 2017-08-05T01:06:35Z

Instead of extending from an abstract position observer class, the difference between inabox or ampdoc are handled in Viewport class. Thus the PositionObserver should be identical in both cases.

zhouyx · 2017-08-09T23:10:06Z

PTAL

aghassemi

Awesome refactoring! few small requests.

aghassemi · 2017-08-09T23:13:57Z

ads/inabox/position-observer.js

+  }
+
+  connect() {
+    //console.log('connect but do nothing! should fail');


aghassemi · 2017-08-09T23:18:05Z

src/inabox/inabox-position-observer.js

+
+import {Services} from '../../src/services';
+import {registerServiceBuilderForDoc} from '../../src/service';
+import {


nit: one line for single imports

aghassemi · 2017-08-09T23:40:07Z

src/service/position-observer/position-observer-viewport-info.js

+   * @param {function(?)} unusedCallback
+   * @return {function()}
+   */
+  onHostMessage(unusedCallback) {


aghassemi · 2017-08-09T23:51:30Z

ads/inabox/position-observer.js

+
+
+/**
+ * @implements {PosObViewportInfoDef}


naming suggestions:
PositionObserverHostInterface
PositionObserverNonAmpHost
etc...

Haha I am really bad at naming.
changed to PosObHostInterfaceDef, then PosObAmpdocHostInterface PosObInaboxHostInterface PosObNonAmpHostInterface

aghassemi · 2017-08-10T00:00:41Z

src/inabox/inabox-position-observer.js

+      // If not receive iframe position from host, or if iframe is outside vp
+      return null;
+    }
+    if (!element.inIframePositionRect) {


can this really happen? like getLayoutRect be called before inIframePositionRect is set for InABoxCase?

Yes, but not because getLayoutRect is called before.
It happens when the old inIframePositionRect is outdated, and we clear the stored value. For example, if the inabox iframe gets resized, we would want to refresh the inIframePositionRect value.

I see. So to make sure I understand this right: I was expecting another AmpDocPositionObserver in here monitoring the element but I guess in-a-box is not scrollable (which will likely stay true for forseeable future) so the only real event that can invalidate this is a resize change and the host handles that. Correct?

aghassemi · 2017-08-10T00:04:32Z

src/service/position-observer/position-observer-entry.js

+    if (prePos
+        && layoutRectEquals(prePos.positionRect, position.positionRect)
+        && layoutRectEquals(prePos.viewportRect, position.viewportRect)) {
+      // position doesn't change, do nothing.


aghassemi · 2017-08-10T00:08:21Z

src/service/position-observer/position-observer-impl.js

+
+  /**
+   * This should always be called in vsync.
+   * @param {boolean=} force


this is opt too right?

Yes. force can be used to support fidelity. Viewport resize event for example would require we force update all entries, while viewport scroll doesn't

ok opt_force then?

aghassemi · 2017-08-10T00:12:28Z

src/service/position-observer/position-observer-impl.js

+   * @param {!Element} element
+   */
+  unobserve(element) {
+    for (let i = 0; i < this.entries_.length; i++) {


filterSplice in array.js can do this

Not exactly. filterSplice splice every array item that filter false. Here we can use some optimization to only splice the first item that filter false.
Do you think it is worth to create a new function for this?

I see. Neither the short-circuitingnot not using filterSplice here are a big deal here. Up to you.

Yup, #filterSplice is meant only for multi-removals.

aghassemi · 2017-08-10T00:12:45Z

src/service/position-observer/position-observer-impl.js

+    /** @private {function()} */
+    this.boundStopScroll_ = debounce(this.win_, () => {
+      this.inScroll_ = false;
+    }, 500);


const for 500

jridgewell · 2017-08-10T21:19:31Z

src/service/position-observer/position-observer-entry.js

+      // Need to notify that element gets outside viewport
+      // NOTE: This is required for inabox position observer.
+      this.position = null;
+      position.positionRect = null;


Why are we mutating an object we don't own?

Because we don't want to expose the element position when it gets out of viewport. So set its positionRect to null.

jridgewell · 2017-08-10T21:20:05Z

src/service/position-observer/position-observer-impl.js

+    /** @private {!Window} */
+    this.win_ = win;
+
+    /** @private {!Array<!Object>} */


Update the typedef.

jridgewell · 2017-08-10T21:22:24Z

src/service/position-observer/position-observer-impl.js

+  unobserve(element) {
+    for (let i = 0; i < this.entries_.length; i++) {
+      if (this.entries_[i].element == element) {
+        this.entries_[i].handler = null;


This shouldn't be necessary.

jridgewell · 2017-08-10T21:27:45Z

src/service/position-observer/position-observer-impl.js

+    for (let i = 0; i < this.entries_.length; i++) {
+      const entry = this.entries_[i];
+      if (opt_remeasure) {
+        entry.element.inIframePositionRect = null;


Why does an Element have a #inIframePositionRect ?

😢 I don't like this either. But in the case of inabox, I know an element's relative pos to the inabox window is rarely changed. So I store the value in elemenet['inIframePositionRect'] (need avoid using .) to avoid unnecessary getBoundingClientRect() call. However in the case of inabox window resize I'll need to clear the stored value. That's why I set it back to null here.

I realized it's really not good to store the info to element. I moved it to entry.inInframePositionRect.

jridgewell · 2017-08-10T21:28:59Z

src/service/position-observer/position-observer-impl.js

+        continue;
+      }
+      // Reset entry.turn value.
+      if (!force) {


Nit: join this with the previous if statement.

jridgewell · 2017-08-10T21:34:21Z

src/service/position-observer/position-observer-impl.js

+   * @param {!Event} event
+   * @private
+   */
+  onHostMessageHandler_(event) {


Why does a standard position observer care about this?

Two types of PositionObserver

Top doc level PosOb where the doc viewport is the top level viewport. (not needed)

Iframe PosOb where it requires position message from host doc (need this handler)

I am trying to let the PositionObserver to work in all scenarios, and that's why I have this #onHostMessageHandler_.

jridgewell · 2017-08-10T21:35:50Z

src/service/position-observer/position-observer-impl.js

+    this.viewport_.storeIframePosition(iframePosition);
+
+    // do this in vsyn.measure
+    this.vsync_.measure(() => {


Call #schedulePass_.

jridgewell · 2017-08-10T21:36:02Z

src/service/position-observer/position-observer-impl.js

+   */
+  onResizeHandler_() {
+    this.needRefreshOnMessage_ = true;
+    this.vsync_.measure(() => {


Call #schedulePass_.

But in my current code, #schedulePass_ actively remeasure element position in every animation frame until scroll stops. I can modify the #schedulePass_ function, let me know if you like that better, would become #schedulePass_(isOneTime)

zhouyx · 2017-08-11T23:08:23Z

See #10995. We will need the inabox position api anyway.

Sync with @aghassemi and @lannka offline. Somehow decided to switch from Solution B to solution A.

Solution A

Parent window inform iframe window to start observe position through postMessage (‘start’, ‘end’)
Position Observer in CORS iframe request positions via a series of postMessages (‘send-position’)

Pros:

Can embed different logic to viewport. Ampdoc and Inabox will both have the same logic.
Easy to handle fidelity at the iframe side.
Works well for inabox in non AMP host case. With very straightforward logic in non AMP host.
Cons:

AMP runtime needs to listen to send position request all the time.
Though we send ‘start’ ‘end’ signal to CORS, 3P iframe can request position any time.

Solution B

CORS iframe register the iframe.
Parent window send positions to iframe window at certain fidelity while iframe position is being updated.

Pros:

AMP runtime handles sending positions.
Less abuse of the API.
Cons:

Optimization logic lives in non AMP host. We want to keep it as simple as possible.
Ampdoc and inabox position observer will have different logic.

zhouyx · 2017-08-25T18:28:02Z

extensions/amp-position-observer/0.1/amp-position-observer.js

 } from '../../../src/layout';
 import {
  installPositionObserverServiceForDoc,
+} from '../../../src/service/position-observer/position-observer-impl';


Since the position-observer-impl.js position-observer-entry.js are much cleaner after the cleanup, I am OK if we want to move them to one single file. So that we don't need to import from different files or whitelist multiple files.

Ideas?

zhouyx · 2017-08-25T18:29:39Z

test/manual/amp-animation/scrollbound-embeds.amp.html

    // This code is here for demo purposes and enables the required experiments
    // automatically for this demo so that visitors do not need to enable them manually.
-    var experiments = ['amp-animation', 'amp-position-observer'];
+    var experiments = ['amp-animation', 'amp-position-observer', 'inabox-position-api'];


@aghassemi Is the scrollbound animation already supported. Basically with the new refractor we need to toggle on inabox-position-api to enable position observer in inabox. Let me know if I should remove the experiment flag in this PR

scrollbound is still under an experiment flag but will be removed soon. if you are okay with removing inabox-position-api experiment, go for it. otherwise mention it in #10875 so they get removed at the same time

I'll mention there : )

zhouyx · 2017-08-25T18:31:18Z

I rewrite the PR with #10987 and #11017 in. Ready for review, PTAL : )

aghassemi · 2017-08-25T17:12:27Z

src/service/position-observer/position-observer-entry.js

+ * @typedef {{
+ *  positionRect: ?../../layout-rect.LayoutRectDef,
+ *  viewportRect: !../../layout-rect.LayoutRectDef,
+ *  relativePos: string,


let's make this of type RelativePositions

I added a TODO here. Since relativePos is being used in several files. I would rather make this change a following PR.

aghassemi · 2017-08-25T17:16:11Z

src/service/position-observer/position-observer-entry.js

+   */
+  trigger(position) {
+
+    const prePos = this.prevPosition_ ;


aghassemi · 2017-08-25T17:18:33Z

src/service/position-observer/position-observer-entry.js

+
+    // Add the relative position of the element to its viewport
+    position.relativePos = layoutRectsRelativePos(
+        /** @type {!../../layout-rect.LayoutRectDef} */ (position.positionRect),


if position.positionRect can't be null, do an assert and assign to a constant so type casting happens only once.

aghassemi · 2017-08-25T17:19:56Z

src/service/position-observer/position-observer-entry.js

+      try {
+        this.handler_(position);
+      } catch (err) {
+        // TODO(@zhouyx, #9208) Throw error.


why catch if rethrowing anyway?

hmmm you're right! remove try catch block

aghassemi · 2017-08-25T23:43:30Z

src/service/position-observer/position-observer-impl.js

+  PositionObserverFidelity,
+  LOW_FIDELITY_FRAME_COUNT,
+} from './position-observer-fidelity';
+/** @const @private */


nit: new line above

aghassemi · 2017-08-25T23:47:38Z

src/service/position-observer/position-observer-impl.js

+
+    this.entries_.push(entry);
+
+    if (this.entries_.length == 1) {


this.entries_.length == 1 was confusing to me initially had to think to see why. Can we track through another variable? like callbackStarted_?

aghassemi · 2017-08-25T23:53:10Z

src/service/viewport-impl.js

      const bPromise = this.binding_.getLayoutRectAsync(el, 0, 0);
-      const cPromise = this.binding_.getLayoutRectAsync(
-          frameElement, scrollLeft, scrollTop);
+      const cPromise = this.binding_.getLayoutRectAsync(frameElement);


/cc @dvoytenko for the not passing scroll values anymore as I am not sure what the effects of it would be.

I found if scrollTop/scrollLeft are not passed in, binding will calculate latest scrollTop/scrollLeft. That's what I want.

ok, please ping @dvoytenko for the implications of this before merging

Hmm. This sounds odd. A big change from the original change. Please clarify.

I added two new methods to Viewport class:
#getLayoutRectAsync() and #getBoundingRectAsync(). The idea is that in most cases where an element wants to know its layout rect, #getLayoutRectAsync() should be used instead of #getLayoutRect(). And #getLayoutRectAsync() always return the latest rect in certain animation frame.

To implement this, I added a #getLayoutRectAsync() function to ViewportBindingDef class. Here I am trying to implement Viewport #getLayoutRectAsync() with ViewportBindingDef #getLayoutRectAsync() method. I am not passing scroll values here. Because I want to get the latest layout rect, and don't want to use any cached value here. (I just realized this may not work in ViewportBindingNaturalIosEmbed_ case, I'll fix that)

WDYT?

This should definitely be using the cached values, they should always be correct.

From what I found they are not always correct when we try to calculate rect at a higher rate than browser 'scroll' event. See https://github.com/ampproject/amphtml/blob/master/src/service/viewport-impl.js#L258 that Viewport class use the cached scrollTop_ value when calling #getScrollTop(). But viewport.scrollTop_ is only updated on browser scroll event (throttled already). https://github.com/ampproject/amphtml/blob/master/src/service/viewport-impl.js#L889.

To get the latest layout rect in each animationFrame, I can't use the cached values.

@jridgewell @dvoytenko Maybe we can talk about this offline? My plan to to have Viewport provide functions to get latest rect info at every animationFrame.

aghassemi · 2017-08-25T23:55:39Z

test/manual/amp-animation/scrollbound-embeds.amp.html

    // This code is here for demo purposes and enables the required experiments
    // automatically for this demo so that visitors do not need to enable them manually.
-    var experiments = ['amp-animation', 'amp-position-observer'];
+    var experiments = ['amp-animation', 'amp-position-observer', 'inabox-position-api'];


scrollbound is still under an experiment flag but will be removed soon. if you are okay with removing inabox-position-api experiment, go for it. otherwise mention it in #10875 so they get removed at the same time

zhouyx · 2017-08-26T00:30:11Z

comment addressed

zhouyx · 2017-08-28T17:28:29Z

@jridgewell PTAL : )

jridgewell · 2017-08-28T20:22:44Z

src/service/position-observer/position-observer-impl.js

+    for (let i = 0; i < this.entries_.length; i++) {
+      const entry = this.entries_[i];
+      if (entry.element == element) {
+        entry.fidelity = fidelity;


This if statement should be a method on Entry.

jridgewell · 2017-08-28T20:23:30Z

src/service/position-observer/position-observer-impl.js

+      this.startCallback_();
+    }
+
+    this.callbackStarted_ = true;


Nit: move into if statement.

jridgewell · 2017-08-28T20:24:35Z

src/service/position-observer/position-observer-impl.js

+   * @param {!Element} element
+   */
+  unobserve(element) {
+    for (let i = 0; i < this.entries_.length; i++) {


Yup, #filterSplice is meant only for multi-removals.

jridgewell · 2017-08-28T20:25:50Z

src/service/position-observer/position-observer-impl.js

+  */
+  updateAllEntries(opt_force) {
+    for (let i = 0; i < this.entries_.length; i++) {
+      const entry = this.entries_[i];


This body statement should be a method on Entry.

Still belongs as a method on Entry.

jridgewell · 2017-08-28T20:30:45Z

src/service/position-observer/position-observer-impl.js

+    // P2: do passes on onDomMutation (if available using MutationObserver
+    // mostly for in-a-box host, since most DOM mutations are constraint to the
+    // AMP elements).
+    this.updateAllEntries();


Shouldn't this wait for a measure?

#updateAllEntries() call #updateSingleEntries() then call #Viewport.getBoundingRectAsync(). which returns rect in next vsync measure.

jridgewell · 2017-08-28T20:31:36Z

src/service/position-observer/position-observer-impl.js

+    if (!this.inScroll_) {
+      // Stop measure if viewport is no longer scrolling
+      this.measure_ = false;
+      return;


The positions could still change by this time (without firing a new scroll event). We should probably remeasure again, but not issue another vsync.

But I call updateAllEntries() before checking this.inScroll_. I think that address your comment here.

Lol, ignore me.

jridgewell · 2017-08-28T20:32:23Z

src/service/viewport-impl.js

+  }
+
+  getRectAsync() {
+    return this.vsync_.measurePromise(() => {


This should return a memoized promise until the vsync executes (it'll save a lot of memory).

jridgewell · 2017-08-28T20:33:06Z

src/service/viewport-impl.js

      const bPromise = this.binding_.getLayoutRectAsync(el, 0, 0);
-      const cPromise = this.binding_.getLayoutRectAsync(
-          frameElement, scrollLeft, scrollTop);
+      const cPromise = this.binding_.getLayoutRectAsync(frameElement);


This should definitely be using the cached values, they should always be correct.

zhouyx · 2017-09-05T21:47:21Z

I think #11148 unblock the work here. Changed the PR. PTAL : )

jridgewell · 2017-09-06T18:55:59Z

src/service/position-observer/position-observer-entry.js

+   * @param {!PositionInViewportEntryDef} position
+   */
+  trigger(position) {
+


Nit: no newline.

jridgewell · 2017-09-06T18:56:25Z

src/service/position-observer/position-observer-entry.js

+      return;
+    }
+
+    dev().assert(position.positionRect);


Nit: add a message.

jridgewell · 2017-09-06T18:57:48Z

src/service/position-observer/position-observer-entry.js

+
+    dev().assert(position.positionRect);
+    const positionRect =
+        /** @type {!../../layout-rect.LayoutRectDef} */ (position.positionRect);


This would be cleaner if the position we passed to trigger weren't nullable, and the one we pass to handler was. Or, we just pass two objects to the handler.

position is always NOT nullable. position.postionRect is be nullable.
The problem is I want to pass position.positionRect = null to handler for the first time. After that I don't want to invoke handler with another positionRect null.

position.postionRect is be nullable.

That's what I meant.

The problem is I want to pass position.positionRect = null to handler for the first time.

Don't you mean when there's no intersection? It can still happen that way by using a second type, or passing two arguments to the handler function.

emmm. I sorry I kind of get lost here.
So what you want is have position.positionRect NOT nullable when passed to #trigger(), but have it nullable when passed to #handler()? This is what I have now.

Yes I can definitely change the arguments to the handler function. But now handler function expects single object, and I think our code is already depending on that return type. Dima proposed some more general object type that we might migrate to. But that's following work then.

jridgewell · 2017-09-06T18:59:53Z

src/service/position-observer/position-observer-impl.js

+        this.entries_.splice(i, 1);
+        if (this.entries_.length == 0) {
+          this.stopCallback_();
+          this.callbackStarted_ = false;


Nit: this can be moved into start and stop callbacks.

good idea! done

jridgewell · 2017-09-06T18:59:59Z

src/service/position-observer/position-observer-impl.js

+
+    if (!this.callbackStarted_) {
+      this.startCallback_();
+      this.callbackStarted_ = true;


Nit: this can be moved into start and stop callbacks.

jridgewell · 2017-09-06T19:01:19Z

src/service/position-observer/position-observer-impl.js

+  */
+  updateAllEntries(opt_force) {
+    for (let i = 0; i < this.entries_.length; i++) {
+      const entry = this.entries_[i];


Still belongs as a method on Entry.

jridgewell · 2017-09-06T19:02:31Z

src/service/position-observer/position-observer-entry.js

+    const positionRect =
+        /** @type {!../../layout-rect.LayoutRectDef} */ (position.positionRect);
+    // Add the relative position of the element to its viewport
+    position.relativePos = layoutRectsRelativePos(positionRect,


Why is this being calculated here?

Hmmm that's how we define (at least currently) the return value of PositionObserverCallback

{ viewportRect, positionRect, relativePos, }

Because we will be setting positionRect to null when element gets out of viewport, we need to calculate its relativePosition to viewport (above/below) here, handler won't have enough data to calculate it.

jridgewell · 2017-09-07T18:58:18Z

src/service/position-observer/position-observer-impl.js

    for (let i = 0; i < this.entries_.length; i++) {
      const entry = this.entries_[i];

      if (opt_force) {


These two if statements belong in Entry#update as well.

jridgewell · 2017-09-07T19:00:02Z

src/service/position-observer/position-observer-entry.js

+
+    dev().assert(position.positionRect);
+    const positionRect =
+        /** @type {!../../layout-rect.LayoutRectDef} */ (position.positionRect);


position.postionRect is be nullable.

That's what I meant.

The problem is I want to pass position.positionRect = null to handler for the first time.

Don't you mean when there's no intersection? It can still happen that way by using a second type, or passing two arguments to the handler function.

jridgewell · 2017-09-07T22:15:46Z

src/service/position-observer/position-observer-impl.js

-  LOW_FIDELITY_FRAME_COUNT,
-} from './position-observer-fidelity';
+  PositionObserverEntry,
+  /* eslint no-unused-vars: 0 */ PositionObserverFidelity,


Nit: Appending Def to the name will skip the lint error.

but the value is an enum not a Def 😕 Should i do that.

Oh, this is just being used as a typedef. Just use the ./position-observer-entry.PositionObserverFidelity types instead.

That's the first thing I try.
Get

Bad type annotation. Unknown type module$src$service$position_observer$position_observer_entry.PositionObserverFidelity

Strange that ./position-observer-entry.PositionInViewportEntryDef works but ./position-observer-entry.PositionObserverFidelity doesn't.

That's likely during the extension compiling. What's the full output?

src/service/position-observer/position-observer-impl.js:73: ERROR - Bad type annotation. Unknown type module$src$service$position_observer$position_observer_entry.PositionObserverFidelity * @param {!./position-observer-entry.PositionObserverFidelity} fidelity

get this running gulp check-types

jridgewell · 2017-09-07T22:21:16Z

src/service/position-observer/position-observer-entry.js

+      this.turn--;
+      return;
+    }
+    this.turn = (this.fidelity == PositionObserverFidelity.LOW) ?


This is slightly different than the old code. Now, when a force update comes in, all low fidelity Entries will be synchronized to the same frame turn. Using two nested if statements cleans it up:

if (!opt_force) { if (this.turn > 0) { this.turn--; return; } // An if statement just seems nicer than the assignment conditional if (this.fidelity == LOW) { this.turn = 4; } }

Good point. Talked offline, will move back to old behavior. But we may need future discussion on whether we should randomize entry.turn or sync all low fidelity elements' measurement

lannka · 2017-09-08T18:28:47Z

src/service/position-observer/position-observer-entry.js

+ */
+export let PositionInViewportEntryDef;
+
+export class PositionObserverEntry {


-> xxxWorker

lannka · 2017-09-08T18:29:38Z

src/service/position-observer/position-observer-impl.js

+   * @param {!Element} element
+   * @param {PositionObserverFidelity} fidelity
+   */
+  changeFidelity(element, fidelity) {


can we remove?

zhouyx · 2017-09-08T21:12:57Z

fix #8755

zhouyx requested review from aghassemi and lannka August 5, 2017 01:06

googlebot added the cla: yes label Aug 5, 2017

zhouyx force-pushed the non-AMP-host-posob branch 2 times, most recently from adae0fa to f636fd1 Compare August 7, 2017 22:56

zhouyx requested a review from jridgewell August 8, 2017 18:57

lannka added the PR use: Work in Progress label Aug 9, 2017

zhouyx force-pushed the non-AMP-host-posob branch from c81b1bd to bea519d Compare August 9, 2017 20:37

zhouyx removed the PR use: Work in Progress label Aug 9, 2017

aghassemi suggested changes Aug 10, 2017

View reviewed changes

jridgewell requested changes Aug 10, 2017

View reviewed changes

zhouyx force-pushed the non-AMP-host-posob branch from ebb29ab to 9272904 Compare August 11, 2017 17:45

zhouyx force-pushed the non-AMP-host-posob branch from 9272904 to 7314b7e Compare August 25, 2017 16:02

zhouyx commented Aug 25, 2017

View reviewed changes

aghassemi suggested changes Aug 25, 2017

View reviewed changes

aghassemi approved these changes Aug 27, 2017

View reviewed changes

jridgewell requested changes Aug 28, 2017

View reviewed changes

zhouyx mentioned this pull request Aug 31, 2017

Introduce #Viewport.getClientRectAsync() function #11148

Merged

zhouyx force-pushed the non-AMP-host-posob branch from 4c32542 to 1134c19 Compare September 5, 2017 21:01

zhouyx added 5 commits September 5, 2017 14:30

rewrite positionobserver for better sharing

1523b71

add more test

6b6d91a

to multi files

dae905d

fix-dep-check

68e37cd

address comment step1

02b5f0f

fix test

8899bc9

zhouyx force-pushed the non-AMP-host-posob branch from 1134c19 to 8899bc9 Compare September 5, 2017 21:32

jridgewell requested changes Sep 6, 2017

View reviewed changes

address comment

c0e02c5

jridgewell reviewed Sep 7, 2017

View reviewed changes

move force_update logic to entry

c8e3f02

jridgewell approved these changes Sep 7, 2017

View reviewed changes

remove position-observer-fidelity

7464bce

lannka approved these changes Sep 8, 2017

View reviewed changes

zhouyx added 2 commits September 8, 2017 13:17

remove unused changeFidelity method

acf1917

rename

5a4370e

zhouyx force-pushed the non-AMP-host-posob branch from bb18853 to 5a4370e Compare September 8, 2017 20:40

zhouyx merged commit a832f26 into ampproject:master Sep 8, 2017

zhouyx deleted the non-AMP-host-posob branch September 8, 2017 21:10

lannka mentioned this pull request Sep 8, 2017

Scroll position is not reported to ads embedded using inabox #8740

Closed

zhouyx mentioned this pull request Sep 16, 2017

Position Observer Improvement #9208

Closed

6 tasks

ampprojectbot mentioned this pull request Apr 16, 2020

🚨 Error: Action execution failed: AMP-ANIMATION start: g.cancel is not a function. (In 'g.cancel()', 'g.cancel' is undefined) #27802

Closed

ampprojectbot mentioned this pull request Jun 1, 2020

🚨 Error: Cannot read property 'getAttribute' of undefined #28645

Closed

ampprojectbot mentioned this pull request Jul 3, 2020

🚨 Error: failed to attach shadow document for page: Attempting to change value of a readonly property. #29134

Closed

This was referenced Mar 16, 2021

🚨 Error: Cannot read property 'querySelectorAll' of undefined ampproject/error-reporting#69

Open

🚨 Error: undefined is not an object (evaluating 'c.ampdoc') #33312

Closed

ampprojectbot mentioned this pull request Apr 2, 2021

🚨 Error: undefined is not an object (evaluating 'c.ampdoc') #33606

Closed

This was referenced May 22, 2021

🚨 Error: undefined is not an object (evaluating 'a.querySelectorAll') #34487

Closed

🚨 Error: undefined is not an object (evaluating 'a.querySelectorAll') #34488

Closed

ampprojectbot mentioned this pull request Aug 28, 2021

🚨 Error: history trace for replace: SecurityError: Attempt to use history.replaceState() more than 100 times per 30 seconds #35852

Closed

ampprojectbot mentioned this pull request Sep 20, 2021

🚨 Error: Maximum call stack size exceeded #36111

Closed

ampprojectbot mentioned this pull request Oct 21, 2021

🚨 Error: undefined is not an object (evaluating 'a.document.createEvent') ampproject/error-reporting#110

Open

ampprojectbot mentioned this pull request Jan 18, 2022

🚨 Error: undefined is not an object (evaluating 't.querySelectorAll') ampproject/error-reporting#130

Open

Rewrite PositionObserver for better sharing #10813

Rewrite PositionObserver for better sharing #10813

Uh oh!

Conversation

zhouyx commented Aug 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouyx commented Aug 9, 2017

Uh oh!

aghassemi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

zhouyx commented Aug 5, 2017 •

edited

Loading

zhouyx Aug 10, 2017 •

edited

Loading

zhouyx commented Aug 11, 2017 •

edited

Loading

zhouyx Aug 26, 2017 •

edited

Loading