Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 18 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ee49a21
fixed merge conflicts
bramanudom Feb 19, 2019
02cd367
make sure outlinks still work
bramanudom Feb 19, 2019
1d7fb9c
fixed merge conflicts
bramanudom Mar 21, 2019
8dc93d5
fixed typecheck error
bramanudom Mar 21, 2019
3b149cc
fixed typecheck error
bramanudom Mar 21, 2019
d33f448
update story sidebar visual example, revert extra added line
bramanudom Mar 22, 2019
74dacc3
fixed merge conflicts
bramanudom Mar 22, 2019
ffa60db
fixed merge conflicts
bramanudom Mar 22, 2019
8f935ee
delete extra testing file
bramanudom Mar 22, 2019
91a3f4d
re-add missing commits from cherry-pick
bramanudom Mar 22, 2019
47978de
Merge branch 'master' into sidebar_links
bramanudom Mar 25, 2019
efddc95
revert changes for page change action, formatting for visual test
bramanudom Mar 25, 2019
1e53480
addressed PR changes
bramanudom Mar 26, 2019
587e05f
suppress type check error
bramanudom Mar 26, 2019
c691147
Merge branch 'master' into sidebar_links
bramanudom Mar 27, 2019
7ec27b4
merge and fix conflicts
bramanudom May 22, 2019
93d0a06
merge and fix conflicts
bramanudom May 22, 2019
7b91cb4
merge and fix conflicts
bramanudom May 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 17 additions & 7 deletions examples/visual-tests/amp-story/amp-story-sidebar.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
publisher-logo-src="/examples/visual-tests/photos/120.png"
poster-portrait-src="/examples/visual-tests/picsum.photos/image981_900x1600.jpg"
poster-landscape-src="/examples/visual-tests/picsum.photos/image981_1600x900.jpg"
poster-square-src="/examples/visual-tests/picsum.photos/image981_1600x1600.jpg">
<amp-sidebar id="sidebar1" layout="nodisplay" >
<button id="close-button" on="tap:sidebar1.close"> close </button>
<ul>
<li>Nav item 1</li>
<li><a href="#idTwo" on="tap:idTwo.scrollTo">Nav item 2</a></li>
<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="/examples/visual-tests/amp-story/amp-story-sidebar.html#page=page-2">Absolute fragment link</a></span></li>
<li><a href="/examples/visual-tests/amp-story/amp-story-tooltip.html#page=page-2"> Link to another story using fragment </a></li>
<li><a href="#page=page-3"> In Link Hash </a> </li>
<li on="tap:story.goToPage(id=page-3)">In Link Action</li>
<li>Nav item 5</li>
<li>Nav item 6</li>
</ul>
Expand All @@ -50,7 +51,7 @@
</amp-story-grid-layer>
<amp-story-grid-layer template="vertical">
<h1 class="hello-world">Hello world!</h1>
<p>Page one of two</p>
<p>Page one of three</p>
</amp-story-grid-layer>
</amp-story-page>

Expand All @@ -59,7 +60,16 @@ <h1 class="hello-world">Hello world!</h1>
</amp-story-grid-layer>
<amp-story-grid-layer template="vertical">
<h1 class="hello-world">Hello world!</h1>
<p>Page two of two</p>
<p>Page two of three</p>
</amp-story-grid-layer>
</amp-story-page>

<amp-story-page id="page-3">
<amp-story-grid-layer template="fill">
</amp-story-grid-layer>
<amp-story-grid-layer template="vertical">
<h1 class="hello-world">Hello world!</h1>
<p>Page three of three</p>
</amp-story-grid-layer>
</amp-story-page>

