Replace dropdown backdrop hack with cleaner JS-only hack #22426
Conversation
js/src/dropdown.js
Outdated
| // if this is a touch-enabled device we remove the extra | ||
| // empty mouseover listeners we added for iOS support | ||
| if ('ontouchstart' in document.documentElement) { | ||
| $('body').children().off('mouseover', $.noop) |
There was a problem hiding this comment.
Here $.noop will continue to be called because the second parameter of off() is a selector see https://api.jquery.com/off/#off-events-selector
To check what I said you can test that :
var lala = function () { console.log('lala') }
var toto = function () { console.log('toto') }
$('body').children().on('mouseover', lala)
$('body').children().on('mouseover', toto)And after try to do this : $('body').children().off('mouseover', lala)
You'll see what I explained before.
To do what you tried you should use this : https://api.jquery.com/off/#off-events-selector-handler
And your code will became
$('body').children().on('mouseover', '*', $.noop)
$('body').children().off('mouseover', '*', $.noop)There was a problem hiding this comment.
crikey, i feel like a doofus. thanks for catching that. made the change and force-pushed the update. one of these days i may even go and learn some jquery...
There was a problem hiding this comment.
You're welcome, happy to help you 😄
There was a problem hiding this comment.
although this is now merged and closed ... i actually got a follow-up here: it seems that even without the selector, it works as expected. when i ran your test in chrome just now, once i did the $('body').children().off('mouseover', lala) it actually removed the lala function correctly (so only toto was triggered). isn't the selector optional?
further, am i right in thinking that $('body').children() and $('body').children().find("*") (which would be what is selected using your proposed change) are not the same thing, as i'm going one level deeper?
(asking because i'm about to add this exact same tweak to tooltip.js, and it made me look deeper into jquery, which is not usually my thing)
There was a problem hiding this comment.
IF we want to keep the explicit selector, and target the children (rather than going one level down), would it actually be better to do something like $('body').children().on('mouseover', null, $.noop) / $('body').children().off('mouseover', null, $.noop) ?
There was a problem hiding this comment.
Yes you're right I checked now and it works but when I checked $('body').children().off('mouseover', lala) didn't worked...
I made a mistake sorry about that :/
There was a problem hiding this comment.
UNACCEPTABLE. no, seriously, no worries. i'll do a quick patch and still use the second parameter as selector, but as null (to make absolutely clear i'm wanting to bind directly to the child elements, and not do event delegation or some such)
As discussed in #22422 the current approach of injecting a backdrop (to work around iOS' broken event delegation for the `click` event) has annoying consequences on touch-enabled laptop/desktop devices. Instead of a backdrop `<div>`, here we simply add extra empty/noop mouse listeners to the immediate children of `<body>` (and remove them when the dropdown is closed) in order to force iOS to properly bubble a `click` resulting from a tap (essentially, method 2 from https://www.quirksmode.org/blog/archives/2014/02/mouse_event_bub.html) This is sufficient (except in rare cases where the user does manage to tap on the body itself, rather than any child elements of body - which is not very likely in an iOS phone/tablet scenario for most layouts) to get iOS to get a grip and do the correct event bubbling/delegation, meaning the regular "click" event will bubble back to the `<body>` when tapping outside of the dropdown, and the dropdown will close properly (just like it already does, even without this fix, in non-iOS touchscreen devices/browsers, like Chrome/Android and Windows on a touch laptop). This approach, though a bit hacky, has no impact on the DOM structure, and has no unforeseen side effects on touch-enabled laptops/desktops. And crucially, it works just fine in iOS.
Same hack as in #22426 (modulo the selector, which is wrong in that PR and will be updated in a separate PR) to get tooltips to work correctly on iOS. Dynamically adds/removes empty (`noop`) `touchstart` event handlers to all children of `<body>` in order to coax iOS into proper event delegation/bubbling
Tweak to #22426, where the wrong selector slipped through the net (selecting all of `<body>`s grand-children rather than children)
As discussed in #22422 the current approach of injecting a backdrop (to work around iOS' broken event
delegation for the
clickevent) has annoying consequences on touch-enabled laptop/desktop devices.Instead of a backdrop
<div>, here we simply add extra empty/noop mouse listeners to the immediate children of<body>(and remove them when the dropdown is closed) in order to force iOS to properly bubble aclickresulting from a tap (essentially, method 2 from https://www.quirksmode.org/blog/archives/2014/02/mouse_event_bub.html)This is sufficient (except in rare cases where the user does manage to tap on the body itself, rather than any child elements of body - which is not very likely in an iOS phone/tablet scenario for most layouts) to get iOS to get a grip and do the correct event bubbling/delegation, meaning the regular
clickevent will bubble back to the<body>when tapping outside of the dropdown, and the dropdown will close properly (just like it already does, even without this fix, in non-iOS touchscreen devices/browsers, like Chrome/Android and Windows on a touch laptop).This approach, though a bit hacky, has no impact on the DOM structure, and has no unforeseen side effects on touch-enabled laptops/desktops (nor non-iOS mobile/tablet devices). And crucially, it works just fine in iOS (tested on iPhone and iPad)
Additionally, this PR removes the styles associated with the now unnecessary dropdown backdrop, and updates the documentation (possibly in far too much "under the hood" detail?)