PolymerElements / paper-dropdown-menu

A Material Design browser select element
https://www.webcomponents.org/element/PolymerElements/paper-dropdown-menu
61 stars 107 forks source link

paper-dropdown-menu does not close when lose focus #141

Open victorhugom-zz opened 8 years ago

victorhugom-zz commented 8 years ago

On chrome the paper-dropdown-menu element does not close when lose focus. Works on Edge and IE.

<paper-dropdown-menu class="flex" style="margin-right:10px" label="Cidade" always-float-label="[[alwaysFloatLabel]]" on-paper-dropdown-close="_addressChanged">
    <paper-listbox class="dropdown-content" attr-for-selected="item" selected="{{address.city}}">
        <template is="dom-repeat" items="[[cities]]" as="city">
            <paper-item item="[[city]]">{{city}}</paper-item>
        </template>
     </paper-listbox>
</paper-dropdown-menu>

The full element is at: https://github.com/victorhugom/acre-address-form or http://victorhugom.github.io/acre-address-form

Browsers Affected

Edit: The issue occours only on touch enabled devices The original paper-dropdown-menu demo has the same problem

freshp86 commented 8 years ago

/cc @danbeam

I am observing the same behavior with touch devices. As a result of this bug multiple drop-down menus on the same page can be open at the same time, see attachment from the demo page.

multiple_dropdown_menus

danbeam commented 8 years ago

/cc @tjsavage

freshp86 commented 8 years ago

@cdata: Any updates here? This is affecting Chromium's new settings, see https://bugs.chromium.org/p/chromium/issues/detail?id=604685#c3.

wq9 commented 8 years ago

The drop-down menus can be closed by tapping somewhere else with finger, but clicking with the mouse elsewhere leaves the menu open.

mgiuffrida commented 8 years ago

A simpler repro of the original issue with just a keyboard: https://jsfiddle.net/j4rfjasf/

  1. Tap Button 1 to focus
  2. Tab to the paper-dropdown-menu
  3. Press Enter or Down/Up to open the menu
  4. Tab to Button 2

Expected: menu closes. Actual: menu stays open.

Chrome 52.0.2743.32

JanMiksovsky commented 8 years ago

I'm looking at this issue now.

JanMiksovsky commented 8 years ago

I wanted to provide an update on this. I’m nearly ready to submit a fix, but at the last minute am having issues getting tests to pass in Firefox/IE under WCT. The tests pass fine in regular browser instances, but focus problems prevent the automated tests from passing in both Firefox and IE at the same time. In a bit of unfortunate timing, I’m about to leave on an extended break until mid-August. So for now, I wanted to provide a record of progress so far, and explain the problem and solution to both others and my future self.

Like many focus issues, this has proven tricky to get right while minimizing impact on related components.

While the problem shows up in paper-dropdown-menu, I believe iron-dropdown is the correct place to fix this. @cdata and I also discussed fixing this in iron-overlay-behavior, but he would prefer to see this fixed for dropdown-style overlays, or at least at first, in order to avoid causing regressions in dialog-style overlays like paper-dialog. We could move this issue to iron-dropdown, although (as noted below), it also requires changes in iron-overlay-behavior and iron-overlay-manager.

What behavior do we want?

Clearly, the existing behavior is a bug: the user can move focus away from the dropdown while leaving the dropdown menu open. But there are at least two ways we could fix this:

  1. Have a dropdown implicitly close itself when it loses focus, or
  2. Have a dropdown prevent the user from tabbing away when the dropdown is open.

Looking at both OS combo boxes and the HTML select elements, it’s easy to find examples of both behaviors. Windows and the Windows browsers Edge/IE use solution 1, as does Firefox. Mac OS X and the Safari and Chrome browsers use solution 2. (This simple jsBin can be used to check behaviors.)

We could solicit input from more sources, but to me, solution 1 feels better. It follows the Principle of Least Astonishment regarding the behavior of the Tab key: the Tab key should always move to the next focusable element. If an element gets the focus and won’t let go (solution 2), that’s surprising, and hence less desirable. The surprise is particularly problematic in accessibility contexts. I tried navigating the above jsBin in Safari with Apple Navigator, and opened a select element. At that point, the Tab key would not move to the next element — but the screen reader provided no feedback as to what was going on. If a blind user accidentally opened the dropdown, it would be very difficult to understand what was happening on the screen. Without being able to see the screen, they would have to guess they were stuck somewhere, and try pressing the Esc key in order to continue.

Given the above, I recommend we pursue solution 1: a dropdown that loses focus should auto-close itself. If others strongly prefer solution 2, please advise.

