Fix amp-sidebar local navigation and history #5520

mkhatib · 2016-10-11T03:03:33Z

Debugging and experimenting with amp-sidebar bug #4184

On 2nd tab
Open Sidebar
Click Section 1 link
- Notice it would close the tab and scroll correctly to the section.
Scroll back up and open sidebar again
Click Section 2 link
- Notice it would close the tab and scroll correctly to the section.
Scroll back up and open sidebar again
Click Section 1 link
- Notice it would close the tab and scroll correctly to the section.
Hit Back button
- Notice it should scroll to Section 2.
Hit Back button Again
- Notice it should scroll to Section 1.

dvoytenko · 2016-10-11T17:06:38Z

examples/viewer.html

+      if (this.visibilityState_ != 'visible') {
+        return;
+      }
+      console.log('popping: ', e.state);


dvoytenko · 2016-10-11T17:08:46Z

extensions/amp-sidebar/0.1/amp-sidebar.js

@@ -103,6 +107,19 @@ export class AmpSidebar extends AMP.BaseElement {
    this.registerAction('toggle', this.toggle_.bind(this));
    this.registerAction('open', this.open_.bind(this));
    this.registerAction('close', this.close_.bind(this));
+
+    installGlobalClickListenerForDoc(this.getAmpDoc()).addBeforeHandler(e => {


If we do something like this, I'd recommend we instead expose onNavigate(href, target, isLocal) method or something so that we don't have to look for all the same A and its parameters. Alternatively, I don't see an issue to just add a click listener directly on the sidebar itself. Why do we need to listen for global clicks anyway?

Why do we need to listen for global clicks anyway?

This is basically to make sure we run before we actually handle the click (which is handled inside document-click handler), and the event is captured there and from testing capturing seems to be first-capture-first-serve and since document-click listener is added on document load it's always going to be the first handle to be called.

In this case we want our handler inside sidebar to be called before it to allow us to pop before pushing.

I don't believe document-click is a capture. It'd seem to be illogical to have it as a capture click, since it's a classical bubbling processing.

Just checked you're right it's not capture. I could swear it was 😅 when I checked

"Schrodinger's capture" (c)

dvoytenko · 2016-10-11T17:09:29Z

src/document-click.js

@@ -37,7 +37,7 @@ const ORIGINAL_HREF_ATTRIBUTE = 'data-amp-orig-href';
 * @param {!./service/ampdoc-impl.AmpDoc} ampdoc
 */
 export function installGlobalClickListenerForDoc(ampdoc) {
-  fromClassForDoc(ampdoc, 'clickhandler', ClickHandler);
+  return fromClassForDoc(ampdoc, 'clickhandler', ClickHandler);


Also add @return

dvoytenko · 2016-10-11T17:09:46Z

src/document-click.js

@@ -73,6 +73,9 @@ export class ClickHandler {
      this.boundHandle_ = this.handle_.bind(this);
      this.ampdoc.getRootNode().addEventListener('click', this.boundHandle_);
    }
+
+    /** @private {!Array<!function(!Event)>} */
+    this.beforeHandlers_ = [];


We have observables for this.

dvoytenko · 2016-10-11T17:10:21Z

extensions/amp-sidebar/0.1/amp-sidebar.js

@@ -103,6 +107,19 @@ export class AmpSidebar extends AMP.BaseElement {
    this.registerAction('toggle', this.toggle_.bind(this));
    this.registerAction('open', this.open_.bind(this));
    this.registerAction('close', this.close_.bind(this));
+
+    installGlobalClickListenerForDoc(this.getAmpDoc()).addBeforeHandler(e => {


Where is the listener removed? We don't need it working all the time, right? Only when the sidebar is open, I assume.

Ah good point. Will update once other parts are figured out.

dvoytenko · 2016-10-11T17:10:33Z

src/document-click.js

@@ -91,9 +94,20 @@ export class ClickHandler {
   * @param {!Event} e
   */
  handle_(e) {
+    for (let i = 0; i < this.beforeHandlers_.length; i++) {


ditto: observable

dvoytenko · 2016-10-11T17:11:05Z

src/document-click.js

@@ -286,10 +300,12 @@ export function onDocumentElementClick_(
  }

  if (tgtLoc.hash != curLoc.hash) {
+    timerFor(win).delay(() => {


What is "wait" for?

Explained in the above discussion.

dvoytenko · 2016-10-11T17:13:07Z

extensions/amp-sidebar/0.1/amp-sidebar.js

@@ -160,7 +177,9 @@ export class AmpSidebar extends AMP.BaseElement {
        }, ANIMATION_TIMEOUT);
      });
    });
-    this.getHistory_().push(this.close_.bind(this)).then(historyId => {
+    this.getHistory_().push(() => {
+      this.close_();


I didn't understand the issue with event sequence. What's the exact event sequence in this case?

So without the doing the delay when pushing the new state in document-submit the sequence goes like this:

Open Sidebar

Click Section 1 Link

We Close Sidebar

Which also Pops History at 1

And then handle the actual click on the link and Push history to 1

And then Popping actually happen and history is popped to 0 removing the new push with it.

I am not entirely sure yet why does this happen. But this might be dependent on when the viewer is updating their stack index and communicate the change back with the amp doc. In viewer.html for example we do that inside the popstate event handler. This is were my theory about the event queue delaying the stack index update from happening right away after the close-pop.

(I know the viewer.html implementation is incomplete but my guess is the Google.com viewer is doing something similar, I've also tested with the sandbox viewer and seen the same experience)

So to summarize this is what is being logged in sequence when the timeout is not in place.

[VIEWER] push history to 1 [VIEWER] pop history at 1 [VIEWER] push history to 1 [VIEWER] history popped to 0 # Notice our history push is gone now!

After adding the timeout this gives time for popstate handlers to update the stack indexes. These are the logs after timeout:

[VIEWER] push history to 1 [VIEWER] pop history at 1 [VIEWER] history popped to 0 [VIEWER] push history to 1 # Yay pushed state preserved!

Does this make any sense 😄 because I am not 80% share!

So event processing loop can possibly interfere here. In this case, though, waiting for one event loop (e.g. setTimeout(..., 0)) should be enough. Can you try that in your experiments. We just need this info to make a better decision.

Generally, I'd say, we don't have here enough information to make the right calls. We need more research and understand the exact sequence of events much better. If we need more data from sandbox - we should find a way to get it. One important thing: our history supports queuing and returns promises for push and pop. In theory, these promises should be sufficient to address any risk conditions. For this purpose I'm a little skeptical that we need timers.

During myinitiall experiments I couldn't fix with a small timeout. I didn't investigate this enough bit wanted to see if you might have an idea why that might be happening.

I'll do some more work on this and see if I can use the promises for this.

@dvoytenko I've done some more investigation, so basically this is my understanding of what's happening. The call to history.pop and history.push happens right after each other. The viewer then receives a message for each, causing it to execute first popState and pushState callbacks right after each other.

At this point the viewer hasn't actually popped the state nor did it update the stackIndex because that happens in statepopped event handler onPopState_, this is also where the viewer communicate back to the ampdoc the new stackIndex with the historyPopped message.

I've just sent out a new change to the PR where I basically wait for any popping to be confirmed before any pushing happens.

My understanding is that we might be already doing something very similar in the natural implementation of history but I haven't looked into that a lot.

This fixes the problem, basically it waits to make sure the stack index is reflected.

What do you think?

That makes sense. To confirm, you'd wait for viewer to respond as with "push is done"?

More of the "pop is done" - I don't think the same problem happens with pushing since there's no pushstate event.

mkhatib · 2016-10-12T03:41:07Z

cc/ @aghassemi and @camelburrito this is the PR we were discussing today.

googlebot · 2016-10-14T00:07:18Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

…o history

googlebot · 2016-10-14T00:11:51Z

CLAs look good, thanks!

mkhatib · 2016-10-18T05:11:47Z

@dvoytenko I cleaned up the PR PTAL 👀 - if this looks good I'll add tests. I'll also test with the sandbox viewer.

dvoytenko · 2016-10-24T18:23:30Z

@mkhatib Is this ready to be reviewed again?

mkhatib · 2016-10-24T18:39:51Z

@dvoytenko yes, I'll work on tests

jridgewell

I don't understand this PR at all. Your description's use case works as it's written, and that feels like the correct behavior to me.

jridgewell · 2016-10-24T19:33:28Z

extensions/amp-sidebar/0.1/amp-sidebar.js

+
+    this.element.addEventListener('click', e => {
+      const target = closestByTag(dev().assertElement(e.target), 'A');
+      if (!target) {


Check that target has a href, too. Also, this code's duplicated.

jridgewell · 2016-10-24T19:35:53Z

extensions/amp-sidebar/0.1/amp-sidebar.js

-    screenReaderCloseButton.addEventListener('click', () => {
-      this.close_();
-    });
+    screenReaderCloseButton.addEventListener('click', () => this.close_());


It seems like we should create a boundClose, since it's used so frequently.

jridgewell · 2016-10-24T19:43:41Z

src/service/history-impl.js

+          // Current implementation doesn't wait for response from viewer.
+          this.updateStackIndex_(this.stackIndex_ + 1);
+          this.viewer_.postPushHistory(this.stackIndex_);
+          return Promise.resolve(this.stackIndex_);


Just return #stackIndex.

dvoytenko · 2016-10-24T19:49:31Z

src/service/history-impl.js

-  constructor(viewer) {
+  constructor(viewer, win) {
+    /** @private @const {!../service/timer-impl.Timer} */
+    this.timer_ = timerFor(win);


You can just do viewer.ampdoc.win. No need to pass win as an arg here.

dvoytenko · 2016-10-24T19:54:42Z

src/service/history-impl.js

+    // between popping and pushing might happen causing history to be messed
+    // up. This will only wait 100ms to avoid getting stuck if confirmation
+    // never arrived.
+    return Promise.race([this.timer_.promise(100), this.awaitPoppingPromise_])


Why awaitPoppingPromise_ necessary? Isn't queuing feature already provided by the parent History class? It'd seem all we need to do is return a promise from viewer's postPushHistory call, which may already be the case.

mkhatib · 2016-11-10T19:58:33Z

Closing this one for #5961

mkhatib mentioned this pull request Oct 11, 2016

<amp-sidebar> is scrolling back to the page top on local navigation #4184

Closed

dvoytenko suggested changes Oct 11, 2016

View reviewed changes

mkhatib added 3 commits October 13, 2016 17:11

Fix amp-sidebar local navigation and history.

43828ce

Wait for the popping confirmation to arrive before pushing anything t…

27f10e9

…o history

fix type

e2a6a27

mkhatib force-pushed the sidebar-links branch from baa9b4c to e2a6a27 Compare October 14, 2016 00:11

updates

22496a9

mkhatib force-pushed the sidebar-links branch from 22fc2bf to 22496a9 Compare October 18, 2016 05:09

mkhatib assigned dvoytenko Oct 20, 2016

mkhatib added the NEEDS REVIEW label Oct 20, 2016

mkhatib changed the title ~~DO NOT SUBMIT: Fix amp-sidebar local navigation and history~~ Fix amp-sidebar local navigation and history Oct 20, 2016

jridgewell reviewed Oct 24, 2016

View reviewed changes

dvoytenko suggested changes Oct 24, 2016

View reviewed changes

mkhatib closed this Nov 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix amp-sidebar local navigation and history #5520

Fix amp-sidebar local navigation and history #5520

mkhatib commented Oct 11, 2016 •

edited

dvoytenko Oct 11, 2016

dvoytenko Oct 11, 2016 •

edited

mkhatib Oct 11, 2016

dvoytenko Oct 12, 2016

mkhatib Oct 12, 2016

dvoytenko Oct 12, 2016

dvoytenko Oct 11, 2016

dvoytenko Oct 11, 2016

dvoytenko Oct 11, 2016

mkhatib Oct 11, 2016

dvoytenko Oct 11, 2016

dvoytenko Oct 11, 2016

mkhatib Oct 11, 2016

dvoytenko Oct 11, 2016

mkhatib Oct 11, 2016

dvoytenko Oct 12, 2016

mkhatib Oct 12, 2016

mkhatib Oct 14, 2016

dvoytenko Oct 18, 2016

mkhatib Oct 18, 2016

mkhatib commented Oct 12, 2016

googlebot commented Oct 14, 2016

googlebot commented Oct 14, 2016

mkhatib commented Oct 18, 2016

dvoytenko commented Oct 24, 2016

mkhatib commented Oct 24, 2016

jridgewell left a comment

jridgewell Oct 24, 2016

jridgewell Oct 24, 2016

jridgewell Oct 24, 2016

dvoytenko Oct 24, 2016

dvoytenko Oct 24, 2016

mkhatib commented Nov 10, 2016

Fix amp-sidebar local navigation and history #5520

Fix amp-sidebar local navigation and history #5520

Conversation

mkhatib commented Oct 11, 2016 • edited

Choose a reason for hiding this comment

dvoytenko Oct 11, 2016 • edited

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

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

mkhatib commented Oct 12, 2016

googlebot commented Oct 14, 2016

googlebot commented Oct 14, 2016

mkhatib commented Oct 18, 2016

dvoytenko commented Oct 24, 2016

mkhatib commented Oct 24, 2016

jridgewell left a 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

mkhatib commented Nov 10, 2016

mkhatib commented Oct 11, 2016 •

edited

dvoytenko Oct 11, 2016 •

edited