🐛Ensure that goToPage actions and fragment Urls work in sidebar for stories #20944

bramanudom · 2019-02-19T22:46:11Z

#20083

This PR makes the necessary changes to make both goToPage actions and fragments work when used with amp-sidebar in amp-stories, which helps make branching use-cases like table of contents possible.

Previously, clicking on elements with the goToPage action triggers a page change but does not close the sidebar or the sidebar mask. Fragment Urls should be supported as they match with conventions surrounding making ToC in general.

Link to example

Open sidebar, click on either links to navigate to the second page.
Elements with a goToPage actions in amp-story-page should still work

Tests have not yet been written; there will be a follow-up PR with e2e tests.

newmuis · 2019-02-19T23:28:40Z

extensions/amp-story/1.0/amp-story.js

+            this.switchTo_(args['id'], NavigationDirection.NEXT);
+            this.closeOpacityMask_();
+          }
+        } this.switchTo_(args['id'], NavigationDirection.NEXT);


nit: Can you put this on its own line, instead of on the same line as the brace?

newmuis · 2019-02-19T23:30:02Z

extensions/amp-story/1.0/amp-story.js

@@ -2216,6 +2239,24 @@ export class AmpStory extends AMP.BaseElement {
    if (!this.sidebar_) {
      return;
    }
+    if (isExperimentOn(this.win,'amp-story-branching')) {
+      this.sidebar_.addEventListener('click', e => {
+        if (e.target.tagName === 'A') {


I wonder: is this necessarily sufficient to know that it was a fragment link?

Ahh, you're right. I added in a check to make sure that the link contains "#page=" and I think that should be sufficient to make sure that the following steps don't apply to outlinks.

I updated the example linked in the PR description to reflect this change.

gmajoulet · 2019-02-20T18:38:58Z

extensions/amp-story/1.0/amp-story.js

+
+          if (caller['offsetParent'].tagName === 'AMP-SIDEBAR') {
+            this.sidebar_.getImpl()
+                .then(sidebarImpl => sidebarImpl.close_());


We can't call a private method, we'd have to make this one public. :(

gmajoulet · 2019-02-20T18:43:39Z

extensions/amp-story/1.0/amp-story.js

+          if (caller['offsetParent'].tagName === 'AMP-SIDEBAR') {
+            this.sidebar_.getImpl()
+                .then(sidebarImpl => sidebarImpl.close_());
+            this.switchTo_(args['id'], NavigationDirection.NEXT);


This is introducing a bug. Let's imagine this story has a third page:

Load the story, you're on the cover page

Open the sidebar and click on a link or action to go to page 2

You're on page2, you click next, you're on page 3

Navigate back

Expected: now on page2 / Actual: now on page1

(I haven't tried this exact scenario, but from the history state bug this should happen)

When the sidebar opens, it pushes a new history entry. Because you're using a promise to get the sidebar implementation, the sidebarImpl.close() will happen after the switchTo.
switchTo writes the history state in the entry added by the sidebar, that is then popped by sidebarImpl.close(). So the navigation path actually never got updated, resulting in the navigation bug described above.

An easy fix would be to call switchTo after the sidebar's history entry is popped.

Synched offline but you're right that there was a historyState bug, but it actually corrects itself as you navigate forwards.

With the changes I just pushed, when you navigate using a sidebar link (not action), checking the ampStoryNavigationPath will yield an undefined state, but that also corrects itself when you either navigates forwards or backwards (this is due to manipulating the history state to not show the fragment as the user is navigating via the sidebar links/actions).

gmajoulet · 2019-02-20T18:53:22Z

extensions/amp-story/1.0/amp-story.js

+          }
+        }
+      });
+    }


A few things about this block:

If the link is meant to navigate to another story, this will fail. ie: sidebar in story.com/1 links to story.com/2#page=foo would not navigate to story.com/2

This will fail if there is any element within the <a> tag. ie: <a href="#page=foo">Foo</a> will work, but <a href="#page=foo"><span>Foo</span></a> won't

Does this piece of code have the same history state issue described above?

Could we do all that from the hashchange handler?

We took this offline a little, but done! Utilized hashchange handler for fragments, but not for goToPage. I also added a check for the pathname, to handler for href values that look like #page=somePage or story.com/2#page=somePage if the story was on story.com/2

googlebot · 2019-03-22T16:33:03Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-22T19:51:27Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

bramanudom · 2019-03-25T19:30:06Z

I think this is ready for review! @gmajoulet @newmuis @cvializ

newmuis · 2019-03-25T19:42:37Z

examples/visual-tests/amp-story/amp-story-sidebar.html

@@ -29,18 +29,19 @@
  </head>

  <body>
-    <amp-story standalone publisher="AMP Team" title="Visual Diff Test"
+    <amp-story id = "story" standalone publisher="AMP Team" title="Visual Diff Test"


nit: remove spaces around the = sign

newmuis · 2019-03-25T19:52:55Z

examples/visual-tests/amp-story/amp-story-sidebar.html

-          <li>Nav item 3</li>
-          <li><a href="#idFour" on="tap:idFour.scrollTo">Nav item 4</a></li>
+          <li><span><a href="http://google.com">Out item 1</a></span></li>
+          <li><span><a href="http://localhost:8000/examples/visual-tests/amp-story/amp-story-sidebar.html#page=page-2">Absolute fragment link</a></span></li>


These examples are intended to work regardless of the server. This will break if someone changes the port, or serves their examples off of, for example, Firebase.

Ahh, yes. Thank you for pointing this out! Done.

newmuis · 2019-03-25T19:54:05Z

extensions/amp-story/1.0/amp-story.js

        if (args) {
          this.storeService_.dispatch(
              Action.SET_ADVANCEMENT_MODE, AdvancementMode.GO_TO_PAGE);
-          this.switchTo_(args['id'], NavigationDirection.NEXT);
+
+          if (caller['offsetParent'].tagName === 'AMP-SIDEBAR') {


Is it always exactly the parent? Or are we really expecting it to be any ancestor?

Can we use caller.offsetParent instead of caller['offsetParent']?

Changed the code to use closestAncestorElementBySelector since it seems like what we care about is that at some point the callers ancestor is an amp-sidebar.

newmuis · 2019-03-25T19:55:09Z

extensions/amp-story/1.0/amp-story.js

+          // Remove the fragment parameter from the URL
+          const {history} = this.win;
+          history.pushState(
+              '', this.win.document.title, this.win.location.pathname);


Can you add a comment to document the first parameter?

Done, and for the other args as well.

mrjoro · 2019-05-15T20:44:27Z

Is this PR still active?

newmuis · 2019-05-21T10:54:19Z

Yes, it would be great for us to get this merged. I'll see if I have some time next week; I think it's mostly done, but we just need to rebase and manually test.

bramanudom · 2019-05-21T16:42:09Z

Hello, I can go ahead and rebase today. I'll also manually test but I'll leave it to @newmuis and the team to officially verify.

newmuis · 2019-05-22T10:08:52Z

extensions/amp-story/1.0/amp-story.js

@@ -444,6 +444,7 @@ export class AmpStory extends AMP.BaseElement {

        if (args) {
          this.storeService_.dispatch(
+<<<<<<< HEAD


Looks like there are still change markers here

Hmm, I think that I already got rid of this in this commit.

I double checked those lines in the Files Changed tab in this PR as well.

mrjoro · 2019-08-08T18:09:17Z

@newmuis is this a candidate for a fixit fix? :)

/cc @ampproject/wg-stories

gmajoulet · 2019-08-23T22:03:13Z

I rebased Pet's code and pushed it to a new branch here, I'll double check that everything still works and send a PR next week. :)

gmajoulet · 2019-10-08T20:56:10Z

Closed in favor of #24968

bramanudom requested review from gmajoulet, newmuis and cvializ February 19, 2019 22:46

googlebot added the cla: yes label Feb 19, 2019

newmuis reviewed Feb 19, 2019

View reviewed changes

bramanudom force-pushed the sidebar_links branch 2 times, most recently from ca62b59 to f821798 Compare February 20, 2019 00:09

gmajoulet reviewed Feb 20, 2019

View reviewed changes

bramanudom force-pushed the sidebar_links branch from 89184d5 to b8cdbfd Compare March 22, 2019 16:30

googlebot added cla: no and removed cla: yes labels Mar 22, 2019

bramanudom added 8 commits March 22, 2019 14:00

fixed merge conflicts

ee49a21

make sure outlinks still work

02cd367

fixed merge conflicts

1d7fb9c

fixed typecheck error

8dc93d5

fixed typecheck error

3b149cc

update story sidebar visual example, revert extra added line

d33f448

fixed merge conflicts

74dacc3

fixed merge conflicts

ffa60db

bramanudom force-pushed the sidebar_links branch from b8cdbfd to ffa60db Compare March 22, 2019 19:51

googlebot added cla: yes and removed cla: no labels Mar 22, 2019

bramanudom added 4 commits March 22, 2019 15:53

delete extra testing file

8f935ee

re-add missing commits from cherry-pick

91a3f4d

Merge branch 'master' into sidebar_links

47978de

revert changes for page change action, formatting for visual test

efddc95

newmuis reviewed Mar 25, 2019

View reviewed changes

bramanudom added 3 commits March 26, 2019 13:53

addressed PR changes

1e53480

suppress type check error

587e05f

Merge branch 'master' into sidebar_links

c691147

bramanudom added 3 commits May 22, 2019 00:52

merge and fix conflicts

7ec27b4

merge and fix conflicts

93d0a06

merge and fix conflicts

7b91cb4

newmuis reviewed May 22, 2019

View reviewed changes

gmajoulet mentioned this pull request Oct 8, 2019

Story branching support for sidebar links. #24968

Merged

gmajoulet closed this Oct 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛Ensure that goToPage actions and fragment Urls work in sidebar for stories #20944

🐛Ensure that goToPage actions and fragment Urls work in sidebar for stories #20944

bramanudom commented Feb 19, 2019

newmuis Feb 19, 2019

bramanudom Feb 20, 2019

newmuis Feb 19, 2019

bramanudom Feb 20, 2019

gmajoulet Feb 20, 2019

gmajoulet Feb 20, 2019

bramanudom Mar 21, 2019

gmajoulet Feb 20, 2019

bramanudom Mar 21, 2019

googlebot commented Mar 22, 2019

googlebot commented Mar 22, 2019

bramanudom commented Mar 25, 2019

newmuis Mar 25, 2019

bramanudom Mar 26, 2019

newmuis Mar 25, 2019

bramanudom Mar 26, 2019

newmuis Mar 25, 2019

bramanudom Mar 26, 2019

newmuis Mar 25, 2019

bramanudom Mar 26, 2019

mrjoro commented May 15, 2019

newmuis commented May 21, 2019

bramanudom commented May 21, 2019 •

edited

newmuis May 22, 2019

bramanudom May 22, 2019

mrjoro commented Aug 8, 2019

gmajoulet commented Aug 23, 2019

gmajoulet commented Oct 8, 2019

🐛Ensure that goToPage actions and fragment Urls work in sidebar for stories #20944

🐛Ensure that goToPage actions and fragment Urls work in sidebar for stories #20944

Conversation

bramanudom commented Feb 19, 2019

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

googlebot commented Mar 22, 2019

googlebot commented Mar 22, 2019

bramanudom commented Mar 25, 2019

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

mrjoro commented May 15, 2019

newmuis commented May 21, 2019

bramanudom commented May 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrjoro commented Aug 8, 2019

gmajoulet commented Aug 23, 2019

gmajoulet commented Oct 8, 2019

bramanudom commented May 21, 2019 •

edited