Switch toggle to hidden attribute (#17988)

* Switch toggle to hidden attribute

The hidden attribute has a few advantages:

- It does not rely on inline styles
  - Toggling from `display: none` back and forth to the initial
  `display: inline-block` is a snap regardless of whether that
  `inline-block` is an inline style or a CSS style.
  - We do not suffer from Safari's `!important` [bugs][1].
- It's easy to detect mutations to `[hidden]`, making #17475 possible.

[1]: #14178 (comment)

* Tests

* Use toggle helper

* Lint

* Update tests to use computed display

* lint

* fixes

* Update tests

* Remove debugger

* Update assertion message

* Directly test hidden attribute when element in in DOM tree

If it's not in the DOM tree, `getComputedStyle` won't work.

* Update Google ads to use toggle

* Fix fixed-layer test

* Revert "Update Google ads to use toggle"

This reverts commit 5ead2de.

* Fix mock hasAttribute impl

* Lint ads/google/a4a as 1p code

* Undo changes to ima-video tests

* Fix test

ads/google/a4a/.eslintrc

@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "amphtml-internal/no-style-display": 2,
+  }
+}

build-system/eslint-rules/no-style-display.js

@@ -49,7 +49,7 @@ module.exports = function(context) {
         }
 
         return context.report({
-          node: arg || node,
+          node: arg,
           message: 'property argument (the second argument) to setStyle must be a string literal',
         });
       }
@@ -79,7 +79,7 @@ module.exports = function(context) {
         }
 
         return context.report({
-          node: arg || node,
+          node: arg,
           message: `styles argument (the second argument) to ${callName} must be an object literal. You may also pass in an explicit call to assertDoesNotContainDisplay`,
         });
       }
