PolymerElements / iron-overlay-behavior

Makes an element an overlay with an optional backdrop
41 stars 71 forks source link

iron-overlay-behavior does not autofocus inputs in nested custom elements #260

Open aarongable opened 6 years ago

aarongable commented 6 years ago

Description

The iron-overlay-element by default sets focus to any child of itself which has the "autofocus" attribute. However, it does not do so for descendants which are within another custom element.

Steps to reproduce

  1. Have a custom element which contains an autofocus input (such as https://chromium.googlesource.com/infra/gerrit-plugins/buildbucket/+/8b6bbfc8104e5015735e36c577b072bdfbb7326d/src/main/resources/static/cr-tryjob-picker.html#65)
  2. Make that element the child of an element which has the IronOverlayBehavior (such as https://chromium.googlesource.com/infra/gerrit-plugins/buildbucket/+/8b6bbfc8104e5015735e36c577b072bdfbb7326d/src/main/resources/static/cr-buildbucket-view.html#84)
  3. Open, close, and then re-open the overlay

Expected outcome

Every time the overlay opens, the autofocus input element gets focus.

Actual outcome

The autofocus input element only gets focus the first time the overlay opens (due to native browser support for autofocus, not due to actions taken by iron-overlay-behavior). The element does not get focus subsequent times.

Live demo

https://crrev.com/c/868827; sign in and then click "Choose Tryjobs"

Suggested action

The following patch is a working fix:

$ diff iron-overlay-behavior.html iron-overlay-behavior.html.patched 
143c143
<       return this._focusedChild || Polymer.dom(this).querySelector('[autofocus]') || this;
---
>       return this._focusedChild || this.querySelector('[autofocus]') || this;

This causes the querySelector to crawl all descendents of the element, even those within the shadow dom of a child element, rather than only considering the custom elements themselves. However, I suspect that Polymer.dom(this) is used for a reason and this may not be the desired approach, so I haven't directly submitted a pull request.

valdrinkoshi commented 6 years ago

Hi @aarongable, the suggested change will not work with native ShadowDOM, as this.querySelector will have visibility only on the light dom of the overlay (in your case, <gr-overlay> will see only <cr-tryjob-picker>).

You'll need to set autofocus on <cr-tryjob-picker>, make it focusable but not tabbable (set tabindex=-1), and handle focus redirection to the <input> in the shadowRoot.

aarongable commented 6 years ago

Thanks, I was able to follow your advice and get it working with this change: https://chromium-review.googlesource.com/c/infra/gerrit-plugins/buildbucket/+/895845

I still think it would be useful for iron-overlay-behavior to be able to autofocus an element that is in the shadow dom of one of its descendants, especially for cases where the devloper is composing a lot of pre-made elements and wants to focus an input contained in (e.g.) a pre-made mailing address form element.

valdrinkoshi commented 6 years ago

I believe iron-overlay-behavior should not pierce into children's shadowRoots, but just handle the light dom. A shadowRoot might be closed, hence iron-overlay-behavior wouldn't even be able to pierce those shadowRoots.

Declaring a shadowRoot with delegatesFocus: true addresses this issue; a custom element that delegates focus forwards the focus to its focusable content by default, and if no content is present, it keeps the focus on itself - see this explainer for more info. This behavior is not polyfilled tho, and I believe it's currently implemented only on Chrome...

In Polymer, you can pass configuration options to attachShadow by overriding _attachDom method, see https://github.com/Polymer/polymer/issues/3988#issuecomment-249319608