From f2a361651b4b4d1d484c6cd9502c895695545d7b Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Thu, 15 Jun 2017 13:05:56 -0700 Subject: [PATCH] Needs careful review: Fix a few and introduce new FOUC for amp-accordion (#9945) - Makes amp-accordion not be render blocking. - Implements basic expand logic in CSS only - Moves the CSS into the main CSS. With the extension being so popular I think this is a good trade off. Avoids a major FOUC source. - The `mutateElement` based changing of DOM was actually wrong here. Caused FOUC. Removed. - `sessionStorage` based change of state may now cause FOUC, but this should be very rare in practice since `sessionStorage` only has contents if AMP is actually in cache. --- css/amp.css | 46 +++++++++++++ .../amp-accordion/0.1/amp-accordion.css | 67 ------------------- extensions/amp-accordion/0.1/amp-accordion.js | 54 ++++++++------- .../0.1/test/test-amp-accordion.js | 2 + gulpfile.js | 2 +- src/render-delaying-services.js | 1 - .../test-render-delaying-services.js | 8 +-- 7 files changed, 80 insertions(+), 100 deletions(-) delete mode 100644 extensions/amp-accordion/0.1/amp-accordion.css diff --git a/css/amp.css b/css/amp.css index 22deb1cf51c0..0794e32060b6 100644 --- a/css/amp.css +++ b/css/amp.css @@ -683,3 +683,49 @@ i-amphtml-video-mask, i-amp-video-mask { 0% {transform: translateY(100%);} 100% {transform: translateY(0);} } + +/** + * amp-accordion to avoid FOUC. + */ + +/* Non-overridable properties */ +amp-accordion { + display: block !important; +} + +/* Make sections non-floatable */ +amp-accordion > section { + float: none !important; +} + +/* Display the first 2 elements (heading and content) */ +amp-accordion > section > * { + float: none !important; + display: block !important; + overflow: hidden !important; /* clearfix */ + position: relative !important; +} + +amp-accordion, +amp-accordion > section, +amp-accordion > section > * { + margin: 0; +} + +/* heading element*/ +amp-accordion > section > :first-child { + cursor: pointer; + background-color: #efefef; + padding-right: 20px; + border: solid 1px #dfdfdf; +} + +/* Collapse content by default. */ +amp-accordion > section > :last-child { + display: none !important; +} + +/* Expand content when needed. */ +amp-accordion > section[expanded] > :last-child { + display: block !important; +} diff --git a/extensions/amp-accordion/0.1/amp-accordion.css b/extensions/amp-accordion/0.1/amp-accordion.css deleted file mode 100644 index 7d6754f2ad00..000000000000 --- a/extensions/amp-accordion/0.1/amp-accordion.css +++ /dev/null @@ -1,67 +0,0 @@ -/** - * Copyright 2016 The AMP HTML Authors. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS-IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -/* Non-overridable properties */ -amp-accordion { - display: block !important; -} - -/* Make sections non-floatable */ -amp-accordion > section { - float: none !important; -} - -/* Hide all children and make them non-floatable */ -amp-accordion > section > *:nth-child(n) { - display: none !important; - float: none !important; -} - -/* Display the first 2 elements (heading and content) */ -amp-accordion > section > .i-amphtml-accordion-header, -amp-accordion > section > .i-amphtml-accordion-content { - display: block !important; - overflow: hidden !important; /* clearfix */ - position: relative !important; -} - -amp-accordion, -amp-accordion > section, -.i-amphtml-accordion-header, -.i-amphtml-accordion-content { - margin: 0; -} - - - -/* heading element*/ -.i-amphtml-accordion-header { - cursor: pointer; - background-color: #efefef; - padding-right: 20px; - border: solid 1px #dfdfdf; -} - -/* Collapse content by default. */ -amp-accordion > section > .i-amphtml-accordion-content { - display: none !important; -} - -/* Expand content when needed. */ -amp-accordion > section[expanded] > .i-amphtml-accordion-content { - display: block !important; -} - diff --git a/extensions/amp-accordion/0.1/amp-accordion.js b/extensions/amp-accordion/0.1/amp-accordion.js index a7eb22d2fbfc..3a9951092a15 100644 --- a/extensions/amp-accordion/0.1/amp-accordion.js +++ b/extensions/amp-accordion/0.1/amp-accordion.js @@ -14,7 +14,6 @@ * limitations under the License. */ -import {CSS} from '../../../build/amp-accordion-0.1.css'; import {KeyCodes} from '../../../src/utils/key-codes'; import {Layout} from '../../../src/layout'; import {closest} from '../../../src/dom'; @@ -69,34 +68,41 @@ class AmpAccordion extends AMP.BaseElement { 'Each section must have exactly two children. ' + 'See https://github.com/ampproject/amphtml/blob/master/extensions/' + 'amp-accordion/amp-accordion.md. Found in: %s', this.element); - this.mutateElement(() => { - const content = sectionComponents[1]; - content.classList.add('i-amphtml-accordion-content'); - let contentId = content.getAttribute('id'); - if (!contentId) { - contentId = this.element.id + '_AMP_content_' + index; - content.setAttribute('id', contentId); - } + const content = sectionComponents[1]; + let contentId = content.getAttribute('id'); + if (!contentId) { + contentId = this.element.id + '_AMP_content_' + index; + content.setAttribute('id', contentId); + } + + + if (!this.currentState_[contentId] != null) { if (this.currentState_[contentId]) { section.setAttribute('expanded', ''); - } else if (this.currentState_[contentId] == false) { + } else if (this.currentState_[contentId] === false) { section.removeAttribute('expanded'); } + this.mutateElement(() => { + // Just mark this element as dirty since we changed the state + // based on runtime state. This triggers checking again + // whether children need layout. + // See https://github.com/ampproject/amphtml/issues/3586 + // for details. + }); + } - const header = sectionComponents[0]; - header.classList.add('i-amphtml-accordion-header'); - header.setAttribute('role', 'heading'); - header.setAttribute('aria-controls', contentId); - header.setAttribute('aria-expanded', - section.hasAttribute('expanded').toString()); - if (!header.hasAttribute('tabindex')) { - header.setAttribute('tabindex', 0); - } - this.headers_.push(header); - header.addEventListener('click', this.clickHandler_.bind(this)); - header.addEventListener('keydown', this.keyDownHandler_.bind(this)); - }); + const header = sectionComponents[0]; + header.setAttribute('role', 'heading'); + header.setAttribute('aria-controls', contentId); + header.setAttribute('aria-expanded', + section.hasAttribute('expanded').toString()); + if (!header.hasAttribute('tabindex')) { + header.setAttribute('tabindex', 0); + } + this.headers_.push(header); + header.addEventListener('click', this.clickHandler_.bind(this)); + header.addEventListener('keydown', this.keyDownHandler_.bind(this)); }); } @@ -244,4 +250,4 @@ class AmpAccordion extends AMP.BaseElement { } -AMP.registerElement('amp-accordion', AmpAccordion, CSS); +AMP.registerElement('amp-accordion', AmpAccordion); diff --git a/extensions/amp-accordion/0.1/test/test-amp-accordion.js b/extensions/amp-accordion/0.1/test/test-amp-accordion.js index 79bdfa640fba..a3cdde16f98f 100644 --- a/extensions/amp-accordion/0.1/test/test-amp-accordion.js +++ b/extensions/amp-accordion/0.1/test/test-amp-accordion.js @@ -193,6 +193,8 @@ describes.sandboxed('amp-accordion', {}, () => { 'section > *:first-child'); // Focus the first header, tryFocus(headerElements[0]); + expect(iframe.doc.activeElement) + .to.equal(headerElements[0]); const upArrowEvent = { keyCode: KeyCodes.UP_ARROW, target: headerElements[0], diff --git a/gulpfile.js b/gulpfile.js index 9f7850642651..8b694fe0608d 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -50,7 +50,7 @@ var extensionAliasFilePath = {}; declareExtension('amp-3q-player', '0.1', false, 'NO_TYPE_CHECK'); declareExtension('amp-access', '0.1', true, 'NO_TYPE_CHECK'); declareExtension('amp-access-laterpay', '0.1', true, 'NO_TYPE_CHECK'); -declareExtension('amp-accordion', '0.1', true); +declareExtension('amp-accordion', '0.1', false); declareExtension('amp-ad', '0.1', true); declareExtension('amp-ad-network-adsense-impl', 0.1, false); declareExtension('amp-ad-network-doubleclick-impl', 0.1, false); diff --git a/src/render-delaying-services.js b/src/render-delaying-services.js index 857e9d18ce08..bd6fff350dee 100644 --- a/src/render-delaying-services.js +++ b/src/render-delaying-services.js @@ -37,7 +37,6 @@ import {timerFor} from './services'; * @const {!Object} */ const SERVICES = { - 'amp-accordion': '[custom-element=amp-accordion]', 'amp-dynamic-css-classes': '[custom-element=amp-dynamic-css-classes]', 'variant': 'amp-experiment', }; diff --git a/test/functional/test-render-delaying-services.js b/test/functional/test-render-delaying-services.js index 8cad8b0103d0..5047a5cf54d2 100644 --- a/test/functional/test-render-delaying-services.js +++ b/test/functional/test-render-delaying-services.js @@ -28,7 +28,6 @@ describe('waitForServices', () => { let win; let sandbox; let clock; - let accordionResolve; let dynamicCssResolve; let experimentResolve; let variantResolve; @@ -36,7 +35,6 @@ describe('waitForServices', () => { beforeEach(() => { sandbox = sinon.sandbox.create(); const getService = sandbox.stub(service, 'getServicePromise'); - accordionResolve = waitForService(getService, 'amp-accordion'); dynamicCssResolve = waitForService(getService, 'amp-dynamic-css-classes'); experimentResolve = waitForService(getService, 'amp-experiment'); variantResolve = waitForService(getService, 'variant'); @@ -59,14 +57,12 @@ describe('waitForServices', () => { }); it('should timeout if some blocking services are missing', () => { - addExtensionScript(win, 'amp-accordion'); addExtensionScript(win, 'amp-dynamic-css-classes'); win.document.body.appendChild(win.document.createElement('amp-experiment')); expect(hasRenderDelayingServices(win)).to.be.true; addExtensionScript(win, 'non-blocking-extension'); const promise = waitForServices(win); - accordionResolve(); dynamicCssResolve(); experimentResolve(); // 'amp-experiment' is actually blocked by 'variant' clock.tick(3000); @@ -74,18 +70,16 @@ describe('waitForServices', () => { }); it('should resolve when all extensions are ready', () => { - addExtensionScript(win, 'amp-accordion'); addExtensionScript(win, 'amp-dynamic-css-classes'); win.document.body.appendChild(win.document.createElement('amp-experiment')); expect(hasRenderDelayingServices(win)).to.be.true; addExtensionScript(win, 'non-blocking-extension'); const promise = waitForServices(win); - accordionResolve(); dynamicCssResolve(); variantResolve(); // this unblocks 'amp-experiment' - return expect(promise).to.eventually.have.lengthOf(3); + return expect(promise).to.eventually.have.lengthOf(2); }); });