- Issue created by @larowlan
- 🇦🇹Austria roromedia Linz
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.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
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
- 🇦🇹Austria roromedia Linz
I agree, toggling a class on the body element would make it much easier to achieve that.
- 🇫🇮Finland lauriii Finland
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.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Pushed some POC code, unfortunately now I can't get it to work in FF - seems it handles overflow: clip differently
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
:has is now supported in all browsers with Firefox 120 out in December 2023
- 🇬🇷Greece Pavlos.Dontas
Pavlos.Dontas → changed the visibility of the branch 3388408 to hidden.
- 🇬🇷Greece Pavlos.Dontas
Pavlos.Dontas → changed the visibility of the branch 3388408 to active.
- Status changed to Needs review
8 months ago 12:18pm 24 January 2024 - 🇷🇸Serbia finnsky
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!
- 🇷🇺Russia kostyashupenko Omsk
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
- 🇷🇸Serbia finnsky
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. - 🇷🇸Serbia finnsky
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. - 🇷🇸Serbia finnsky
Something wrong with
Drupal\KernelTests\Core\Theme\Stable9LibraryOverrideTest
Seems library should be there somehow.
Please review in terms of approach anyway. - 🇩🇪Germany diqidoq Berlin | Hamburg | New York | London | Paris
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.
- 🇷🇸Serbia finnsky
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-pseudoWeekly Downloads
6,034,801 - 🇩🇪Germany diqidoq Berlin | Hamburg | New York | London | Paris
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.
- Status changed to Needs work
7 months ago 9:18pm 28 January 2024 - 🇺🇸United States smustgrave
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?
- 🇬🇧United Kingdom catch
@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
- 🇩🇪Germany diqidoq Berlin | Hamburg | New York | London | Paris
@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.
- Status changed to Needs review
about 2 months ago 7:31pm 20 July 2024 - 🇷🇸Serbia finnsky
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
- 🇬🇧United Kingdom catch
Changing the title to 11.1, thanks for remembering to pick this one back up!,
- 🇫🇷France andypost
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