A secondary question is what form of close we want here: a regular dropdown close or a cancel. Looking at the OSes and browsers for reference, it appears that losing focus effectively results in a close. So, that’s the model I’ve pursued.

Note that paper-dropdown-menu does not implicitly update the dropdown value to match the currently-selected item when close() is called programmatically. Hence, the expected behavior here will be: if the user opens a paper-dropdown-menu, changes the selection, then tabs away, the selected value will not update. This is counter the example of the standard select element. However, this would be a separate issue with paper-dropdown-menu / paper-menu-button that could be pursued separately. The solution here makes that issue no worse.

The solution is in two parts…

Part 1: change to iron-overlay-behavior

First, a change to iron-overlay-behavior that enables components like iron-dropdown to refine the existing restoreFocus behavior. Interested parties can see this change on this fork of iron-overlay-behavior.

The existing restoreFocus behavior in iron-overlay-manager prevents an iron-dropdown from letting go of the focus. If the user presses Tab, and focus moves outside of the dropdown, the new iron-dropdown PR will auto-close the dropdown. However, in removeOverlay, iron-overlay-manager will try to restore the focus back to the element that had the focus before the dropdown opened. That is, iron-overlay-manager is fighting to prevent the behavior we want in iron-dropdown.

The underlying issue is that iron-overlay-manager is applying a single restore behavior to both dropdown-style and dialog-style overlays. We want to preserve the existing behavior for dialog-style overlays, but give dropdown-style overlays more sophisticated behavior. Specifically: at the point a dropdown closes, if the focus is nowhere inside the dropdown, then iron-overlay-manager should not restore focus.

To discriminate between these cases, this has removeOverlay invoke a new method _restoreFocusToNode. The default iron-overlay-behavior implementation of this method is to always focus the indicated node. This default behavior can then be overridden in dropdown-style overlays components like iron-dropdown. In this case, it allows iron-dropdown to perform an extra test: if the focus is no longer anywhere inside the dropdown, it does not restore focus to the indicated node, and lets the focus leave the dropdown as the user wants.

Part 2: change to iron-dropdown

The second part requires teaching iron-dropdown how to track when it has completely lost focus. The change can be viewed on this fork of iron-dropdown.

This is tricky, for a number of reasons:

A specific case of the last issue comes up in Safari/Firefox, where a click/tap on a trigger element (e.g., button) does not automatically focus the tapped element. (Chrome/Edge/IE do automatically focus a tapped element.) So, in Safari/Firefox, the auto-close feature only works if: the dropdown is invoked from the keyboard, or if the dropdown itself can take focus. When viewing the iron-dropdown demo in Safari/Firefox and using the mouse to click the “Basic” button, pressing Tab will not auto-close the dropdown, because neither the button nor dropdown will have focus. Using the keyboard to trigger that button will mean the focus is on the button, so pressing Tab will auto-close the dropdown as expected. Also, using the mouse to click on “Bottom-left Aligned”, then pressing Tab, will also work as expected because that dropdown can receive the focus.

It’s worth noting that, in the originally-filed bug on paper-dropdown-menu, the auto-close feature should always work as expected in Safari/Firefox, even when using tab/click to open the dropdown, because the paper-input (or plain input) should always be able to take the focus.

Summary

Together, these changes produce the desired behavior. Opening an iron-dropdown or paper-dropdown-menu, then pressing Tab, will implicitly closes the dropdown and let the focus move to the next element.

Note: if you press Shift+Tab to move backward in the tab order, the dropdown will auto-close, but the focus will end up on the button that triggered the dropdown. You’ll need to press Shift+Tab again to actually move to the previous element in the tab order. That’s a little surprising, but probably acceptable, especially in light of the focus games that would be required to remove eliminate that behavior.

Next steps

As noted above, the fixes above are essentially complete. All unit tests pass in all browsers when run directly — the remaining problem at this point is getting unit tests to pass in Firefox or IE under WCT. Browsers appear to handle focus/blur events differently if the window is not active (as often happens when testing multiple browsers under WCT), and both Firefox and IE seem particularly prone to this problem. I’ve found a workaround for Firefox that doesn’t work in IE, and vice versa, and I’m stuck trying to find a single workaround that works for both browsers.

If anyone wants to comment on the UI design questions here, or on the implementation approach, please go ahead. If someone knows the answer to the secret of testing real focus/blur events under WCT, please feel free to take a shot at getting the unit tests to pass. As noted above, I won’t be able to work on this for a while, but will pick this up when I’m back in mid-August.

valdrinkoshi commented 8 years ago

FWIW the native