From 0a840e26d0d32bafb1a8f3f150183acfb0f9b6ba Mon Sep 17 00:00:00 2001 From: zhouyx Date: Thu, 17 Aug 2017 17:37:15 -0700 Subject: [PATCH 1/4] remove unused timer delay --- extensions/amp-sidebar/0.1/amp-sidebar.js | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/extensions/amp-sidebar/0.1/amp-sidebar.js b/extensions/amp-sidebar/0.1/amp-sidebar.js index 5eff71cc1d7f..b0f452a7c087 100644 --- a/extensions/amp-sidebar/0.1/amp-sidebar.js +++ b/extensions/amp-sidebar/0.1/amp-sidebar.js @@ -90,6 +90,7 @@ export class AmpSidebar extends AMP.BaseElement { const children = this.getRealChildren(); this.scheduleLayout(children); this.scheduleResume(children); + tryFocus(this.element); }, 500); } @@ -257,18 +258,8 @@ export class AmpSidebar extends AMP.BaseElement { this.openMask_(); this.element.setAttribute('open', ''); this.element.setAttribute('aria-hidden', 'false'); - if (this.openOrCloseTimeOut_) { - this.timer_.cancel(this.openOrCloseTimeOut_); - } - this.openOrCloseTimeOut_ = this.timer_.delay(() => { - const children = this.getRealChildren(); - this.scheduleLayout(children); - this.scheduleResume(children); - this.element.addEventListener('transitionend', this.boundReschedule_); - this.element.addEventListener('animationend', this.boundReschedule_); - // Focus on the sidebar for a11y. - tryFocus(this.element); - }, ANIMATION_TIMEOUT); + this.element.addEventListener('transitionend', this.boundReschedule_); + this.element.addEventListener('animationend', this.boundReschedule_); }); }); this.getHistory_().push(this.close_.bind(this)).then(historyId => { From e04a4c1fd4446c236691c80f2abbf479df2fe1b9 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Thu, 17 Aug 2017 19:58:21 -0700 Subject: [PATCH 2/4] wait for transitionend on cloase --- extensions/amp-sidebar/0.1/amp-sidebar.js | 51 ++++++++++++----------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/extensions/amp-sidebar/0.1/amp-sidebar.js b/extensions/amp-sidebar/0.1/amp-sidebar.js index b0f452a7c087..92b033cb6e13 100644 --- a/extensions/amp-sidebar/0.1/amp-sidebar.js +++ b/extensions/amp-sidebar/0.1/amp-sidebar.js @@ -79,19 +79,12 @@ export class AmpSidebar extends AMP.BaseElement { /** @private {boolean} */ this.bottomBarCompensated_ = false; - /** @private @const {!../../../src/service/timer-impl.Timer} */ - this.timer_ = Services.timerFor(this.win); - /** @private {number|string|null} */ this.openOrCloseTimeOut_ = null; /** @const {function()} */ - this.boundReschedule_ = debounce(this.win, () => { - const children = this.getRealChildren(); - this.scheduleLayout(children); - this.scheduleResume(children); - tryFocus(this.element); - }, 500); + this.boundOnAnimationEnd_ = + debounce(this.win, this.onAnimationEnd_.bind(this), ANIMATION_TIMEOUT); } /** @override */ @@ -192,6 +185,9 @@ export class AmpSidebar extends AMP.BaseElement { } } }, true); + + this.element.addEventListener('transitionend', this.boundOnAnimationEnd_); + this.element.addEventListener('animationend', this.boundOnAnimationEnd_); } /** @override */ @@ -257,9 +253,8 @@ export class AmpSidebar extends AMP.BaseElement { this.vsync_.mutate(() => { this.openMask_(); this.element.setAttribute('open', ''); + this.boundOnAnimationEnd_(); this.element.setAttribute('aria-hidden', 'false'); - this.element.addEventListener('transitionend', this.boundReschedule_); - this.element.addEventListener('animationend', this.boundReschedule_); }); }); this.getHistory_().push(this.close_.bind(this)).then(historyId => { @@ -276,23 +271,11 @@ export class AmpSidebar extends AMP.BaseElement { return; } this.viewport_.leaveOverlayMode(); - this.element.removeEventListener('transitionend', this.boundReschedule_); - this.element.removeEventListener('animationend', this.boundReschedule_); this.vsync_.mutate(() => { this.closeMask_(); this.element.removeAttribute('open'); + this.boundOnAnimationEnd_(); this.element.setAttribute('aria-hidden', 'true'); - if (this.openOrCloseTimeOut_) { - this.timer_.cancel(this.openOrCloseTimeOut_); - } - this.openOrCloseTimeOut_ = this.timer_.delay(() => { - if (!this.isOpen_()) { - this.vsync_.mutate(() => { - toggle(this.element, /* display */false); - this.schedulePause(this.getRealChildren()); - }); - } - }, ANIMATION_TIMEOUT); }); if (this.historyId_ != -1) { this.getHistory_().pop(this.historyId_); @@ -371,6 +354,26 @@ export class AmpSidebar extends AMP.BaseElement { getHistory_() { return Services.historyForDoc(this.getAmpDoc()); } + + /** + * Get called when animation/transition end when open/close sidebar + * @private + */ + onAnimationEnd_() { + if (this.isOpen_()) { + // On open sidebar + const children = this.getRealChildren(); + this.scheduleLayout(children); + this.scheduleResume(children); + tryFocus(this.element); + } else { + // On close sidebar + this.vsync_.mutate(() => { + toggle(this.element, /* display */false); + this.schedulePause(this.getRealChildren()); + }); + } + } } AMP.extension('amp-sidebar', '0.1', AMP => { From db91e34883554cfe2fa6557fb826f7cf586fae13 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Fri, 18 Aug 2017 10:54:27 -0700 Subject: [PATCH 3/4] change timeout time --- extensions/amp-sidebar/0.1/amp-sidebar.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/amp-sidebar/0.1/amp-sidebar.js b/extensions/amp-sidebar/0.1/amp-sidebar.js index 92b033cb6e13..2cde8b9f76eb 100644 --- a/extensions/amp-sidebar/0.1/amp-sidebar.js +++ b/extensions/amp-sidebar/0.1/amp-sidebar.js @@ -31,7 +31,7 @@ import {toArray} from '../../../src/types'; const TAG = 'amp-sidebar toolbar'; /** @const */ -const ANIMATION_TIMEOUT = 550; +const ANIMATION_TIMEOUT = 350; /** @const */ const IOS_SAFARI_BOTTOMBAR_HEIGHT = '10vh'; From 89eb012e0e6724162c10f9268ebf4be5d534d967 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Fri, 18 Aug 2017 15:12:44 -0700 Subject: [PATCH 4/4] fix test --- .../amp-sidebar/0.1/test/test-amp-sidebar.js | 77 ++++++++----------- 1 file changed, 34 insertions(+), 43 deletions(-) diff --git a/extensions/amp-sidebar/0.1/test/test-amp-sidebar.js b/extensions/amp-sidebar/0.1/test/test-amp-sidebar.js index a864b11b546e..0390f944b572 100644 --- a/extensions/amp-sidebar/0.1/test/test-amp-sidebar.js +++ b/extensions/amp-sidebar/0.1/test/test-amp-sidebar.js @@ -23,6 +23,7 @@ import {assertScreenReaderElement} from '../../../../testing/test-helper'; import {toggleExperiment} from '../../../../src/experiments'; import * as sinon from 'sinon'; import '../amp-sidebar'; +import * as lolex from 'lolex'; adopt(window); @@ -38,6 +39,7 @@ describes.realWin('amp-sidebar 0.1 version', { }, () => { let sandbox; let platform; + let clock; let timer; function getAmpSidebar(options) { @@ -180,9 +182,6 @@ describes.realWin('amp-sidebar 0.1 version', { callback(); }, }; - sandbox.stub(timer, 'delay', function(callback) { - callback(); - }); impl.open_(); expect(iframe.doc.querySelectorAll('.i-amphtml-sidebar-mask').length) .to.equal(1); @@ -210,6 +209,7 @@ describes.realWin('amp-sidebar 0.1 version', { return getAmpSidebar().then(obj => { const sidebarElement = obj.ampSidebar; const impl = sidebarElement.implementation_; + clock = lolex.install(impl.win, 0, ['Date', 'setTimeout']); const historyPushSpy = sandbox.spy(); const historyPopSpy = sandbox.spy(); impl.scheduleLayout = sandbox.spy(); @@ -230,23 +230,22 @@ describes.realWin('amp-sidebar 0.1 version', { callback(); }, }; - sandbox.stub(timer, 'delay', function(callback) { - callback(); - }); - timer.cancel = sandbox.spy(); impl.openOrCloseTimeOut_ = 10; impl.open_(); expect(sidebarElement.hasAttribute('open')).to.be.true; expect(sidebarElement.getAttribute('aria-hidden')).to.equal('false'); expect(sidebarElement.getAttribute('role')).to.equal('menu'); - expect(obj.iframe.doc.activeElement).to.equal(sidebarElement); - expect(sidebarElement.style.display).to.equal(''); - expect(timer.cancel).to.be.calledOnce; - expect(impl.scheduleLayout).to.be.calledOnce; + expect(historyPushSpy).to.be.calledOnce; expect(historyPopSpy).to.have.not.been.called; expect(impl.historyId_).to.not.equal('-1'); + expect(impl.scheduleLayout).to.not.be.called; + + clock.tick(600); + expect(obj.iframe.doc.activeElement).to.equal(sidebarElement); + expect(sidebarElement.style.display).to.equal(''); + expect(impl.scheduleLayout).to.be.calledOnce; // second call to open_() should be a no-op and not increase call counts. impl.open_(); @@ -261,6 +260,7 @@ describes.realWin('amp-sidebar 0.1 version', { return getAmpSidebar({'open': true}).then(obj => { const sidebarElement = obj.ampSidebar; const impl = sidebarElement.implementation_; + clock = lolex.install(impl.win, 0, ['Date', 'setTimeout']); impl.schedulePause = sandbox.spy(); const historyPushSpy = sandbox.spy(); const historyPopSpy = sandbox.spy(); @@ -283,17 +283,13 @@ describes.realWin('amp-sidebar 0.1 version', { callback(); }, }; - sandbox.stub(timer, 'delay', function(callback) { - callback(); - }); - timer.cancel = sandbox.spy(); impl.openOrCloseTimeOut_ = 10; impl.close_(); expect(sidebarElement.hasAttribute('open')).to.be.false; expect(sidebarElement.getAttribute('aria-hidden')).to.equal('true'); + clock.tick(600); expect(sidebarElement.style.display).to.equal('none'); - expect(timer.cancel).to.be.calledOnce; expect(impl.schedulePause).to.be.calledOnce; expect(historyPopSpy).to.be.calledOnce; expect(impl.historyId_).to.equal(-1); @@ -309,6 +305,7 @@ describes.realWin('amp-sidebar 0.1 version', { return getAmpSidebar().then(obj => { const sidebarElement = obj.ampSidebar; const impl = sidebarElement.implementation_; + clock = lolex.install(impl.win, 0, ['Date', 'setTimeout']); impl.scheduleLayout = sandbox.spy(); impl.schedulePause = sandbox.spy(); impl.vsync_ = { @@ -316,9 +313,7 @@ describes.realWin('amp-sidebar 0.1 version', { callback(); }, }; - sandbox.stub(timer, 'delay', function(callback) { - callback(); - }); + expect(sidebarElement.hasAttribute('open')).to.be.false; expect(sidebarElement.getAttribute('aria-hidden')).to.equal('true'); expect(sidebarElement.getAttribute('role')).to.equal('menu'); @@ -326,12 +321,14 @@ describes.realWin('amp-sidebar 0.1 version', { impl.toggle_(); expect(sidebarElement.hasAttribute('open')).to.be.true; expect(sidebarElement.getAttribute('aria-hidden')).to.equal('false'); + clock.tick(600); expect(obj.iframe.doc.activeElement).to.equal(sidebarElement); expect(sidebarElement.style.display).to.equal(''); expect(impl.scheduleLayout).to.be.calledOnce; impl.toggle_(); expect(sidebarElement.hasAttribute('open')).to.be.false; expect(sidebarElement.getAttribute('aria-hidden')).to.equal('true'); + clock.tick(600); expect(sidebarElement.style.display).to.equal('none'); expect(impl.schedulePause).to.be.calledOnce; }); @@ -342,15 +339,13 @@ describes.realWin('amp-sidebar 0.1 version', { const iframe = obj.iframe; const sidebarElement = obj.ampSidebar; const impl = sidebarElement.implementation_; + clock = lolex.install(impl.win, 0, ['Date', 'setTimeout']); impl.schedulePause = sandbox.spy(); impl.vsync_ = { mutate(callback) { callback(); }, }; - sandbox.stub(timer, 'delay', function(callback) { - callback(); - }); expect(sidebarElement.hasAttribute('open')).to.be.false; impl.open_(); expect(sidebarElement.hasAttribute('open')).to.be.true; @@ -367,6 +362,7 @@ describes.realWin('amp-sidebar 0.1 version', { el.dispatchEvent(eventObj) : el.fireEvent('onkeydown', eventObj); expect(sidebarElement.hasAttribute('open')).to.be.false; expect(sidebarElement.getAttribute('aria-hidden')).to.equal('true'); + clock.tick(600); expect(sidebarElement.style.display).to.equal('none'); expect(impl.schedulePause).to.be.calledOnce; }); @@ -376,6 +372,7 @@ describes.realWin('amp-sidebar 0.1 version', { return getAmpSidebar().then(obj => { const sidebarElement = obj.ampSidebar; const impl = sidebarElement.implementation_; + clock = lolex.install(impl.win, 0, ['Date', 'setTimeout']); impl.schedulePause = sandbox.spy(); impl.scheduleResume = sandbox.spy(); impl.vsync_ = { @@ -383,26 +380,29 @@ describes.realWin('amp-sidebar 0.1 version', { callback(); }, }; - sandbox.stub(timer, 'delay', function(callback) { - callback(); - }); + expect(impl.isOpen_()).to.be.false; + clock.tick(600); expect(impl.schedulePause).to.have.not.been.called; expect(impl.scheduleResume).to.have.not.been.called; impl.toggle_(); expect(impl.isOpen_()).to.be.true; + clock.tick(600); expect(impl.schedulePause).to.have.not.been.called; expect(impl.scheduleResume).to.be.calledOnce; impl.toggle_(); expect(impl.isOpen_()).to.be.false; + clock.tick(600); expect(impl.schedulePause).to.be.calledOnce; expect(impl.scheduleResume).to.be.calledOnce; impl.toggle_(); expect(impl.isOpen_()).to.be.true; + clock.tick(600); expect(impl.schedulePause).to.be.calledOnce; expect(impl.scheduleResume).to.have.callCount(2); impl.toggle_(); expect(impl.isOpen_()).to.be.false; + clock.tick(600); expect(impl.schedulePause).to.have.callCount(2); expect(impl.scheduleResume).to.have.callCount(2); }); @@ -459,15 +459,13 @@ describes.realWin('amp-sidebar 0.1 version', { const anchor = sidebarElement.getElementsByTagName('a')[0]; anchor.href = '#newloc'; const impl = sidebarElement.implementation_; + clock = lolex.install(impl.win, 0, ['Date', 'setTimeout']); impl.schedulePause = sandbox.spy(); impl.vsync_ = { mutate(callback) { callback(); }, }; - sandbox.stub(timer, 'delay', function(callback) { - callback(); - }); expect(sidebarElement.hasAttribute('open')).to.be.false; impl.open_(); expect(sidebarElement.hasAttribute('open')).to.be.true; @@ -491,6 +489,7 @@ describes.realWin('amp-sidebar 0.1 version', { anchor.fireEvent('onkeydown', eventObj); expect(sidebarElement.hasAttribute('open')).to.be.false; expect(sidebarElement.getAttribute('aria-hidden')).to.equal('true'); + clock.tick(600); expect(sidebarElement.style.display).to.equal('none'); expect(impl.schedulePause).to.be.calledOnce; }); @@ -623,34 +622,26 @@ describes.realWin('amp-sidebar 0.1 version', { return getAmpSidebar().then(obj => { const sidebarElement = obj.ampSidebar; const impl = sidebarElement.implementation_; - impl.boundReschedule_ = sandbox.spy(); + clock = lolex.install(impl.win, 0, ['Date', 'setTimeout']); + impl.boundOnAnimationEnd_ = sandbox.spy(); + impl.buildCallback(); impl.vsync_ = { mutate(callback) { callback(); }, }; - sandbox.stub(timer, 'delay', function(callback) { - callback(); - }); - impl.toggle_(); - const animationEndEventObj = new Event( + const animationEndEvent = new Event( 'animationend', {bubbles: true} ); - sidebarElement.firstChild.dispatchEvent(animationEndEventObj); - expect(impl.boundReschedule_).to.be.calledOnce; - impl.boundReschedule_.reset(); + sidebarElement.firstChild.dispatchEvent(animationEndEvent); + expect(impl.boundOnAnimationEnd_).to.be.calledOnce; const transitionEndEvent = new Event( 'transitionend', {bubbles: true} ); sidebarElement.firstChild.dispatchEvent(transitionEndEvent); - expect(impl.boundReschedule_).to.be.calledOnce; - impl.boundReschedule_.reset(); - impl.toggle_(); - sidebarElement.firstChild.dispatchEvent(animationEndEventObj); - sidebarElement.firstChild.dispatchEvent(transitionEndEvent); - expect(impl.boundReschedule_).to.not.be.called; + expect(impl.boundOnAnimationEnd_).to.be.calledTwice; }); }); });