Improve scroll locking #1449

jimbo · 2019-07-18T15:29:37Z

Description

When the app is masked, such as when a drawer or other modal is open, we intend to prevent scrolling at the window level. Currently, we apply styles to the <Main> element to achieve this, but it doesn't work properly in mobile Safari. This PR would shift scroll locking to the html and body elements as necessary for mobile Safari.

This also resolves a number of visual quirks and inconsistencies in the cart/checkout flow.

Lock window scrolling and overscrolling when mask is active
Fit minicart height to viewport in mobile Safari (replace 100vh)
Adjust padding in cart footers (decrease)
Adjust payments form height (decrease)
Adjust braintree form styles (slightly)
Adjust checkout subform header padding (increase)
Adjust padding in cart item editor (increase)
Fit cart item editor to viewport in mobile Safari (fix auto sizing)

Related Issue

Closes #1071.

Verification Steps

Open the mobile simulator in Xcode.
Go to the deployment of this branch and open the cart.
Verify that the window can't be scrolled (or overscrolled) at all.
Verify that the cart is not obscured by Safari's chrome.

vercel · 2019-07-18T15:29:43Z

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://venia-git-jimbo-minicart-height.magento-research1.now.sh

PWAStudioBot · 2019-07-18T15:36:06Z

	Messages
📖	We are currently working on automating the PR metadata checks. Until that time, you may see failures related to labels/description/linked issues/etc even if you have fixed the problem. Failures will persist until the next push (assuming they are fixed).

Generated by 🚫 dangerJS against 605a44c

packages/peregrine/src/hooks/useScrollLock.js

sirugh · 2019-07-18T15:43:08Z

Did you also check the paymentsForm which relies on the cart height but also some calculations based on footers/etc?

sirugh

Not sure how this fixes the cart height issue. Did you forget to add a commit?

sirugh · 2019-07-18T19:12:20Z

Also check this screenshot out. In chrome, I set the height of the paymentsForm body to:

    height: calc(
        100vh - var(--flow-footer-height) -
            var(--minicart-footer-padding-bottom)
    );

In iOS I have to add an additional height calculation for the iOS footer to achieve the same effect, note the - 7rem:

    height: calc(
        100vh - 7rem - var(--flow-footer-height) -
            var(--minicart-footer-padding-bottom)
    );

sirugh · 2019-07-19T15:44:30Z

Going to need to move files to lib.

sirugh · 2019-07-19T18:14:17Z

packages/venia-concept/src/components/Checkout/paymentsForm.css

-            var(--minicart-footer-padding-bottom)
-    );
-    overflow-y: scroll;
+    height: 30rem;


Are you making a conscious decision to not have the billing form span the height of the viewport? If so, I'm fine with this change, but we are definitely changing the way the payment form looks now.

Also, why not just use vh units?

The whole problem here is that mobile Safari doesn't understand that the "viewport" shouldn't include browser chrome. So 100vh works fine in every other browser, but in Safari you end up with buttons sitting underneath the browser UI at the bottom of the screen.

Ok -- can we maybe increase it a tiny bit? As is unchecking the box to add a distinct address doesn't cause the UI to change or move so it's a little bit of a weird UX. Maybe we should add an scroll/focus to the address form when the button is unchecked?

Haha, I was just demoing this to @soumya-ashok and she had the same feedback. I've added an effect to scroll the new fields into view when added.

jimbo · 2019-07-19T19:36:27Z

Did you also check the paymentsForm which relies on the cart height but also some calculations based on footers/etc?

This has been addressed.

Are you making a conscious decision to not have the billing form span the height of the viewport? If so, I'm fine with this change, but we are definitely changing the way the payment form looks now.

Yes, I'm making that conscious decision. None of the other forms do that—you're supposed to be able to see some of the mask and the cart items underneath. The payments form already has to handle overflow (read: scroll) because it can exceed the height of the viewport, so we might as well reduce it to an intermediate size and let it overflow.

sirugh · 2019-07-22T15:01:18Z

packages/venia-concept/src/components/Checkout/paymentsFormItems.js

@@ -79,6 +81,7 @@ const PaymentsFormItems = props => {
                    />
                </Field>
            </div>
+            <span ref={anchorRef} />


I like this. I just wanted to point out that we are expanding this to be 5 lines of form inputs in #1286. I checked and this code will function fine, even if the form is 6 lines long. Beyond that, however, the "Billing Information" banner hides the top of the form. If you could make this resilient to that, such as if the top field/label were scrolled just under the header banner, then that would be best, however I don't think it's worth holding up the PR since, as I said, it'll work as is (3 lines of form inputs) and will work even up to 6 lines.

sirugh

Just one comment -- if you are going to make changes for it, LMK and I will re-review, otherwise 👍

dpatil-magento · 2019-07-23T15:56:38Z

@jimbo @sirugh Please check below observations -

Desktop chrome browser > Home page, search results, category page shifts left on accessing Left/Right drawer.

On Android - When user access shipping form and tap on Name field - Form shifts up and user cant see whats being typed. (Tried on actual device and also on browser stack virtual device. )

sirugh · 2019-07-23T19:03:48Z

@dpatil-magento is the second one a problem on develop?

dpatil-magento · 2019-07-23T19:11:25Z

@sirugh Yes, I do see the second issue on develop too but thought that needs to be fixed as part of this ticket as its related to scrolling/focus ?

supernova-at · 2019-07-24T14:59:29Z

packages/venia-concept/src/components/MiniCart/cartOptions.css

-    bottom: 0;
-    right: 0;
+    grid-template-rows: min-content 1fr;
+    height: calc(100% - var(--minicart-header-height));


I'm so happy to get rid of this ⭐️

jimbo · 2019-07-24T17:31:41Z

@dpatil-magento I've fixed the second issue for now. In general, presentation is not great in the Android scenario because the keyboard reduces the viewport height rather than overlaying it, but the checkout forms have now been adjusted to constrain themselves to the viewport.

dpatil-magento · 2019-07-24T20:36:32Z

Thanks @jimbo , looks good now.
QA Pass.

sirugh

👍

PWAStudioBot added pkg:peregrine pkg:venia-concept labels Jul 18, 2019

sirugh reviewed Jul 18, 2019

View reviewed changes

packages/peregrine/src/hooks/useScrollLock.js Outdated Show resolved Hide resolved

jimbo added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Jul 18, 2019

vercel bot deployed to staging July 18, 2019 17:33 View deployment

sirugh self-assigned this Jul 18, 2019

sirugh requested changes Jul 18, 2019

View reviewed changes

vercel bot deployed to staging July 18, 2019 23:36 View deployment

vercel bot deployed to staging July 19, 2019 15:39 View deployment

jimbo force-pushed the jimbo/minicart-height branch from d1c4352 to b5c4fe1 Compare July 19, 2019 17:15

vercel bot deployed to staging July 19, 2019 17:16 View deployment

vercel bot deployed to staging July 19, 2019 17:41 View deployment

sirugh reviewed Jul 19, 2019

View reviewed changes

vercel bot deployed to staging July 19, 2019 22:10 View deployment

sirugh reviewed Jul 22, 2019

View reviewed changes

vercel bot temporarily deployed to staging July 22, 2019 17:08 Inactive

sirugh previously approved these changes Jul 22, 2019

View reviewed changes

vercel bot deployed to staging July 22, 2019 17:11 View deployment

vercel bot deployed to staging July 22, 2019 19:13 View deployment

sirugh removed their assignment Jul 22, 2019

supernova-at mentioned this pull request Jul 23, 2019

MiniCart Height Fix #1387

Closed

5 tasks

vercel bot deployed to staging July 23, 2019 17:38 View deployment

vercel bot temporarily deployed to staging July 23, 2019 19:30 Inactive

vercel bot deployed to staging July 23, 2019 19:31 View deployment

supernova-at reviewed Jul 24, 2019

View reviewed changes

vercel bot deployed to staging July 24, 2019 17:18 View deployment

jimbo and others added 11 commits July 24, 2019 10:20

Improve scroll locking when masked

abb0a6e

Fix cart css

b1407f7

Fix cart item editor height

4ec3e51

Fix snapshot

9ab2a6f

Fix checkbox styles in Safari

f33df9f

Add min-height for braintree

e9de48a

Scroll address form on checkbox change

59b296c

Remove billing form affix

3b8cbaa

Fix page layout when masked

29d571b

Simplify CSS inheritance in checkout form

00ec93e

Add max-height to checkout forms

71745da

jimbo force-pushed the jimbo/minicart-height branch from a891c3e to 71745da Compare July 24, 2019 17:28

vercel bot deployed to staging July 24, 2019 17:28 View deployment

Merge branch 'develop' into jimbo/minicart-height

605a44c

vercel bot deployed to staging July 24, 2019 19:53 View deployment

sirugh approved these changes Jul 25, 2019

View reviewed changes

dpatil-magento merged commit db219f0 into develop Jul 25, 2019

sirugh deleted the jimbo/minicart-height branch July 25, 2019 16:21

jimbo mentioned this pull request Jul 16, 2020

Scroll lock effect avoid body shift when activated #2543

Merged

This was referenced Jan 18, 2022

[Snyk] Security upgrade node-fetch from 2.3.0 to 2.6.7 sharkySharks/pwa-studio#52

Open

[Snyk] Security upgrade node-fetch from 2.3.0 to 2.6.7 sharkySharks/pwa-studio#53

Open

[Snyk] Security upgrade node-fetch from 2.3.0 to 2.6.7 sharkySharks/pwa-studio#54

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve scroll locking #1449

Improve scroll locking #1449

jimbo commented Jul 18, 2019 •

edited

Loading

vercel bot commented Jul 18, 2019

PWAStudioBot commented Jul 18, 2019 •

edited

Loading

sirugh commented Jul 18, 2019

sirugh left a comment

sirugh commented Jul 18, 2019

sirugh commented Jul 19, 2019

sirugh Jul 19, 2019

sirugh Jul 19, 2019

jimbo Jul 19, 2019

sirugh Jul 19, 2019

jimbo Jul 19, 2019

jimbo commented Jul 19, 2019

sirugh Jul 22, 2019

sirugh left a comment

dpatil-magento commented Jul 23, 2019

sirugh commented Jul 23, 2019

dpatil-magento commented Jul 23, 2019

supernova-at Jul 24, 2019

jimbo commented Jul 24, 2019

dpatil-magento commented Jul 24, 2019

sirugh left a comment

Improve scroll locking #1449

Improve scroll locking #1449

Conversation

jimbo commented Jul 18, 2019 • edited Loading

Description

Related Issue

Verification Steps

vercel bot commented Jul 18, 2019

PWAStudioBot commented Jul 18, 2019 • edited Loading

sirugh commented Jul 18, 2019

sirugh left a comment

Choose a reason for hiding this comment

sirugh commented Jul 18, 2019

sirugh commented Jul 19, 2019

sirugh Jul 19, 2019

Choose a reason for hiding this comment

sirugh Jul 19, 2019

Choose a reason for hiding this comment

jimbo Jul 19, 2019

Choose a reason for hiding this comment

sirugh Jul 19, 2019

Choose a reason for hiding this comment

jimbo Jul 19, 2019

Choose a reason for hiding this comment

jimbo commented Jul 19, 2019

sirugh Jul 22, 2019

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

dpatil-magento commented Jul 23, 2019

sirugh commented Jul 23, 2019

dpatil-magento commented Jul 23, 2019

supernova-at Jul 24, 2019

Choose a reason for hiding this comment

jimbo commented Jul 24, 2019

dpatil-magento commented Jul 24, 2019

sirugh left a comment

Choose a reason for hiding this comment

jimbo commented Jul 18, 2019 •

edited

Loading

PWAStudioBot commented Jul 18, 2019 •

edited

Loading