Replace event.which + remove event.delegateTarget + fix crash

[tkrotoff]

Following #19991 (comment)

3 commits

Replace event.which

KeyboardEvent.which and MouseEvent.which are deprecated/non standard.

It's a relic from the jQuery time.

Replaced with KeyboardEvent.key and MouseEvent.button.

Remove event.delegateTarget inside event-handler.js

event.delegateTarget is only used by tooltip, replacing it with event.target works fine, thus no more fixEvent().

Fix crash in dropdown: "Uncaught TypeError: Cannot read property 'focus' of undefined"

While working on this, I've noticed a crash when opening the dropdown and pressing ArrowUp for the first time. Crash already present here: https://twbs-bootstrap.netlify.com/docs/4.3/components/dropdowns/

bootstrap/js/src/dropdown.js

Line 482 in f0abe26

let index = items.indexOf(event.target) || 0

items.indexOf(event.target) || 0 gives -1 || 0 which returns... -1... See explanations "Why are JavaScript negative numbers not always true or false?"

Testing

All tests passing, manually tested (dropdown, tooltip, carousel, tooltip) under macOS with Chrome 81, Firefox 75 and Safari 13.1.

[XhmikosR]

Can you please split the bugfix and make a new PR with that patch only?

As for this PR, I'm not so sure if we should keep using features that aren't available in most browsers. I mean, sure, we officially don't support some browsers, but with using ES3 features until v4, things worked decently in older browsers.

Just thinking out loud, but I think we can just continue with using more modern features.

• https://caniuse.com/#feat=keyboardevent-key
• https://caniuse.com/#feat=mdn-api_mouseevent_button

[tkrotoff]

features that aren't available in most browsers

• https://caniuse.com/#feat=keyboardevent-key
• https://caniuse.com/#feat=mdn-api_mouseevent_button

This is misleading.

Here a demo: https://codepen.io/tkrotoff/pen/RwWrjer (switch to "debug" or "full" view on mobile).

I've tested all browsers under Android 8.1 on a real smartphone.

As you can see event.key and event.button are always present. However on mobile everything (.key, .keyCode, .charCode, .which, .code) is trash on purpose: https://stackoverflow.com/q/36753548

All alternative browsers (except Firefox (Gecko) and Safari/GNOME Web (WebKit)) are based on Chromium anyway...

The only browser that does not follow the standard is IE (and somewhat old Edge) ("Esc" instead of "Escape", "Up" instead of "ArrowUp"...) (ES5 demo for IE: https://codepen.io/tkrotoff/pen/xxwZYVd)

Related PR: Fyrd/caniuse#5362

Tests

Android Chrome 80

Android Firefox 68

Android Opera 57

Android Opera Mini 47

Android UC Browser 13.1

Android UC Browser 12.0

Android QQ Browser 10.2

(downloaded from https://browser.qq.com/ not available on Google Play Store)

Android Baidu 11.21

Windows 7 IE 10

Windows 10 IE 11

Windows 10 Edge 18

macOS 10.13.6 Safari 13.1

macOS 10.13.6 Firefox 75

Windows 10 UC Browser 7.0

iOS 12.1 Safari 12.0 (Simulator 10.1 with physical keyboard plugged)

[tkrotoff]

Can you please split the bugfix and make a new PR with that patch only?

That would be painful: I would have to rewrite the fix for event.which and then change it back to event.key (and you would need to review it 2 times). This PR is only 3 commits.

[XhmikosR]

Not sure why it's painful since it's just 2 lines to change.

[XhmikosR]

Anyway, LGTM. I'll wait for another approval before merging.

[Johann-S]

Seems a good change to me 👍

[nlemoine]

Salut @Johann-S !

I think you might reconsider the removal of event.delegateTarget.

Providing an event delegation function without providing the delegateTarget seems awkward to me. This means you have to look by yourself for the delegateTarget.

event.delegateTarget is only used by tooltip

Whether BS is using it or not isn't relevant IMHO. BS now provides modules such as SelectorEngine, EventHandler, etc. that could be used without any BS components (modal, carousel, etc.).

Consider the following example using the latest version of BS: https://codepen.io/hellonico/pen/JjYwMPa?editors=1011

The event.target will never be the a tag which is what every developer is probably looking for. You can still use CSS to avoid this by setting pointer-events: none on the svg element but I don't think it's a good solution.

What do you think?

[nlemoine]

Another example demonstrating the need for event.delegateTarget: https://codepen.io/hellonico/pen/PoPXEJz?editors=1111

(hover the icon)

Tooltip won't work if the trigger has nested elements.

[XhmikosR]

BS now provides modules such as SelectorEngine, EventHandler, etc. that could be used without any BS components (modal, carousel, etc.).

You are not supposed to use these, they are for private use only.

As for the rest, please wait for @Johann-S to have a look.

[nlemoine]

Thanks for the prompt reply!

You are not supposed to use these, they are for private use only.

Private or not, these modules are exported. They can be used and actually behave just like any other library (a quite popular event delegation lib for example).

However, even they are not supposed to be used directly, there's still an issue about delegateTarget 😉

[tkrotoff]

Re-introduce delegateEvent here: #30928