showModal throws exception when dialog is inside a custom-element

[akc42]

I have a large web-components (using lit-element) application. It will be used in a very closed environment, so I am able to control the browser support. So far its been used exclusively with chrome.

I do use the <dialog> element inside a few components, and built in the use of the polyfill from the beginning. These elements exclusively use showModal() to open dialogs when needed.

I have been asked to check for IOS support on ipads - but with them it has been impossible to debug why things didn't work, so I just starting using Safari on the mac.

However using the debugger on my mac with Safari I have discovered that showModal() tries to check whether the dialog exists by doing a document.body.contains(this.dialog_) call. Node.contains() doesn't include elements inside the shadowRoot of a contained element, so the call throws an exception.

[samthor]

Thanks for the report. The polyfill should work inside a shadow root (even if automatically injecting the CSS is "up in the air"). I'll look at this soon.

[paulerickson]

Removing those checks, it seems to work for me (CDN dist here), except on form submission the page reloads, but maybe that's unrelated.

[nalply]

An alternative to remove the check is using something like this at:

https://github.com/GoogleChrome/dialog-polyfill/blob/master/index.js#L287

var root = this.dialog_.getRootNode ? this.dialog_.getRootNode() : document.body;
if (!root.contains(this.dialog_)) {
  throw new Error('Failed to execute \'showModal\' on dialog: The element is not in a Document nor a shadow root.');
}

Edit changed the code, found a simpler way, see commit below, it works for my project.

[hadriann]

@samthor @nalply can the nalply/dialog-polyfill fork be merged to make the dialog usable inside shadow DOM?

[samthor]

I'll look at this soon.

My concern is that when you're in a shadow root, it's very likely that something is causing layout, so the position: fixed code won't work. But we detect that anyway and complain if you're doing something wrong :)

On Thu, 27 Feb 2020 at 18:30, Adrian Nita notifications@github.com wrote:

@samthor https://github.com/samthor @nalply https://github.com/nalply can the nalply/dialog-polyfill fork be merged to make the dialog usable inside shadow DOM?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GoogleChrome/dialog-polyfill/issues/186?email_source=notifications&email_token=AAA5DEFNBW3PKL2LWLZY7MLRE5T3BA5CNFSM4IWWUSZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENDIO2Q#issuecomment-591824746, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA5DED5QHDHSPHBWIULNIDRE5T3BANCNFSM4IWWUSZA .

[hadriann]

@samthor @nalply I think that any layout issue inside a shadow root is acceptable compared to not working at all in that context. Would be nice and helpful to see that fix merged. Thanks :)

[kwaclaw]

Whats the best way forward? Looks like this is not on anyone's list to fix. Should we use the nalply/dialog-polyfill fork? Or is another polyfill out there?

[samthor]

If there's a PR that fixes this I can take a look very easily.

On Tue, 7 Apr 2020, 00:57 Karl Waclawek, notifications@github.com wrote:

Whats the best way forward? Looks like this is not on anyone's list to fix. Should we use the nalply/dialog-polyfill fork? Or is another polyfill out there?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GoogleChrome/dialog-polyfill/issues/186#issuecomment-609846606, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA5DEEUZURMOGJRLIXHN23RLHUXHANCNFSM4IWWUSZA .

[hadriann]

I just forked and will try to do the minimal changes for this issue to be fixed. @samthor do you know why the dist files in the repo seem not to be up-to-date with index.js?

[samthor]

I'm not sure, but I can build it with Closure if needed. IIRC the non-dist version should be fine for testing.

On Wed, 8 Apr 2020 at 22:43, Adrian Nita notifications@github.com wrote:

I just forked and will try to do the minimal changes for this issue to be fixed. @samthor https://github.com/samthor do you know why the dist files in the repo seem not to be up-to-date with index.js?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GoogleChrome/dialog-polyfill/issues/186#issuecomment-610936635, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA5DEAYEU42WQQ7BWOUMA3RLRWP3ANCNFSM4IWWUSZA .

[hadriann]

@samthor please check pull request https://github.com/GoogleChrome/dialog-polyfill/pull/194 that fixes this issue. Thanks!

[samthor]

I've published 0.5.1 which includes this.

On Fri, 10 Apr 2020 at 20:48, Adrian Nita notifications@github.com wrote:

@samthor https://github.com/samthor please check pull request #194 https://github.com/GoogleChrome/dialog-polyfill/pull/194 that fixes this issue. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GoogleChrome/dialog-polyfill/issues/186#issuecomment-611981317, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA5DEDIO6TTTYZXNIT7DJLRL32RVANCNFSM4IWWUSZA .

[hadriann]

@samthor I believe this issue can be closed as per 0.5.1 ? And I think the version should be bumped as well to 0.5.1 in package.json and package-lock.json.

[samthor]

I bumped up package.json to 0.5.1.