Expand Down
91 changes: 83 additions & 8 deletions extensions/amp-story/1.0/amp-story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ import {
childElementByTag,
childElements,
closest,
closestAncestorElementBySelector,
createElementWithAttributes,
isRTL,
scopedQuerySelectorAll,
Expand All @@ -109,6 +110,7 @@ import {getMode} from '../../../src/mode';
import {isExperimentOn} from '../../../src/experiments';
import {parseQueryString} from '../../../src/url';
import {registerServiceBuilder} from '../../../src/service';
import {startsWith} from '../../../src/string';
import {upgradeBackgroundAudio} from './audio';
import LocalizedStringsAr from './_locales/ar';
import LocalizedStringsDe from './_locales/de';
Expand Down Expand Up @@ -438,13 +440,35 @@ export class AmpStory extends AMP.BaseElement {

if (isExperimentOn(this.win, 'amp-story-branching')) {
this.registerAction('goToPage', invocation => {
const {args} = invocation;
const {args, caller} = invocation;

if (args) {
this.storeService_.dispatch(
Action.SET_ADVANCEMENT_MODE,
AdvancementMode.GO_TO_PAGE
);
this.switchTo_(args['id'], NavigationDirection.NEXT);

if (
closestAncestorElementBySelector(
dev().assertElement(caller),
'AMP-SIDEBAR'
)
) {
const pageId = args['id'];
Services.historyForDoc(this.getAmpDoc())
.goBack()
.then(() => {
this.win.requestAnimationFrame(() => {
if (this.isActualPage_(pageId)) {
this.switchTo_(pageId, NavigationDirection.NEXT).then(() =>
this.closeOpacityMask_()
);
}
});
});
} else {
this.switchTo_(args['id'], NavigationDirection.NEXT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Load the story, you're on the cover page
  2. Open the sidebar and click on a link or action to go to page 2
  3. You're on page2, you click next, you're on page 3
  4. Navigate back
  5. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
}
});
}
Expand Down Expand Up @@ -750,6 +774,24 @@ export class AmpStory extends AMP.BaseElement {
}
});

if (isExperimentOn(this.win, 'amp-story-branching')) {
this.win.addEventListener('hashchange', () => {
const maybePageId = parseQueryString(this.win.location.hash)['page'];
if (this.isActualPage_(maybePageId)) {
this.switchTo_(maybePageId, NavigationDirection.NEXT).then(() => {
this.closeOpacityMask_();
});
// Remove the fragment parameter from the URL
const {history} = this.win;
history.pushState(
/* data */ '',
/* title */ this.win.document.title,
/* URL */ this.win.location.pathname
);
}
});
}

// TODO(#16795): Remove once the runtime triggers pause/resume callbacks
// on document visibility change (eg: user switches tab).
this.documentState_.onVisibilityChanged(() => this.onVisibilityChanged_());
Expand Down Expand Up @@ -1016,27 +1058,35 @@ export class AmpStory extends AMP.BaseElement {
* @private
*/
getInitialPageId_(firstPageEl) {
const isActualPage = pageId =>
findIndex(this.pages_, page => page.element.id === pageId) >= 0;
const historyPage = /** @type {string} */ (getHistoryState(
this.win,
HistoryState.PAGE_ID
));

if (isExperimentOn(this.win, 'amp-story-branching')) {
const maybePageId = parseQueryString(this.win.location.hash)['page'];
if (maybePageId && isActualPage(maybePageId)) {
if (maybePageId && this.isActualPage_(maybePageId)) {
return maybePageId;
}
}

if (historyPage && isActualPage(historyPage)) {
if (historyPage && this.isActualPage_(historyPage)) {
return historyPage;
}

return firstPageEl.id;
}

/**
* Checks to see if a page ID refers to an actual page in the story.
* @param {string} pageId
* @return {boolean}
* @private
*/
isActualPage_(pageId) {
return findIndex(this.pages_, page => page.element.id === pageId) >= 0;
}

/**
* @param {number} timeoutMs The maximum amount of time to wait, in
* milliseconds.
Expand Down Expand Up @@ -2504,6 +2554,28 @@ export class AmpStory extends AMP.BaseElement {
if (!this.sidebar_) {
return;
}
if (isExperimentOn(this.win, 'amp-story-branching')) {
const linkEls = Array.prototype.slice.call(
this.sidebar_.querySelectorAll('a')
);

linkEls.forEach(linkEl => {
const url = linkEl.getAttribute('href');
// Handles both anchor links (#page=someId) and absolute links; click
// handler should not be added to absolute links from same domain but
// different story.
if (
url.indexOf('#page=') >= 0 &&
(startsWith(url, '#') || url.indexOf(this.win.location.pathname) >= 0)
) {
linkEl.addEventListener('click', () => {
// Do not prevent default; in safari, preventing default and
// appending a hash to a URL without a preceding slash fails.
this.win.location.hash = url.slice(url.search('#(.*)'));
});
}
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


this.mutateElement(() => {
this.sidebar_.classList.add(SIDEBAR_CLASS_NAME);
Expand All @@ -2512,12 +2584,15 @@ export class AmpStory extends AMP.BaseElement {
this.initializeOpacityMask_();
this.storeService_.dispatch(Action.TOGGLE_HAS_SIDEBAR, !!this.sidebar_);

const actions = [
const sidebarActions = [
{tagOrTarget: 'AMP-SIDEBAR', method: 'open'},
{tagOrTarget: 'AMP-SIDEBAR', method: 'close'},
{tagOrTarget: 'AMP-SIDEBAR', method: 'toggle'},
];
this.storeService_.dispatch(Action.ADD_TO_ACTIONS_WHITELIST, actions);
this.storeService_.dispatch(
Action.ADD_TO_ACTIONS_WHITELIST,
sidebarActions
);
}

/**
Expand Down