Closing Popup by pressing Escape doesn't work in all cases

[Schleuse]

Description

The keyboard handler adds the keydown-Listener to the whole document and not to the map-container element.

In order not to interfere with other keyboard-events, the keydown-listener gets registered when the map-container receives focus and gets removed when a blur event on the map-container occurs.

Leaflet/src/map/handler/Map.Keyboard.js

Lines 56 to 59 in 37d2fd1

	this._map.on({
	focus: this._addHooks,
	blur: this._removeHooks
	}, this);

Leaflet/src/map/handler/Map.Keyboard.js

Lines 71 to 74 in 37d2fd1

	this._map.off({
	focus: this._addHooks,
	blur: this._removeHooks
	}, this);

Leaflet/src/map/handler/Map.Keyboard.js

Lines 132 to 138 in 37d2fd1

	_addHooks: function () {
	on(document, 'keydown', this._onKeyDown, this);
	},
	_removeHooks: function () {
	off(document, 'keydown', this._onKeyDown, this);
	},

When the marker gets clicked, it receives focus, therefore the map container looses focus and triggers a blur event. The blur event disables the Keyboard-Handler and the opened popup can't be closed by pressing Esc because the Keyboard-Handler is disabled.

• I've looked at the documentation to make sure the behavior is documented and expected
• I'm sure this is a Leaflet code issue, not an issue with my own code nor with the framework I'm using (Cordova, Ionic, Angular, React…)
• I've searched through the issues to make sure it's not yet reported

Steps to reproduce
Steps to reproduce the behavior:

• Add a marker to a map
• Bind a popup to that marker
• Click the marker to open the popup
• Try to close the opened popup by pressing Esc

Expected behavior
The opened popup should be closed then the Esc-Key is pressed

Current behavior
The popup get's only closed when the layer the popup is bound to is not focusable

Environment

• Leaflet version: 1.4
• Browser (with version): Chrome 73
• OS/Platform (with version): Windows 10

Minimal example reproducing the issue

https://next.plnkr.co/edit/Soh1otCSsfdBBCi0

• this example is as simple as possible
• this example does not rely on any third party code

[Schleuse]

I'm wondering why the keydown listeners are added to the document and not the map-container.
Is it necessary for some legacy browser?

I've made a branch where the KeyBoard-Handler adds the listeners to container and not the whole document. By doing that it is not necessary to add and remove the listeners on focus and blur and it seems somewhat simpler and more robust

[IvanSanchez]

Because keyboard events only fire/bubble on HTMLElements that the browser deems focusable. Let me quote from https://w3c.github.io/uievents/#events-keyboard-event-order , emphasis mine:

The event target of a key event is the currently focused element which is processing the keyboard activity. This is often an HTML input element or a textual element which is editable, but MAY be an element defined by the host language to accept keyboard input for non-text purposes, such as the activation of an accelerator key or trigger of some other behavior. If no suitable element is in focus, the event target will be the HTML body element if available, otherwise the root element.

Because a Leaflet map container is often a non-focusable-by-default <div>, markers can have non-focusable DivIcons and pop-ups have non-focusable containers, and on top of that there's multiple browser implementations, the only reliable way of handling keyboard input is to attach the event handlers to the document.

The current code attaches focus/blur and keyboard event handlers to the map container at ...
https://github.com/Leaflet/Leaflet/blob/master/src/map/Map.js#L1326-L1327
...since #6421, but anyway this is one of the areas where I must ask for testing in several browsers and environments.

[Malvoz]

@Schleuse's #6551 (comment):

When the marker gets clicked, it receives focus, therefore the map container looses focus and triggers a blur event. The blur event disables the Keyboard-Handler and the opened popup can't be closed by pressing Esc because the Keyboard-Handler is disabled.

@IvanSanchez's #6551 (comment):

Because keyboard events only fire/bubble on HTMLElements that the browser deems focusable

Because [...] pop-ups have non-focusable containers

#8115 as proposed requires the popup's container to be programmatically focusable via tabindex="-1", perhaps that'd be sufficient to have the Esc key close popups more reliably? Just an observation on the potential relation.

[joeyskuo]

hello, I've modified the Popup class's Layer to include a keydown eventListener to close the popups. ( PR #8239 )

Edited Plunkrs for demonstration
https://plnkr.co/edit/pkOzSCsI5QvtAcAe
https://plnkr.co/edit/4hXgn7EMinn9Wwn6