Remove scrollable from amp-lightbox 1.0 (#33280)

* Remove scrollable * Move manual example to 1.0 (to test scrolling outside Storybook) * Don't need scrollable attr * Update zIndex * Remove comment in styles * Add overscroll-behavior: none
ampproject · Mar 24, 2021 · 653da6c · 653da6c
1 parent 632f3f8
commit 653da6c
Show file tree

Hide file tree

Showing 7 changed files with 17 additions and 78 deletions.
diff --git a/css/Z_INDEX.md b/css/Z_INDEX.md
@@ -60,7 +60,7 @@
 | `amp-image-lightbox`                                                                                      | 1000       | [extensions/amp-image-lightbox/0.1/amp-image-lightbox.css](/extensions/amp-image-lightbox/0.1/amp-image-lightbox.css)                                                   |
 | `amp-lightbox`                                                                                            | 1000       | [extensions/amp-lightbox/0.1/amp-lightbox.css](/extensions/amp-lightbox/0.1/amp-lightbox.css)                                                                           |
 | `i-amphtml-ad-close-header`                                                                               | 1000       | [extensions/amp-lightbox/0.1/amp-lightbox.css](/extensions/amp-lightbox/0.1/amp-lightbox.css)                                                                           |
-| `defaultStyles`                                                                                           | 1000       | [extensions/amp-lightbox/1.0/component.jss.js](/extensions/amp-lightbox/1.0/component.jss.js)                                                                           |
+| `wrapper`                                                                                                 | 1000       | [extensions/amp-lightbox/1.0/component.jss.js](/extensions/amp-lightbox/1.0/component.jss.js)                                                                           |
 | `amp-live-list > [update]`                                                                                | 1000       | [extensions/amp-live-list/0.1/amp-live-list.css](/extensions/amp-live-list/0.1/amp-live-list.css)                                                                       |
 | `.i-amphtml-mega-menu-mask-parent`                                                                        | 1000       | [extensions/amp-mega-menu/0.1/amp-mega-menu.css](/extensions/amp-mega-menu/0.1/amp-mega-menu.css)                                                                       |
 | `amp-mega-menu`                                                                                           | 1000       | [extensions/amp-mega-menu/0.1/amp-mega-menu.css](/extensions/amp-mega-menu/0.1/amp-mega-menu.css)                                                                       |

diff --git a/examples/amp-lightbox.amp.html b/examples/amp-lightbox.amp.html
@@ -35,19 +35,24 @@
       z-index: 10;
     }
   </style>
-  <script async custom-element="amp-lightbox" src="https://cdn.ampproject.org/v0/amp-lightbox-0.1.js"></script>
+  <script async custom-element="amp-lightbox" src="https://cdn.ampproject.org/v0/amp-lightbox-1.0.js"></script>
   <script async custom-element="amp-bind" src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"></script>
   <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
   <script async src="https://cdn.ampproject.org/v0.js"></script>
   <script async custom-element="amp-video" src="https://cdn.ampproject.org/v0/amp-video-0.1.js"></script>
+  <script>
+    (self.AMP = self.AMP || []).push(function (AMP) {
+      AMP.toggleExperiment('bento', true);
+    });
+  </script>
 </head>
 <body class="comic-amp-font-loading">
 
 <article>
   <span class="fixed" style="background-color: red">Fixed header</span>
 
   <h2>Scrollable Lightbox</h2>
-  <amp-lightbox scrollable id="lightbox-scrollable" layout="nodisplay" animate-in="fly-in-bottom">
+  <amp-lightbox id="lightbox-scrollable" layout="nodisplay" animate-in="fly-in-bottom">
     <button on="tap:lightbox-scrollable.close" class="lightbox-close-button">
       Close Scrollable Lightbox
     </button>

diff --git a/extensions/amp-lightbox/1.0/base-element.js b/extensions/amp-lightbox/1.0/base-element.js
@@ -73,7 +73,6 @@ BaseElement['props'] = {
   'closeButton': {selector: '[slot="close-button"]', single: true},
   'children': {passthrough: true},
   'id': {attr: 'id'},
-  'scrollable': {attr: 'scrollable', type: 'boolean'},
 };
 
 /** @override */

diff --git a/extensions/amp-lightbox/1.0/component.js b/extensions/amp-lightbox/1.0/component.js
@@ -58,7 +58,6 @@ function LightboxWithRef(
     closeButtonAs,
     onBeforeOpen,
     onAfterClose,
-    scrollable = false,
     ...rest
   },
   ref
