iron-overlay-behavior icon indicating copy to clipboard operation
iron-overlay-behavior copied to clipboard

Focus trapping logic doesn't work with non-Polymer elements

Open galanovnick opened this issue 6 years ago • 5 comments

Description

Focus traping logic is not compatible with elements that are not extending PolymerElement. Moreover, it won't work properly even if at least one of parent shadow roots is not a Polymer element.

As far as I can tell the main reason for that is because of the way deepActiveElement getter of the iron-overlay-manager is implemented. It expects that the shadow root is accessible via root property. LitElement, for example, contains the shadow root in the _root property, so the deepActiveElement won't get a proper active element for it.

Suggestion: why not use shadowRoot to access the shadow root instead?

Live demo

https://glitch.com/edit/#!/focus-trap-issue

Browsers Affected

  • [x] Chrome
  • [ ] Firefox
  • [ ] Safari 9
  • [ ] Safari 8
  • [ ] Safari 7
  • [ ] Edge
  • [ ] IE 11
  • [ ] IE 10

galanovnick avatar Jun 07 '18 11:06 galanovnick

Yeah, using active.root || active.shadowRoot in deepActiveElement should be fine.

In any case, beware that focus trapping doesn't work well when there are nodes with tabindex > 0 - see more details in #241 and in this comment in the code
https://github.com/PolymerElements/iron-overlay-behavior/blob/77a75f0b0c129bca356a4ef9dcdb97f7f3c69951/iron-focusables-helper.html#L116-L127

valdrinkoshi avatar Jun 08 '18 00:06 valdrinkoshi

@valdrinkoshi there is one more place where using .root strikes for vanilla Web Components: https://github.com/PolymerElements/iron-overlay-behavior/blob/89dfc6a36a9db84b8950ae686f2086eba46e5b23/iron-focusables-helper.js#L132

I'm up to submit a PR but we would need this fix for both Polymer 2 and 3 (so there will be 2 PRs)

web-padawan avatar Sep 24 '18 08:09 web-padawan

Is this fixed/coming? I think that line should read: children = dom(element.root || element.shadowRoot || element).children;

yosilevy avatar Mar 20 '19 06:03 yosilevy

Submitted PR https://github.com/PolymerElements/iron-overlay-behavior/pull/292

yosilevy avatar Mar 20 '19 06:03 yosilevy

I have a workaround for this... it is horrible, but I doubt they're ever going to fix this given the age of this issue.

Basically:

  • Add a getSpoofRootCollection that checks .children and .shadowRoot.
  • Wrap everything it returns in a Proxy that replaces the root getter.
  • Use Object.defineProperty to add a root property to the content added to the <paper-dialog> (or whatever).
/** Looking for focus elements we want any shadow DOM children and any light DOM childen
 *  Wrap all of them in the proxy so Polymer's recursion reaches nested web components */
function getSpoofRootCollection(
    parent: Element,
    proxyHandler: ProxyHandler<Element>) {
    const children = parent.shadowRoot ?
        [...parent.shadowRoot.children, ...parent.children] :
        [...parent.children];

    return children.map(c => new Proxy<Element>(c, proxyHandler));
}

/** Proxy that echoes everything except .root or .children, which it gets from the shadow root and wraps in the same proxy */
class SpoofPolymerRoot
    implements ProxyHandler<Element> {

    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    get?<K extends keyof Element>(target: Element, p: K): any {
        if (typeof p === 'symbol')
            return target[p];

        if (p === 'children' ||
            p as any === 'root')
            return getSpoofRootCollection(target, this);

        return target[p];
    }
}

/** Hack fix workaround https://github.com/PolymerElements/iron-overlay-behavior/issues/282
 *  Use a proxy to dummy the root property, which Polymer's deepActiveElement uses to find shadow focus inputs */
function polymerRoot(content: HTMLElement) {
    // Ensure that we only get into this proxy if the recursion down the tree starts with .root
    Object.defineProperty(content, 'root', { get: () => getSpoofRootCollection(content, new SpoofPolymerRoot()) });
    return content; 
}

KeithHenry avatar Mar 15 '21 20:03 KeithHenry