PolymerElements / iron-overlay-behavior

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

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

Open galanovnick opened 6 years ago

galanovnick commented 6 years ago

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

valdrinkoshi commented 6 years ago

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

web-padawan commented 6 years ago

@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)

yosilevy commented 5 years ago

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

yosilevy commented 5 years ago

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

KeithHenry commented 3 years ago

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:

/** 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; 
}