[July 2024/11.1] Remove tua-body-scroll-lock in favor of a CSS :has()

The CSS I shared earlier was just a preliminary test. It requires further refinement and testing because it currently has a fixed height of 90vh, which isn't suitable for small confirmation modals. Additionally, as larowlan pointed out, it needs a Firefox-Polyfill.

I think we can use the points where we call lock and unlock in dialog.js to add and remove a class from the body and then use that

I agree, toggling a class on the body element would make it much easier to achieve that.

The library was added as an internal dependency so that we could drop it in a later minor once we have sufficient browser support. 👍 It looks like we would have sufficient browser support for the new class on body element approach. If we end up implementing that, let's make sure that we test it extensively with iOS and Android because they were much trickier to get right in comparison to desktop browsers.

Pushed some POC code, unfortunately now I can't get it to work in FF - seems it handles overflow: clip differently

:has is now supported in all browsers with Firefox 120 out in December 2023

Pavlos.Dontas → changed the visibility of the branch 3388408 to hidden.

Pavlos.Dontas → changed the visibility of the branch 3388408 to active.

Hello all!

I removed this obviously unnecessary dependency.

Now

1. :has in 4 last major FF versions: https://caniuse.com/css-has
2.`ui-vidget-overlay` only exists when dialog modal in jQuery UI
https://github.com/search?q=repo%3Ajquery%2Fjquery-ui%20ui-widget-overla...

Please review!

All javascript libraries which helps to scroll-lock across devices - it's not about `overflow` CSS property only.
`overflow: hidden` is still not working on iOS devices (probably even desktop Safari).

Moveover it will disable `position: sticky` for the child elements somewhere in tree.

So this task & MR requires deep tests

finnsky → changed the visibility of the branch 11.x to hidden.

finnsky → changed the visibility of the branch 3388408-remove-tua-body-scroll-lock-in to hidden.

finnsky → changed the visibility of the branch 3388408 to hidden.

I agree that tests should be runned in any case.
But for `basic current target` of this library in core i think overflow: hidden/clip is enough.

From Slack note (thanks to @sokru) we need to wait until 9th of July. ( https://whattrainisitnow.com/calendar/ )
Until this it may work with:
https://www.npmjs.com/package/css-has-pseudo#browser
as external deps.

Something wrong with
Drupal\KernelTests\Core\Theme\Stable9LibraryOverrideTest
Seems library should be there somehow.
Please review in terms of approach anyway.

Fully agree with #13.

Apart from that, as shown already here in the comments: https://caniuse.com/css-has ... But I obviously read these stats the other way.

Since when do we accept to drop browser support for Drupal on a quick 2-4 years back basis? Obviously too many contributors do live in a comfort zone. No offense ;-) Just too much coffee. Polyfills only work on moderm powerfull enough devices and aren't the solution (golden hammer) for each modern implementation. Besides its own issues, not ot mention.

Based on https://whattrainisitnow.com/calendar/ the Firefox ESR still lacks the :has() feature until 9th of July.

I interpret accessibility differently, it seems. But I fully understand the wish to move forward. Drupal 11 will be out this year. I strongly recommend to at least delay this issue at least to something next year. 11.3 or something.

RE #20

Obviously too many contributors do live in a comfort zone

You are right! Here i see that this is common (Or even regular) practice:
https://www.npmjs.com/package/css-has-pseudo

Weekly Downloads
6,034,801

PostCSS Has Pseudo works in all major browsers, including Internet Explorer 11. With a Mutation Observer polyfill, the script will work down to Internet Explorer 9.

Interesting. Maybe we should integrate this.

Seems to have a kernel failure.

Proposed solution mentions that needed to get done before 10.2 but that didn't happen. Should a different approach be taken now?

@dqd https://www.drupal.org/docs/system-requirements/browser-requirements → and the answer to your specific question is since 2019 after many, many years of discussion in #2390621: [policy, no patch] Update Drupal's browser support policy → . It's an off-topic discussion for this issue - as long as the change meets the browser support policy, it's fine, but that does include Firefox ESR.

A polyfill is possible here, but adding 10.4/July 2024 to the title since I think we could also just wait until 10.4 is open for development and do it then, which would mean we actually release the change in December 2024, 10 months from now, which gives lots of people time to update Firefox ESR from when it comes out. We should only add the polyfill if we really want this to land in 10.3/11.0

@catch: Thanks. Good to know. Would have helped to realize that this here builds on top of this discussion and that the respective decisions have been made already some time ago. Didn't had this on my radar, otherwise I wouldn't have rasied these concerns here. Thanks for pointing me to this. Sorry for the noise then.

finnsky → changed the visibility of the branch 3388408-has to hidden.

We are ready to go here.

Seems test failure related to some ckeditor hotfix
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/ckedi...

So keeping it in needs review

Changing the title to 11.1, thanks for remembering to pick this one back up!,

Firefox declare full support of :has in version 121 (Released 2023-12-19) but current ESR is 128 released on July 9 so I think polyfill no longer needed

@andypost yes current mr without polifill

Just a rebase, there were conflicts in the following,

CONFLICT (content): Merge conflict in core/.eslintrc.json
CONFLICT (content): Merge conflict in core/misc/dialog/dialog.js
CONFLICT (content): Merge conflict in core/package.json
CONFLICT (content): Merge conflict in core/yarn.lock

Proposed solution mentions getting into 10.2, obviously missed that boat. Is this something that could land in 11.1 or 11.2?

What's a good way to test?

You can test it opening modal dialogs. And check if rest of page under opened modal dialogs cannot be scrolled

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

rebased

Would this be a good one to try and get into 11.1 alpha?

I gave this my best shot but will admit a little above my head.

Looking at the MR I see it's relatively up to date (less 90 commits back)
Tests are passing
No open threads.

Applying locally and thought views would be a good test with all the dialogs there. And didn't notice any issues.

With 11.1 approaching think it would be neat to get this one in. So will go out on a limb

Started a change record and release notes. Feel free to improve them :]

nod_ → credited rkoller → .

So thanks to testing by rkoller, scrolling is still an issue on iOS with this solution, unless we have a solution that works there too I don't think we should commit this

https://drupal.slack.com/archives/C1BMUQ9U6/p1730889311131359

So ios 18 fixed the issue, and we're still supporting ios 17 per our supported browser policy, so not quite in the clear

sorry wrong status change

Good to get this tested. So did we mis-identify the caniuse data for safari/ios 17 and this feature? Or does it nominally support the feature but due to a bug, actually not support it after all?

Assuming this never gets fixed in ios17, that leaves two options:

1. If we can find a CSS only workaround for ios17, we could add that, an open an issue to remove the workaround but still get rid of the polyfill.

https://stackoverflow.com/questions/78447747/overflow-hidden-in-body-not... suggests that specifying overflow-x and overflow-y might be enough.

2. Otherwise, we'd need to postpone this on Drupal core requiring ios18 safari which would mean September 2025 if my googling is correct.

Didn't mean to change the status.

It doesn't sound like this make sense to find an alternative solution. Let's just wait.