@@ -158,18 +157,10 @@ function LightboxWithRef(
         layout={true}
         paint={true}
         part="lightbox"
-        contentStyle={
-          // Prefer style over class to override `ContainWrapper`'s overflow
-          scrollable && {
-            overflow: 'scroll',
-            overscrollBehavior: 'none',
-          }
-        }
-        wrapperClassName={`${classes.defaultStyles} ${classes.wrapper} ${
-          scrollable ? '' : classes.containScroll
-        }`}
+        contentClassName={classes.content}
+        wrapperClassName={classes.wrapper}
         role="dialog"
-        tabindex="0"
+        tabIndex="0"
         onKeyDown={(event) => {
           if (event.key === Keys.ESCAPE) {
             setVisible(false);

diff --git a/extensions/amp-lightbox/1.0/component.jss.js b/extensions/amp-lightbox/1.0/component.jss.js
@@ -46,31 +46,20 @@ const wrapper = {
   height: '100%',
   position: 'fixed',
   boxSizing: 'border-box',
-};
-
-// User overridable styles
-const defaultStyles = {
   zIndex: 1000,
   backgroundColor: 'rgba(0, 0, 0, 0.9)',
   color: '#fff',
 };
 
-/* 
-  overflow: hidden does not trigger overscrollBehavior, so 
-  overflow: auto is applied even though the lightbox should not scroll.
-  We rely instead on the overflow: hidden in ContainWrapper to prevent 
-  scrolling inside the lightbox.
-*/
-const containScroll = {
-  overflow: 'auto', // Prevent scrolling inside lightbox.
-  overscrollBehavior: 'none', // Prevent scrolling outside lightbox.
+const content = {
+  overflow: 'auto !important',
+  overscrollBehavior: 'none !important',
 };
 
 const JSS = {
   closeButton,
   wrapper,
-  defaultStyles,
-  containScroll,
+  content,
 };
 
 // useStyles gets replaced for AMP builds via `babel-plugin-transform-jss`.

diff --git a/extensions/amp-lightbox/1.0/storybook/Basic.amp.js b/extensions/amp-lightbox/1.0/storybook/Basic.amp.js
@@ -59,15 +59,14 @@ export const Default = () => {
   );
 };
 
-export const scrollable = () => {
+export const overflowAuto = () => {
   const animation = select('animation', [
     'fade-in',
     'fly-in-top',
     'fly-in-bottom',
   ]);
   const backgroundColor = text('background color', 'rgba(0,0,0,0.5)');
   const color = text('font color', '');
-  const scrollable = boolean('scrollable', true);
   const lotsOfText = boolean('lots of text?', true);
   return (
     <>
@@ -78,12 +77,7 @@ export const scrollable = () => {
         }
       `}</style>
       <div style="height: 300px;">
-        <amp-lightbox
-          id="lightbox"
-          layout="nodisplay"
-          animation={animation}
-          scrollable={scrollable}
-        >
+        <amp-lightbox id="lightbox" layout="nodisplay" animation={animation}>
           <p>
             Dessert tootsie roll marzipan pastry. Powder powder jelly beans
             chocolate bar candy sugar plum. Jelly-o gummi bears jelly icing

diff --git a/extensions/amp-lightbox/1.0/test/test-component.js b/extensions/amp-lightbox/1.0/test/test-component.js
@@ -35,15 +35,6 @@ describes.sandboxed('Lightbox preact component v1.0', {}, () => {
 
     // Render provided children
     expect(wrapper.children()).to.have.lengthOf(1);
-    expect(
-      wrapper
-        .find('div[part="lightbox"]')
-        .getDOMNode()
-        .className.includes('contain-scroll')
-    ).to.be.true;
-    expect(
-      wrapper.find('div[part="lightbox"] > div').getDOMNode().style.overflow
-    ).to.equal('hidden');
     expect(wrapper.find('p').text()).to.equal('Hello World');
 
     // Default SR button is present
@@ -54,36 +45,6 @@ describes.sandboxed('Lightbox preact component v1.0', {}, () => {
     expect(closeButton.textContent).to.equal('');
   });
 
-  it('renders scrollable', () => {
-    const ref = Preact.createRef();
-    const wrapper = mount(
-      <Lightbox id="lightbox" ref={ref} scrollable>
-        <p>Hello World</p>
-      </Lightbox>
-    );
-
-    ref.current.open();
-    wrapper.update();
-
-    // Render provided children
-    expect(
-      wrapper
-        .find('div[part="lightbox"]')
-        .getDOMNode()
-        .className.includes('contain-scroll')
-    ).to.be.false;
-    expect(
-      wrapper.find('div[part="lightbox"] > div').getDOMNode().style.overflow
-    ).to.equal('scroll');
-
-    // Default SR button is present
-    const buttons = wrapper.find('button');
-    expect(buttons).to.have.lengthOf(1);
-    const closeButton = buttons.first().getDOMNode();
-    expect(closeButton.getAttribute('aria-label')).to.equal('Close the modal');
-    expect(closeButton.textContent).to.equal('');
-  });
-
   it('renders custom close button', () => {
     const ref = Preact.createRef();
     const wrapper = mount(