@@ -116,7 +116,7 @@ module.exports = function(context) {
 
       if (arg.type !== 'ArrayExpression') {
         return context.report({
-          node: arg || node,
+          node: arg,
           message: `styles argument (the second argument) to resetStyles must be an array literal`,
         });
       }

css/amp.css

@@ -51,7 +51,13 @@ body {
   text-size-adjust: 100%;
 }
 
+/**
+ * We intentionally break with HTML5's default [hidden] styling to apply
+ * !important.
+ */
 [hidden] {
+  /* This must be !important, else the toggle helper system would break down
+     due to specificity */
   display: none !important;
 }
 

extensions/amp-ad/0.1/test/test-amp-ad-3p-impl.js

@@ -94,7 +94,7 @@ describes.realWin('amp-ad-3p-impl', {
         expect(iframe.tagName).to.equal('IFRAME');
         const url = iframe.getAttribute('src');
         expect(url).to.match(/^http:\/\/ads.localhost:/);
-        expect(iframe.style.display).to.equal('');
+        expect(iframe).to.not.have.attribute('hidden');
 
         expect(url).to.match(/frame(.max)?.html/);
         const data = JSON.parse(iframe.name).attributes;

extensions/amp-analytics/0.1/test/test-amp-analytics.js

@@ -159,10 +159,10 @@ describes.realWin('amp-analytics', {
   }
 
   function waitForSendRequest(analytics, opt_max, opt_cnt) {
-    expect(analytics.element.style.display).to.equal('');
+    expect(analytics.element).to.not.have.display('none');
     const callCount = opt_cnt || 0;
     return analytics.layoutCallback().then(() => {
-      expect(analytics.element.style.display).to.equal('none');
+      expect(analytics.element).to.have.display('none');
       if (sendRequestSpy.callCount > callCount) {
         return;
       }
@@ -428,7 +428,7 @@ describes.realWin('amp-analytics', {
     analytics.buildCallback();
     const iniPromise = analytics.iniPromise_;
     expect(iniPromise).to.be.ok;
-    expect(el.style.display).to.equal('none');
+    expect(el).to.have.attribute('hidden');
     // Viewer.whenFirstVisible is the first blocking call to initialize.
     expect(whenFirstVisibleStub).to.be.calledOnce;
 

extensions/amp-animation/0.1/test/test-amp-animation.js

@@ -117,7 +117,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
         expect(anim.element.style['left']).to.equal('0px');
         expect(anim.element.style['width']).to.equal('1px');
         expect(anim.element.style['height']).to.equal('1px');
-        expect(anim.element.style['display']).to.equal('block');
+        expect(anim.element).to.have.display('block');
       });
     });
 

extensions/amp-app-banner/0.1/test/test-amp-app-banner.js

@@ -109,7 +109,7 @@ describes.realWin('amp-app-banner', {
     return getAppBanner({iosMeta, androidManifest}).then(banner => {
       return banner.implementation_.isDismissed().then(() => {
         expect(banner.parentElement).to.not.be.null;
-        expect(banner.style.display).to.be.equal('');
+        expect(banner).to.not.have.display('none');
         const bannerTop = banner.querySelector(
             'i-amphtml-app-banner-top-padding');
         expect(bannerTop).to.exist;
@@ -123,7 +123,6 @@ describes.realWin('amp-app-banner', {
   function testRemoveBanner(config = {iosMeta, androidManifest}) {
     return getAppBanner(config).then(banner => {
       expect(banner.parentElement).to.be.null;
-      expect(banner.style.display).to.be.equal('none');
     });
   }
 

extensions/amp-byside-content/0.1/test/test-amp-byside-content.js

@@ -134,7 +134,7 @@ describes.realWin('amp-byside-content', {
       const placeholder = elem.querySelector('[placeholder]');
       const iframe = elem.querySelector('iframe');
       expect(iframe).to.be.null;
-      expect(placeholder.style.display).to.be.equal('');
+      expect(placeholder).to.not.have.display('none');
     }).then(elem => {
       const placeholder = elem.querySelector('[placeholder]');
       elem.getVsync = () => {
@@ -148,7 +148,7 @@ describes.realWin('amp-byside-content', {
 
       // test placeholder too
       elem.implementation_.iframePromise_.then(() => {
-        expect(placeholder.style.display).to.be.equal('none');
+        expect(placeholder).to.have.display('none');
       });
     });
   });

extensions/amp-carousel/0.1/test/test-scrollable-carousel.js

@@ -78,9 +78,8 @@ describes.realWin('test-scrollable-carousel', {
       // build child slides
       const carouselSlideEls =
         container.getElementsByClassName('amp-carousel-slide');
-      const slideStyle = win.getComputedStyle(carouselSlideEls[0], null);
       expect(carouselSlideEls.length).to.equal(7);
-      expect(slideStyle.getPropertyValue('display')).to.equal('inline-block');
+      expect(carouselSlideEls[0]).to.have.display('inline-block');
 
       // show control buttons correctly
       expect(impl.hasPrev()).to.be.false;

extensions/amp-consent/0.1/test/test-amp-consent.js

@@ -20,7 +20,6 @@ import {
 } from '../amp-consent';
 import {CONSENT_ITEM_STATE} from '../consent-state-manager';
 import {CONSENT_POLICY_STATE} from '../../../../src/consent-state';
-import {computedStyle} from '../../../../src/style';
 import {dict} from '../../../../src/utils/object';
 import {elementByTag} from '../../../../src/dom';
 import {macroTask} from '../../../../testing/yield';
@@ -650,21 +649,16 @@ describes.realWin('amp-consent', {
         ampConsent.buildCallback();
         ampConsent.element.classList.remove('i-amphtml-notbuilt');
         expect(ampConsent.postPromptUI_).to.not.be.null;
-        expect(computedStyle(ampConsent.win, ampConsent.element)['display'])
-            .to.equal('none');
-        expect(computedStyle(ampConsent.win, ampConsent.postPromptUI_)
-            ['display']).to.equal('none');
+        expect(ampConsent.element).to.have.display('none');
+        expect(ampConsent.postPromptUI_).to.have.display('none');
         yield macroTask();
 
-        expect(computedStyle(ampConsent.win, ampConsent.element)['display'])
-            .to.not.equal('none');
+        expect(ampConsent.element).to.not.have.display('none');
         expect(ampConsent.element.classList.contains('amp-active')).to.be.true;
         expect(ampConsent.element.classList.contains('amp-hidden')).to.be.false;
-        expect(computedStyle(ampConsent.win, ampConsent.postPromptUI_)
-            ['display']).to.not.equal('none');
+        expect(ampConsent.postPromptUI_).to.not.have.display('none');
         ampConsent.scheduleDisplay_('ABC');
-        expect(computedStyle(ampConsent.win, ampConsent.postPromptUI_)
-            ['display']).to.equal('none');
+        expect(ampConsent.postPromptUI_).to.have.display('none');
       });
 
       describe('hide/show postPromptUI', () => {
@@ -692,8 +686,7 @@ describes.realWin('amp-consent', {
           ampConsent.element.classList.remove('i-amphtml-notbuilt');
           expect(ampConsent.postPromptUI_).to.not.be.null;
           yield macroTask();
-          expect(computedStyle(ampConsent.win, ampConsent.postPromptUI_)
-              ['display']).to.equal('none');
+          expect(ampConsent.postPromptUI_).to.have.display('none');
         });
 
         it('show postPromptUI', function* () {
@@ -704,8 +697,7 @@ describes.realWin('amp-consent', {
           ampConsent.element.classList.remove('i-amphtml-notbuilt');
           expect(ampConsent.postPromptUI_).to.not.be.null;
           yield macroTask();
-          expect(computedStyle(ampConsent.win, ampConsent.postPromptUI_)
-              ['display']).to.not.equal('none');
+          expect(ampConsent.postPromptUI_).to.not.have.display('none');
         });
       });
     });

extensions/amp-form/0.1/test/test-validation-bubble.js

@@ -57,6 +57,6 @@ describes.realWin('validation-bubble', {amp: true}, env => {
     expect(bubbleEl.style.left).to.equal('500px');
 
     bubble.hide();
-    expect(bubbleEl.style.display).to.equal('none');
+    expect(bubbleEl).to.have.display('none');
   });
 });

extensions/amp-image-lightbox/0.1/test/integration/test-amp-image-lightbox.js

@@ -51,7 +51,7 @@ describe.configure().run('amp-image-lightbox', function() {
 
     it('should activate on tap of source image', () => {
       const lightbox = win.document.getElementById('image-lightbox-1');
-      expect(lightbox.style.display).to.equal('none');
+      expect(lightbox).to.have.display('none');
       const ampImage = win.document.getElementById('img0');
       const imageLoadedPromise = waitForImageToLoad(ampImage);
       return imageLoadedPromise.then(() => {
@@ -76,7 +76,7 @@ describe.configure().run('amp-image-lightbox', function() {
 function waitForLightboxOpen(document) {
   return poll('wait for image-lightbox-1 to open', () => {
     const lightbox = document.getElementById('image-lightbox-1');
-    return lightbox.style.display == '';
+    return getComputedStyle(lightbox).display != 'none';
   });
 }
 

extensions/amp-instagram/0.1/test/test-amp-instagram.js

@@ -115,7 +115,7 @@ describes.realWin('amp-instagram', {
       const placeholder = ins.querySelector('[placeholder]');
       const iframe = ins.querySelector('iframe');
       expect(iframe).to.be.null;
-      expect(placeholder.style.display).to.be.equal('');
+      expect(placeholder).to.not.have.display('');
       testImage(placeholder.querySelector('amp-img'));
     }).then(ins => {
       const placeholder = ins.querySelector('[placeholder]');
@@ -128,7 +128,7 @@ describes.realWin('amp-instagram', {
       testIframe(iframe);
       testImage(placeholder.querySelector('amp-img'));
       ins.implementation_.iframePromise_.then(() => {
-        expect(placeholder.style.display).to.be.equal('none');
+        expect(placeholder).to.be.have.display('none');
       });
     });
   });
@@ -142,7 +142,7 @@ describes.realWin('amp-instagram', {
       expect(ins.querySelector('iframe')).to.be.null;
       expect(obj.iframe_).to.be.null;
       expect(obj.iframePromise_).to.be.null;
-      expect(placeholder.style.display).to.be.equal('');
+      expect(placeholder).to.not.have.display('none');
     });
   });
 

extensions/amp-install-serviceworker/0.1/test/test-amp-install-serviceworker.js

@@ -250,7 +250,7 @@ describes.realWin('amp-install-serviceworker', {
         const returnedValue = fn();
         expect(iframe).to.exist;
         expect(calledSrc).to.undefined;
-        expect(install.style.display).to.equal('none');
+        expect(install).to.have.display('none');
         expect(iframe.tagName).to.equal('IFRAME');
         expect(iframe.getAttribute('sandbox')).to.equal(
             'allow-same-origin allow-scripts');
@@ -439,7 +439,7 @@ describes.fakeWin('url rewriter', {
       const iframe = element.querySelector('iframe');
       expect(iframe).to.exist;
       expect(iframe.src).to.equal('https://example.com/shell#preload');
-      expect(iframe.style.display).to.equal('none');
+      expect(iframe).to.have.attribute('hidden');
       expect(iframe.getAttribute('sandbox'))
           .to.equal('allow-scripts allow-same-origin');
     });

extensions/amp-lightbox-gallery/0.1/test/integration/test-amp-lightbox-gallery.js

@@ -52,7 +52,7 @@ describe.configure().skip('amp-lightbox-gallery', function() {
     it('should open and close correctly', () => {
       const lightbox = win.document.getElementById('amp-lightbox-gallery');
       return openLightbox(win.document).then(() => {
-        expect(lightbox.style.display).to.not.equal('none');
+        expect(lightbox).to.not.have.display('none');
         const carouselQuery = lightbox.getElementsByTagName('AMP-CAROUSEL');
         expect(carouselQuery.length).to.equal(1);
         const carousel = carouselQuery[0];
@@ -69,7 +69,7 @@ describe.configure().skip('amp-lightbox-gallery', function() {
         closeButton.click();
         return lightboxClose;
       }).then(() => {
-        expect(lightbox.style.display).to.equal('none');
+        expect(lightbox).to.have.display('none');
       });
     });
 
@@ -85,20 +85,20 @@ describe.configure().skip('amp-lightbox-gallery', function() {
         const closeButton = getButton(win.document,
             'i-amphtml-lbg-button-close');
         expect(closeButton.getAttribute('aria-label')).to.equal('Close');
-        expect(win.getComputedStyle(closeButton).display).to.equal('block');
+        expect(closeButton).to.have.display('block');
 
         const galleryButton = getButton(win.document,
             'i-amphtml-lbg-button-gallery');
         expect(galleryButton.getAttribute('aria-label')).to.equal('Gallery');
-        expect(win.getComputedStyle(galleryButton).display).to.equal('none');
+        expect(galleryButton).to.have.display('none');
 
         const prevButton = getButton(win.document, 'i-amphtml-lbg-button-prev');
         expect(prevButton.getAttribute('aria-label')).to.equal('Prev');
-        expect(win.getComputedStyle(prevButton).display).to.equal('none');
+        expect(prevButton).to.have.display('none');
 
         const nextButton = getButton(win.document, 'i-amphtml-lbg-button-next');
         expect(nextButton.getAttribute('aria-label')).to.equal('Next');
-        expect(win.getComputedStyle(nextButton).display).to.equal('none');
+        expect(nextButton).to.have.display('none');
       });
     });
 
@@ -201,7 +201,7 @@ describe.configure().skip('amp-lightbox-gallery', function() {
 
 function openLightbox(document) {
   const lightbox = document.getElementById('amp-lightbox-gallery');
-  expect(lightbox.style.display).to.equal('none');
+  expect(lightbox).to.have.display('none');
   const ampImage = document.getElementById('img0');
   const imageLoadedPromise = waitForImageToLoad(ampImage);
   return imageLoadedPromise.then(() => {
@@ -223,13 +223,15 @@ function getButton(document, className) {
 
 function waitForLightboxOpen(lightbox) {
   return poll('wait for amp-lightbox-gallery to open', () => {
-    return lightbox.style.display == '' && lightbox.style.opacity == '';
+    const styles = lightbox.ownerNode.defaultView.getComputedStyle(lightbox);
+    return styles.display != 'none' && lightbox.style.opacity == '';
   });
 }
 
 function waitForLightboxClose(lightbox, carousel) {
   return poll('wait for amp-lightbox-gallery to close', () => {
-    return carousel.style.display == 'none';
+    const styles = carousel.ownerNode.defaultView.getComputedStyle(carousel);
+    return styles.display == 'none';
   });
 }
 

extensions/amp-playbuzz/0.1/test/test-amp-playbuzz.js

@@ -158,7 +158,7 @@ describes.realWin('amp-playbuzz', {
       const placeholder = ins.querySelector('[placeholder]');
       const iframe = ins.querySelector('iframe');
       expect(iframe).to.be.null;
-      expect(placeholder.style.display).to.be.equal('');
+      expect(placeholder).to.not.have.display('');
       expect(placeholder.getAttribute('aria-label'))
           .to.equal('Loading interactive element');
     }).then(ins => {
@@ -173,7 +173,7 @@ describes.realWin('amp-playbuzz', {
       testIframe(iframe, '//www.playbuzz.com/bob/bobs-life');
       //Should test placeholder too
       ins.implementation_.iframePromise_.then(() => {
-        expect(placeholder.style.display).to.be.equal('none');
+        expect(placeholder).to.have.display('none');
       });
     });
   });

extensions/amp-recaptcha-input/0.1/test/test-amp-recaptcha-input.js

@@ -75,8 +75,7 @@ describes.realWin('amp-recaptcha-input', {
 
     it('Should be visible after built', () => {
       return getRecaptchaInput().then(ampRecaptchaInput => {
-        expect(win.getComputedStyle(ampRecaptchaInput).display)
-            .to.not.equal('none');
+        expect(ampRecaptchaInput).to.not.have.display('none');
       });
     });
 

extensions/amp-sidebar/0.1/test/integration/test-amp-sidebar.js

@@ -99,13 +99,15 @@ describe.configure().skipSafari().skipEdge().run('amp-sidebar', function() {
 function waitForSidebarOpen(document) {
   return poll('wait for sidebar to open', () => {
     const dummy = document.getElementById('dummy');
-    return dummy.style.display == 'none';
+    const styles = document.defaultView.getComputedStyle(dummy);
+    return styles.display == 'none';
   });
 }
 
 function waitForSidebarClose(document) {
   return poll('wait for sidebar to open', () => {
     const dummy = document.getElementById('dummy');
-    return dummy.style.display == '';
+    const styles = document.defaultView.getComputedStyle(dummy);
+    return styles.display != 'none';
   });
 }