From 087dc7ea28fda4cb6e64ddb276dd85e2d645cfd8 Mon Sep 17 00:00:00 2001 From: Alan Orozco Date: Tue, 5 Feb 2019 10:53:16 -0800 Subject: [PATCH 1/8] =?UTF-8?q?=20=E2=9C=A8Honor=20autofocus=20on=20show/t?= =?UTF-8?q?oggle?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/service/standard-actions-impl.js | 50 ++++-- test/manual/autofocus-on-show.html | 99 ++++++++++++ test/unit/test-standard-actions.js | 221 +++++++++++++++++++++++---- 3 files changed, 330 insertions(+), 40 deletions(-) create mode 100644 test/manual/autofocus-on-show.html diff --git a/src/service/standard-actions-impl.js b/src/service/standard-actions-impl.js index 8a3be93793aa..2213b22f967b 100644 --- a/src/service/standard-actions-impl.js +++ b/src/service/standard-actions-impl.js @@ -34,6 +34,18 @@ function isShowable(element) { return element.hasAttribute('hidden'); } +/** + * @param {!Element} element + * @return {?Element} + * @visibleForTesting + */ +export function getAutofocusElementForShowAction(element) { + if (element.hasAttribute('autofocus')) { + return element; + } + return element.querySelector('[autofocus]'); +} + /** @const {string} */ const TAG = 'STANDARD-ACTIONS'; @@ -287,8 +299,8 @@ export class StandardActions { * @param {!./action-impl.ActionInvocation} invocation * @return {?Promise} */ - handleShow(invocation) { - const target = dev().assertElement(invocation.node); + handleShow({node}) { + const target = dev().assertElement(node); const ownerWindow = toWin(target.ownerDocument.defaultView); if (target.classList.contains(getLayoutClass(Layout.NODISPLAY))) { @@ -311,17 +323,37 @@ export class StandardActions { } }); - this.resources_.mutateElement(target, () => { - if (target.classList.contains('i-amphtml-element')) { - target./*OK*/expand(); - } else { - toggle(target, true); - } - }); + const autofocusElOrNull = getAutofocusElementForShowAction(target); + + // Safari only honors focus in sync operations. + if (Services.platformFor(ownerWindow).isSafari() && + autofocusElOrNull) { + this.handleShowSync_(target, autofocusElOrNull); + } else { + this.resources_.mutateElement(target, () => { + this.handleShowSync_(target, autofocusElOrNull); + }); + } return null; } + /** + * @param {!Element} target + * @param {?Element} autofocusElOrNull + * @private + */ + handleShowSync_(target, autofocusElOrNull) { + if (target.classList.contains('i-amphtml-element')) { + target./*OK*/expand(); + } else { + toggle(target, true); + } + if (autofocusElOrNull) { + tryFocus(dev().assertElement(autofocusElOrNull)); + } + } + /** * Handles "toggle" action. * @param {!./action-impl.ActionInvocation} invocation diff --git a/test/manual/autofocus-on-show.html b/test/manual/autofocus-on-show.html new file mode 100644 index 000000000000..7a75062b5298 --- /dev/null +++ b/test/manual/autofocus-on-show.html @@ -0,0 +1,99 @@ + + + + + + Autofocus on show + + + + + + + + + + + + + + + diff --git a/test/unit/test-standard-actions.js b/test/unit/test-standard-actions.js index 121fc8305146..0e07ae576e1f 100644 --- a/test/unit/test-standard-actions.js +++ b/test/unit/test-standard-actions.js @@ -17,8 +17,12 @@ import {AmpDocSingle} from '../../src/service/ampdoc-impl'; import {RAW_OBJECT_ARGS_KEY} from '../../src/action-constants'; import {Services} from '../../src/services'; -import {StandardActions} from '../../src/service/standard-actions-impl'; +import { + StandardActions, + getAutofocusElementForShowAction, +} from '../../src/service/standard-actions-impl'; import {cidServiceForDocForTesting} from '../../src/service/cid-impl'; +import {htmlFor} from '../../src/static-template'; import {installHistoryServiceForDoc} from '../../src/service/history-impl'; import {macroTask} from '../../testing/yield'; import {setParentWindow} from '../../src/service'; @@ -48,40 +52,47 @@ describes.sandboxed('StandardActions', {}, () => { (unusedElement, mutator) => mutator()); } + function expectElementMutatedAsync(element) { + expect(mutateElementStub.withArgs(element, sinon.match.any)) + .to.be.calledOnce; + } + function expectElementToHaveBeenHidden(element) { - expect(mutateElementStub).to.be.calledOnce; - expect(mutateElementStub.firstCall.args[0]).to.equal(element); + expectElementMutatedAsync(element); expect(element).to.have.attribute('hidden'); } - function expectElementToHaveBeenShown(element) { - expect(mutateElementStub).to.be.calledOnce; - expect(mutateElementStub.firstCall.args[0]).to.equal(element); + function expectElementToHaveBeenShown(element, sync = false) { + if (sync) { + expect(mutateElementStub).to.not.have.been.called; + } else { + expectElementMutatedAsync(element); + } expect(element).to.not.have.attribute('hidden'); expect(element.hasAttribute('hidden')).to.be.false; } function expectElementToHaveClass(element, className) { - expect(mutateElementStub).to.be.calledOnce; - expect(mutateElementStub.firstCall.args[0]).to.equal(element); + expectElementMutatedAsync(element); expect(element.classList.contains(className)).to.true; } function expectElementToDropClass(element, className) { - expect(mutateElementStub).to.be.calledOnce; - expect(mutateElementStub.firstCall.args[0]).to.equal(element); + expectElementMutatedAsync(element); expect(element.classList.contains(className)).to.false; } function expectAmpElementToHaveBeenHidden(element) { - expect(mutateElementStub).to.be.calledOnce; - expect(mutateElementStub.firstCall.args[0]).to.equal(element); + expectElementMutatedAsync(element); expect(element.collapse).to.be.calledOnce; } - function expectAmpElementToHaveBeenShown(element) { - expect(mutateElementStub).to.be.calledOnce; - expect(mutateElementStub.firstCall.args[0]).to.equal(element); + function expectAmpElementToHaveBeenShown(element, sync = false) { + if (!sync) { + expectElementMutatedAsync(element); + } else { + expect(mutateElementStub).to.not.have.been.called; + } expect(element.expand).to.be.calledOnce; } @@ -90,6 +101,14 @@ describes.sandboxed('StandardActions', {}, () => { expect(scrollStub.firstCall.args[0]).to.equal(element); } + function trustedInvocation(obj) { + return Object.assign({satisfiesTrust: () => true}, obj); + } + + function stubIsSafari() { + sandbox.stub(Services, 'platformFor').returns({isSafari: () => true}); + } + beforeEach(() => { ampdoc = new AmpDocSingle(window); standardActions = new StandardActions(ampdoc); @@ -100,17 +119,45 @@ describes.sandboxed('StandardActions', {}, () => { }); + describe('getAutofocusElementForShowAction', () => { + + let html; + beforeEach(() => { + html = htmlFor(document); + }); + + it('returns outer element when [autofocus]', () => { + const el = html``; + expect(getAutofocusElementForShowAction(el)).to.equal(el); + }); + + it('returns inner [autofocus] element', () => { + const inner = html``; + const outer = html`
`; + + outer.firstElementChild.appendChild(inner); + + expect(getAutofocusElementForShowAction(outer)).to.equal(inner); + }); + + it('returns null', () => { + const el = html`
`; + expect(getAutofocusElementForShowAction(el)).to.be.null; + }); + + }); + describe('"hide" action', () => { it('should handle normal element', () => { const element = createElement(); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleHide(invocation); expectElementToHaveBeenHidden(element); }); it('should handle AmpElement', () => { const element = createAmpElement(); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleHide(invocation); expectAmpElementToHaveBeenHidden(element); }); @@ -120,7 +167,7 @@ describes.sandboxed('StandardActions', {}, () => { it('should handle normal element (toggle)', () => { const element = createElement(); toggle(element, false); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleShow(invocation); expectElementToHaveBeenShown(element); }); @@ -128,7 +175,7 @@ describes.sandboxed('StandardActions', {}, () => { it('should handle normal element (hidden attribute)', () => { const element = createElement(); element.setAttribute('hidden', ''); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleShow(invocation); expectElementToHaveBeenShown(element); }); @@ -136,7 +183,7 @@ describes.sandboxed('StandardActions', {}, () => { it('should handle AmpElement (toggle)', () => { const element = createAmpElement(); toggle(element, false); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleShow(invocation); expectAmpElementToHaveBeenShown(element); }); @@ -144,18 +191,130 @@ describes.sandboxed('StandardActions', {}, () => { it('should handle AmpElement (hidden attribute)', () => { const element = createAmpElement(); element.setAttribute('hidden', ''); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleShow(invocation); expectAmpElementToHaveBeenShown(element); }); + describe('Safari force sync', () => { + + let html; + beforeEach(() => { + html = htmlFor(document); + stubIsSafari(); + }); + + it('executes asynchronously when not autofocus (wrapped)', () => { + const node = html`
`; + standardActions.handleShow(trustedInvocation({node})); + expectElementToHaveBeenShown(node, /* sync */ false); + }); + + it('executes asynchronously when autofocus (direct)', () => { + const node = html``; + standardActions.handleShow(trustedInvocation({node})); + expectElementToHaveBeenShown(node, /* sync */ false); + }); + + it('executes synchronously when autofocus (wrapped)', () => { + const node = html`
`; + standardActions.handleShow(trustedInvocation({node})); + expectElementToHaveBeenShown(node, /* sync */ true); + }); + + it('executes synchronously when autofocus (direct)', () => { + const node = html``; + standardActions.handleShow(trustedInvocation({node})); + expectElementToHaveBeenShown(node, /* sync */ true); + }); + + }); + + describe('autofocus', () => { + + let html; + beforeEach(() => { + html = htmlFor(document); + stubIsSafari(); + }); + + describe('Safari force sync', () => { + + it('focuses [autofocus] element synchronously (direct)', () => { + const node = html``; + node.focus = sandbox.spy(); + + standardActions.handleShow(trustedInvocation({node})); + + expect(mutateElementStub).to.not.have.been.called; + expect(node.focus).to.have.been.calledOnce; + }); + + it('focuses [autofocus] element synchronously (wrapped)', () => { + const wrapper = html`
`; + const node = html``; + node.focus = sandbox.spy(); + + wrapper.firstElementChild.appendChild(node); + standardActions.handleShow(trustedInvocation({node: wrapper})); + + expect(mutateElementStub).to.not.have.been.called; + expect(node.focus).to.have.been.calledOnce; + }); + + it('does not focus element', () => { + const node = html``; + node.focus = sandbox.spy(); + + standardActions.handleShow(trustedInvocation({node})); + + expect(mutateElementStub).to.not.have.been.called; + expect(node.focus).to.not.have.been.called; + }); + + }); + + it('focuses [autofocus] element asynchronously (direct)', () => { + const node = html``; + node.focus = sandbox.spy(); + + standardActions.handleShow(trustedInvocation({node})); + + expectElementMutatedAsync(node); + expect(node.focus).to.have.been.calledOnce; + }); + + it('focuses [autofocus] element asynchronously (wrapped)', () => { + const wrapper = html`
`; + const node = html``; + node.focus = sandbox.spy(); + + wrapper.firstElementChild.appendChild(node); + standardActions.handleShow(trustedInvocation({node: wrapper})); + + expectElementMutatedAsync(wrapper); + expect(node.focus).to.have.been.calledOnce; + }); + + it('does not focus element', () => { + const node = html``; + node.focus = sandbox.spy(); + + standardActions.handleShow(trustedInvocation({node})); + + expectElementMutatedAsync(node); + expect(node.focus).to.not.have.been.called; + }); + + }); + }); describe('"toggle" action', () => { it('should show normal element when hidden (toggle)', () => { const element = createElement(); toggle(element, false); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleToggle(invocation); expectElementToHaveBeenShown(element); }); @@ -163,14 +322,14 @@ describes.sandboxed('StandardActions', {}, () => { it('should show normal element when hidden (hidden attribute)', () => { const element = createElement(); element.setAttribute('hidden', ''); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleToggle(invocation); expectElementToHaveBeenShown(element); }); it('should hide normal element when shown', () => { const element = createElement(); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleToggle(invocation); expectElementToHaveBeenHidden(element); }); @@ -178,7 +337,7 @@ describes.sandboxed('StandardActions', {}, () => { it('should show AmpElement when hidden (toggle)', () => { const element = createAmpElement(); toggle(element, false); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleToggle(invocation); expectAmpElementToHaveBeenShown(element); }); @@ -186,14 +345,14 @@ describes.sandboxed('StandardActions', {}, () => { it('should show AmpElement when hidden (hidden attribute)', () => { const element = createAmpElement(); element.setAttribute('hidden', ''); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleToggle(invocation); expectAmpElementToHaveBeenShown(element); }); it('should hide AmpElement when shown', () => { const element = createAmpElement(); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleToggle(invocation); expectAmpElementToHaveBeenHidden(element); }); @@ -285,14 +444,14 @@ describes.sandboxed('StandardActions', {}, () => { describe('"scrollTo" action', () => { it('should handle normal element', () => { const element = createElement(); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleScrollTo(invocation); expectAmpElementToHaveBeenScrolledIntoView(element); }); it('should handle AmpElement', () => { const element = createAmpElement(); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); standardActions.handleScrollTo(invocation); expectAmpElementToHaveBeenScrolledIntoView(element); }); @@ -301,7 +460,7 @@ describes.sandboxed('StandardActions', {}, () => { describe('"focus" action', () => { it('should handle normal element', () => { const element = createElement(); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); const focusStub = sandbox.stub(element, 'focus'); standardActions.handleFocus(invocation); expect(focusStub).to.be.calledOnce; @@ -309,7 +468,7 @@ describes.sandboxed('StandardActions', {}, () => { it('should handle AmpElement', () => { const element = createAmpElement(); - const invocation = {node: element, satisfiesTrust: () => true}; + const invocation = trustedInvocation({node: element}); const focusStub = sandbox.stub(element, 'focus'); standardActions.handleFocus(invocation); expect(focusStub).to.be.calledOnce; From 27b5f705bca45230e29e4ce5e2bb189e1c7c713a Mon Sep 17 00:00:00 2001 From: Alan Orozco Date: Tue, 5 Feb 2019 11:18:59 -0800 Subject: [PATCH 2/8] minimum markup --- test/manual/autofocus-on-show.html | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/test/manual/autofocus-on-show.html b/test/manual/autofocus-on-show.html index 7a75062b5298..60fa1fc1c31d 100644 --- a/test/manual/autofocus-on-show.html +++ b/test/manual/autofocus-on-show.html @@ -73,19 +73,13 @@ - - -