Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ally.disengage not piercing shadow roots #107

Closed
ryan-ludwig opened this issue Feb 5, 2016 · 8 comments
Closed

ally.disengage not piercing shadow roots #107

ryan-ludwig opened this issue Feb 5, 2016 · 8 comments

Comments

@ryan-ludwig
Copy link

When using ally.js with web components, ally.disengage does not re-enable elements inside shadow roots.

When using Polymer's "shady DOM" (Polymer 1.0 default), disengage works fine. However, using shadow DOM (which will be the default in the future), elements inside shadow roots remain disabled.

Demo: http://jsfiddle.net/kmdz6Lyq/20/

jordanaustin referenced this issue in VernierST/ally.js Feb 5, 2016
@rodneyrehm rodneyrehm added the bug label Feb 5, 2016
@rodneyrehm
Copy link
Member

When using ally.js with web components, ally.disengage does not re-enable elements inside shadow roots.

Boom. I'm an idiot.

This querySelectorAll and the querySelectorAll in maintain/hidden don't penetrate shadows.

It's tempting to try the quick fix [data-ally-disabled], :disabled to html /deep/ [data-ally-disabled], html /deep/ :disabled as done in selector/focusable. But that would exclude Firefox for now.

I guess I'd prefer maintaining a cache on the service instance, and mutate that upon observed mutation. On that topic, I'm pretty sure the MutationObserver does not see changes within a ShadowRoot. So that's the next issue to solve.

The problems exist since version 1.0.0 in maintain/disabled and maintain/hidden.

@jordanaustin
Copy link

Unfortunately /deep/ and >>> will throw warnings even in Chrome currently as they have already posted an intent to deprecate https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/68qSZM5QMRQ

I agree that probably the best long term solution is to maintain a cache.

@rodneyrehm
Copy link
Member

Unfortunately /deep/ and >>> will throw warnings even in Chrome currently as they have already posted an intent to deprecate

that is only partially true. Yes, currently Chrome logs a warning, but it shouldn't. The deprecation notice concerns the dynamic profile not the static one (which is the one used by querySelector). see

I agree that probably the best long term solution is to maintain a cache.

I guess this would be the perfect time to introduce WeakSet for browsers that already support it and use an array for older browsers.

On that topic, I'm pretty sure the MutationObserver does not see changes within a ShadowRoot.

This means we have to find ShadowHost elements in the context and register individual MutationObservers for each of them. oh the joy.

@jordanaustin
Copy link

The deprecation notice concerns the dynamic profile not the static one (which is the one used by querySelector)

As far as Chrome is concerned I you're right. I guess if I'm reading the spec correctly it looks like this is a go for the static profile. I was a bit concerned by some talk I saw about Safari being completely opposed to /deep/: WICG/webcomponents#78 but maybe that's old news.

@jordanaustin
Copy link

guess this would be the perfect time to introduce WeakSet for browsers that already support it and use an array for older browsers.

Likely this will need to be Set rather than WeakSet since you can't iterate over WeakSet, but either way that sounds good. I'm in the process of testing an implementation of this.

This means we have to find ShadowHost elements in the context and register individual MutationObservers for each of them. oh the joy.

This doesn't sound fun. I did see some talk about adding a property to the MutationObserver config for ShadowRoot, but I think this had mixed reviews for the idea.

@rodneyrehm
Copy link
Member

Likely this will need to be Set rather than WeakSet since you can't iterate over WeakSet …

I did not know that. I was aiming for the weak references, not the Set. :(

@jordanaustin
Copy link

It seems like this can be broken into two bugs:

  1. To actually querying elements within the shadow-root, possibly with /deep/ when re-enabling inert elements
  2. Observe mutations within elements that contain a shadow-root

Thoughts?

@rodneyrehm
Copy link
Member

merged your commits from #107 and resolved the remaining issues. Closing this issue, as #110 tracks the MutationObserver for ShadowDOM issue.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants