Prevent scrollbar overflow in footer disclosures

[kmeleta]

Why are these changes introduced?

Fixes #997

What approach did you take?

When the footer locale popups have corner radius, the scroll bar can overflow outside the popup. This is because the disclosure list is using overflow-y: auto. To keep the list scrollable but contain the content within the border radius I've wrapped the list in a new <div class="disclosure__list-wrapper"> element. This requires some minor changes to both the css and the js. disclosure__list-wrapper should now be the element concerned with the global setting styles and the visibility/positioning of the dialog on the page.

New pattern:

<button aria-expanded="false" aria-controls="MyList" aria-describedby="MyLabel">
<div class="disclosure__list-wrapper" hidden>
  <ul id="MyList" role="list"></ul>
</div>

Other considerations

I believe the changes to markup are safe and didn't notice any changes in the way VO announces the modified pattern, but flagging here in case this does affect expectations from an a11y perspective in some way.

Tip: Disabling whitespace during review will improve readability of these diffs.

Demo links

• Editor

Checklist

• Followed theme code principles
• Linted with Theme Check
• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility

[melissaperreault]

Thanks Ken! Made some observation that are not due to your PR I believe. Let me know if you would like me to create a new issue. 🙏

Otherwise, it looks good!
Tested:

• Different settings range,
• Different Color scheme
• Safari, Chrome
• No JS experience

No JS (Issue opened #1300)

• The tertiary button style seems they have lost their styling (no border visible), even if border is set to 1 and 100% opacity.

Mobile (Issue opened #1301)

• Noticed on small screen the space between the select is very narrow. They could wrap earlier. Tested on my Android Samsung A70
  • Seems to happen in the editor with shorter select option.
• Related to the above, the dropdown is off screen

[kmeleta]

Thanks @melissaperreault I agree, definitely things to address. But maybe let's go ahead and make an issue to clean up all that stuff so it doesn't hold up this update.

[ludoboludo]

The same happens to the cart note I believe but since it's going to need a different approach it most likely should be a different issue/PR.
It's also kind of an edge case since it only comes up if you write a lot of info in the cart note: screenshot

[kmeleta]

The same happens to the cart note I believe but since it's going to need a different approach it most likely should be a different issue/PR. It's also kind of an edge case since it only comes up if you write a lot of info in the cart note: screenshot

Hmm yeah I remember that, guess we never logged that in the same issue. But I just did a quick audit and it's also going to be an issue with the textarea on blogs and contact. I was gonna throw that in, but I'm not sure what approach to use for it, given an overflow: hidden would also hide the focus border animation 🤔 Let's plan to handle separately for now. cc @ludoboludo

[ludoboludo]

Looks good to me when testing and voice over was clear when navigating it.

Just the one thing is maybe a bit more spacing to fully display the focus ring.

[sofiamatulis]

Nice 👍 Just noticed a few things when playing with the global settings

[sofiamatulis]

This is also an issue on cart note:

https://screenshot.click/10-33-0n390-e7xaj.mp4

[kmeleta]

I'm considering the text area issue separately because it's more complex.

The same happens to the cart note I believe but since it's going to need a different approach it most likely should be a different issue/PR. It's also kind of an edge case since it only comes up if you write a lot of info in the cart note: screenshot

Hmm yeah I remember that, guess we never logged that in the same issue. But I just did a quick audit and it's also going to be an issue with the textarea on blogs and contact. I was gonna throw that in, but I'm not sure what approach to use for it, given an overflow: hidden would also hide the focus border animation 🤔 Let's plan to handle separately for now. cc ludoboludo

[sofiamatulis]

When we add corner radius, the scrollbar gets a bit cut off: https://screenshot.click/10-34-vbl98-jvwxu.mp4

[kmeleta]

Nothing we can do about that with this approach (the solution specifically aims to mask the scrollbar). The only other possible solution (that I see anyway) to is just give the scrollable list enough margin so it stays away from the corners (which is kinda ugly).

[melissaperreault]

I am aligned that we can live with the tradeoff of cutting it off. Merchant would have other ways to go around this like reducing the border thickness and roundness if they have long list of currency/country. Very edge case!

[melissaperreault]

🎉 🚢

[sofiamatulis]

Nice 🎉 🚢