Needs careful review: Fix a few and introduce new FOUC for amp-accord…

…ion (#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

@@ -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;
+}

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

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],

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

src/render-delaying-services.js

@@ -37,7 +37,6 @@ import {timerFor} from './services';
  * @const {!Object<string, string>}
  */
 const SERVICES = {
-  'amp-accordion': '[custom-element=amp-accordion]',
   'amp-dynamic-css-classes': '[custom-element=amp-dynamic-css-classes]',
   'variant': 'amp-experiment',
 };

test/functional/test-render-delaying-services.js

@@ -28,15 +28,13 @@ describe('waitForServices', () => {
   let win;
   let sandbox;
   let clock;
-  let accordionResolve;
   let dynamicCssResolve;
   let experimentResolve;
   let variantResolve;
 
   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,33 +57,29 @@ 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);
     return expect(promise).to.eventually.be.rejectedWith('variant');
   });
 
   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);
   });
 });