-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix amp-sidebar local navigation and history #5520
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,9 +14,12 @@ | |
* limitations under the License. | ||
*/ | ||
|
||
import {closestByTag} from '../../../src/dom'; | ||
import {CSS} from '../../../build/amp-sidebar-0.1.css'; | ||
import {dev} from '../../../src/log'; | ||
import {Layout} from '../../../src/layout'; | ||
import {historyForDoc} from '../../../src/history'; | ||
import {parseUrl} from '../../../src/url'; | ||
import {platformFor} from '../../../src/platform'; | ||
import {setStyles, toggle} from '../../../src/style'; | ||
import {vsyncFor} from '../../../src/vsync'; | ||
|
@@ -107,14 +110,25 @@ export class AmpSidebar extends AMP.BaseElement { | |
screenReaderCloseButton.classList.add('-amp-screen-reader'); | ||
// This is for screen-readers only, should not get a tab stop. | ||
screenReaderCloseButton.tabIndex = -1; | ||
screenReaderCloseButton.addEventListener('click', () => { | ||
this.close_(); | ||
}); | ||
screenReaderCloseButton.addEventListener('click', () => this.close_()); | ||
this.element.appendChild(screenReaderCloseButton); | ||
|
||
this.registerAction('toggle', this.toggle_.bind(this)); | ||
this.registerAction('open', this.open_.bind(this)); | ||
this.registerAction('close', this.close_.bind(this)); | ||
|
||
this.element.addEventListener('click', e => { | ||
const target = closestByTag(dev().assertElement(e.target), 'A'); | ||
if (!target) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check that |
||
return; | ||
} | ||
const tgtLoc = parseUrl(target.href); | ||
|
||
if (!tgtLoc.hash) { | ||
return; | ||
} | ||
this.close_(); | ||
}); | ||
} | ||
|
||
/** | ||
|
@@ -172,7 +186,7 @@ export class AmpSidebar extends AMP.BaseElement { | |
}, ANIMATION_TIMEOUT); | ||
}); | ||
}); | ||
this.getHistory_().push(this.close_.bind(this)).then(historyId => { | ||
this.getHistory_().push(() => this.close_()).then(historyId => { | ||
this.historyId_ = historyId; | ||
}); | ||
} | ||
|
@@ -213,9 +227,7 @@ export class AmpSidebar extends AMP.BaseElement { | |
if (!this.maskElement_) { | ||
const mask = this.document_.createElement('div'); | ||
mask.classList.add('-amp-sidebar-mask'); | ||
mask.addEventListener('click', () => { | ||
this.close_(); | ||
}); | ||
mask.addEventListener('click', () => this.close_()); | ||
this.element.parentNode.appendChild(mask); | ||
mask.addEventListener('touchmove', e => { | ||
e.preventDefault(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -573,8 +573,12 @@ export class HistoryBindingVirtual_ { | |
|
||
/** | ||
* @param {!./viewer-impl.Viewer} viewer | ||
* @param {!Window} win | ||
*/ | ||
constructor(viewer) { | ||
constructor(viewer, win) { | ||
/** @private @const {!../service/timer-impl.Timer} */ | ||
this.timer_ = timerFor(win); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can just do |
||
|
||
/** @private @const */ | ||
this.viewer_ = viewer; | ||
|
||
|
@@ -587,6 +591,12 @@ export class HistoryBindingVirtual_ { | |
/** @private {!UnlistenDef} */ | ||
this.unlistenOnHistoryPopped_ = this.viewer_.onHistoryPoppedEvent( | ||
this.onHistoryPopped_.bind(this)); | ||
|
||
/** @private {!Promise} */ | ||
this.awaitPoppingPromise_ = Promise.resolve(); | ||
|
||
/** @private {?function(*)) */ | ||
this.awaitPoppingPromiseResolver_ = null; | ||
} | ||
|
||
/** @override */ | ||
|
@@ -601,14 +611,25 @@ export class HistoryBindingVirtual_ { | |
|
||
/** @override */ | ||
push() { | ||
// Current implementation doesn't wait for response from viewer. | ||
this.updateStackIndex_(this.stackIndex_ + 1); | ||
this.viewer_.postPushHistory(this.stackIndex_); | ||
return Promise.resolve(this.stackIndex_); | ||
// Waiting for popping confirmation to happen otherwise a race condition | ||
// 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_]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why |
||
.then(() => { | ||
// Current implementation doesn't wait for response from viewer. | ||
this.updateStackIndex_(this.stackIndex_ + 1); | ||
this.viewer_.postPushHistory(this.stackIndex_); | ||
return Promise.resolve(this.stackIndex_); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just return |
||
}); | ||
} | ||
|
||
/** @override */ | ||
pop(stackIndex) { | ||
this.awaitPoppingPromise_ = new Promise(resolve => { | ||
this.awaitPoppingPromiseResolver_ = resolve; | ||
}); | ||
|
||
if (stackIndex > this.stackIndex_) { | ||
return Promise.resolve(this.stackIndex_); | ||
} | ||
|
@@ -623,6 +644,10 @@ export class HistoryBindingVirtual_ { | |
*/ | ||
onHistoryPopped_(event) { | ||
this.updateStackIndex_(event.newStackIndex); | ||
if (this.awaitPoppingPromiseResolver_) { | ||
this.awaitPoppingPromiseResolver_(); | ||
this.awaitPoppingPromiseResolver_ = null; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -652,7 +677,7 @@ function createHistory(ampdoc) { | |
let binding; | ||
if (viewer.isOvertakeHistory() || getMode(ampdoc.win).test || | ||
ampdoc.win.AMP_TEST_IFRAME) { | ||
binding = new HistoryBindingVirtual_(viewer); | ||
binding = new HistoryBindingVirtual_(viewer, ampdoc.win); | ||
} else { | ||
// Only one global "natural" binding is allowed since it works with the | ||
// global history stack. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we should create a
boundClose
, since it's used so frequently.