no need to _applyFocus on new currentOverlay

valdrinkoshi commented 6 years ago

Fixes https://github.com/PolymerElements/paper-toast/issues/97. Here the repro showing the fix - click on the input and notice the focus never leaves it http://output.jsbin.com/tuleyag/2/quiet

We don't need to invoke _applyFocus to the new overlay because restoring of the focus is already handled by 2 other code blocks:

restoreFocusOnClose logic already handles restoring the focus if the user didn't move it to another element (e.g. click on another button)
_onCaptureFocus logic takes care of bringing back the focus to the overlay when needed.

Focus handling tests already cover most of these interactions, and adding a new test for this case would result in testing an implementation detail (e.g. "test that _applyFocus is not called").

notwaldorf commented 6 years ago

with my very dubious understanding of overlays, this looks good. the demo looks sensical. are there any case where you would....lose the focus (since the last thing being called is this._focusNode.blur();)?

valdrinkoshi commented 6 years ago

Yes, that would happen for the last overlay being closed. Blurring the _focusNode would move the focus back to <body>. Another case is when there are 2 or more overlays: the closing overlay would blur its node and that's it; the overlay that is still opened would capture the focus event and handle it (e.g. if it is withBackdrop, it would bring focus back into itself)

PolymerElements / iron-overlay-behavior

no need to _applyFocus on new currentOverlay #274