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

Consider throwing for showModal() and showPopover() in non-fully-active documents #10659

Closed
domenic opened this issue Sep 30, 2024 · 12 comments · Fixed by #10705
Closed

Consider throwing for showModal() and showPopover() in non-fully-active documents #10659

domenic opened this issue Sep 30, 2024 · 12 comments · Fixed by #10705
Labels
normative change topic: dialog The <dialog> element topic: popover The popover attribute and friends

Comments

@domenic
Copy link
Member

domenic commented Sep 30, 2024

What is the issue with the HTML Standard?

As a followup to #10634, it doesn't really make sense that you can call showModal() or showPopover() on elements which are inside of synthetic documents, of the type created by document.implementation.createHTMLDocument(). Example: https://jsbin.com/cubafoputo/1/edit?html,js,output

I think we should have these throw exceptions, similar to the exceptions we already throw for disconnected elements.

I suspect this change is web-compatible, especially for popover but even for dialog. These synthetic documents are super rare.

@smaug---- @keithamus @josepharhar @nt1m do you agree? Are you willing to take the potential compat risk or the trouble of adding use counters?

@domenic domenic added normative change topic: dialog The <dialog> element topic: popover The popover attribute and friends labels Sep 30, 2024
domenic added a commit that referenced this issue Sep 30, 2024
@keithamus
Copy link
Contributor

Sounds reasonable to me (non-implementer, userland consumer). They already throw for a handful of exceptional cases (calling showModal() twice for example) which are much more likely to occur.

@lukewarlow
Copy link
Member

Given how new popover is, how most uses of popover are probably based on user input, and how niche this type of document is, my instinct is also that this would be web compatible. Likewise for dialogs, they're relatively new too.

Should this apply to dialog.show() too?

@domenic
Copy link
Member Author

domenic commented Oct 1, 2024

I'm unsure about show(). If we were to ever fix #5802 and make show() roughly equivalent to adding open="", then it would not make much sense to throw, I think.

@domenic domenic added the agenda+ To be discussed at a triage meeting label Oct 1, 2024
@smaug----
Copy link

Throwing sounds good to me.

@annevk
Copy link
Member

annevk commented Oct 1, 2024

Is the idea here that nothing should be in the top layer when a document is no longer fully active? Will we also empty it when a document becomes inactive?

@domenic
Copy link
Member Author

domenic commented Oct 2, 2024

Is the idea here that nothing should be in the top layer when a document is no longer fully active? Will we also empty it when a document becomes inactive?

No, I think the idea is that nothing should be in the top layer for synthetic documents. "Normal" documents that get bfcached can have a top layer without an issue, IMO.

@annevk
Copy link
Member

annevk commented Oct 2, 2024

Wouldn't a browsing context check be more appropriate then? I guess either way could work, but the former is a more exact match of what you're after.

@domenic
Copy link
Member Author

domenic commented Oct 2, 2024

Indeed, they're equivalent in this case. Fully active is just more conventional.

@annevk
Copy link
Member

annevk commented Oct 2, 2024

Well, doesn't a bfcache'd document still hold onto its browsing context?

@domenic
Copy link
Member Author

domenic commented Oct 2, 2024

Yes, but you cannot call showModal() on a <dialog> in a bfcached document, because you cannot have a scripting relationship to such documents.

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 9, 2024

This sounds reasonable, and likely to be web compatible, at least to me. It seems very corner-case to create a document, put a popover in it, and call showPopover() expecting things to happen. There's always the chance that there's a use case, but I can't think of one. I suppose it's possible that people do that and then move those elements to a rendered document and expect things to be "still open", but that's not what happens today, other than an open non-modal dialog because of the open attribute.

I'd be relatively happy to take the risk of compat shipping this change, if there's consensus.

@domenic
Copy link
Member Author

domenic commented Oct 10, 2024

Thanks all. I think that's explicit support from 2/3 implementations and some level of implicit agreement from @annevk, so I'll take this off of agenda+ and proceed with a spec PR asynchronously.

@domenic domenic removed the agenda+ To be discussed at a triage meeting label Oct 10, 2024
domenic added a commit that referenced this issue Oct 10, 2024
domenic added a commit that referenced this issue Oct 16, 2024
Closes #10659. This effectively only impacts dialogs and popovers "synthetic" documents, such as those created via document.implementation.createHTMLDocument().
dizhang168 pushed a commit to dizhang168/html that referenced this issue Oct 28, 2024
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 29, 2024
See discussion at:
  whatwg/html#10659

and spec PR at:
  whatwg/html#10705

Fixed: 373684393
Change-Id: I50e400ee526775f915f006865301fff2f04016b4
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 30, 2024
See discussion at:
  whatwg/html#10659

and spec PR at:
  whatwg/html#10705

Web-facing change PSA:
https://groups.google.com/a/chromium.org/g/blink-dev/c/jRFiIIkXv_k/m/jnPTfg8WBgAJ

Fixed: 373684393
Change-Id: I50e400ee526775f915f006865301fff2f04016b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5943740
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1375681}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 30, 2024
See discussion at:
  whatwg/html#10659

and spec PR at:
  whatwg/html#10705

Web-facing change PSA:
https://groups.google.com/a/chromium.org/g/blink-dev/c/jRFiIIkXv_k/m/jnPTfg8WBgAJ

Fixed: 373684393
Change-Id: I50e400ee526775f915f006865301fff2f04016b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5943740
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1375681}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 31, 2024
…non-active documents, a=testonly

Automatic update from web-platform-tests
Throw exception for popovers/dialogs in non-active documents

See discussion at:
  whatwg/html#10659

and spec PR at:
  whatwg/html#10705

Web-facing change PSA:
https://groups.google.com/a/chromium.org/g/blink-dev/c/jRFiIIkXv_k/m/jnPTfg8WBgAJ

Fixed: 373684393
Change-Id: I50e400ee526775f915f006865301fff2f04016b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5943740
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1375681}

--

wpt-commits: 0b99ee5e8c799f9fbf7d1550ef72fec8bdb45760
wpt-pr: 48854
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Nov 1, 2024
…non-active documents, a=testonly

Automatic update from web-platform-tests
Throw exception for popovers/dialogs in non-active documents

See discussion at:
  whatwg/html#10659

and spec PR at:
  whatwg/html#10705

Web-facing change PSA:
https://groups.google.com/a/chromium.org/g/blink-dev/c/jRFiIIkXv_k/m/jnPTfg8WBgAJ

Fixed: 373684393
Change-Id: I50e400ee526775f915f006865301fff2f04016b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5943740
Reviewed-by: Domenic Denicola <domenicchromium.org>
Auto-Submit: Mason Freed <masonfchromium.org>
Commit-Queue: Domenic Denicola <domenicchromium.org>
Cr-Commit-Position: refs/heads/main{#1375681}

--

wpt-commits: 0b99ee5e8c799f9fbf7d1550ef72fec8bdb45760
wpt-pr: 48854

UltraBlame original commit: ac6cc55b745eb0d675d453266bde353b7d335402
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Nov 1, 2024
…non-active documents, a=testonly

Automatic update from web-platform-tests
Throw exception for popovers/dialogs in non-active documents

See discussion at:
  whatwg/html#10659

and spec PR at:
  whatwg/html#10705

Web-facing change PSA:
https://groups.google.com/a/chromium.org/g/blink-dev/c/jRFiIIkXv_k/m/jnPTfg8WBgAJ

Fixed: 373684393
Change-Id: I50e400ee526775f915f006865301fff2f04016b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5943740
Reviewed-by: Domenic Denicola <domenicchromium.org>
Auto-Submit: Mason Freed <masonfchromium.org>
Commit-Queue: Domenic Denicola <domenicchromium.org>
Cr-Commit-Position: refs/heads/main{#1375681}

--

wpt-commits: 0b99ee5e8c799f9fbf7d1550ef72fec8bdb45760
wpt-pr: 48854

UltraBlame original commit: ac6cc55b745eb0d675d453266bde353b7d335402
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Nov 1, 2024
…non-active documents, a=testonly

Automatic update from web-platform-tests
Throw exception for popovers/dialogs in non-active documents

See discussion at:
  whatwg/html#10659

and spec PR at:
  whatwg/html#10705

Web-facing change PSA:
https://groups.google.com/a/chromium.org/g/blink-dev/c/jRFiIIkXv_k/m/jnPTfg8WBgAJ

Fixed: 373684393
Change-Id: I50e400ee526775f915f006865301fff2f04016b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5943740
Reviewed-by: Domenic Denicola <domenicchromium.org>
Auto-Submit: Mason Freed <masonfchromium.org>
Commit-Queue: Domenic Denicola <domenicchromium.org>
Cr-Commit-Position: refs/heads/main{#1375681}

--

wpt-commits: 0b99ee5e8c799f9fbf7d1550ef72fec8bdb45760
wpt-pr: 48854

UltraBlame original commit: ac6cc55b745eb0d675d453266bde353b7d335402
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Nov 1, 2024
…non-active documents, a=testonly

Automatic update from web-platform-tests
Throw exception for popovers/dialogs in non-active documents

See discussion at:
  whatwg/html#10659

and spec PR at:
  whatwg/html#10705

Web-facing change PSA:
https://groups.google.com/a/chromium.org/g/blink-dev/c/jRFiIIkXv_k/m/jnPTfg8WBgAJ

Fixed: 373684393
Change-Id: I50e400ee526775f915f006865301fff2f04016b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5943740
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1375681}

--

wpt-commits: 0b99ee5e8c799f9fbf7d1550ef72fec8bdb45760
wpt-pr: 48854
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Nov 5, 2024
…non-active documents, a=testonly

Automatic update from web-platform-tests
Throw exception for popovers/dialogs in non-active documents

See discussion at:
  whatwg/html#10659

and spec PR at:
  whatwg/html#10705

Web-facing change PSA:
https://groups.google.com/a/chromium.org/g/blink-dev/c/jRFiIIkXv_k/m/jnPTfg8WBgAJ

Fixed: 373684393
Change-Id: I50e400ee526775f915f006865301fff2f04016b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5943740
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1375681}

--

wpt-commits: 0b99ee5e8c799f9fbf7d1550ef72fec8bdb45760
wpt-pr: 48854
mfreed7 pushed a commit to mfreed7/html that referenced this issue Nov 11, 2024
Closes whatwg#10659. This effectively only impacts dialogs and popovers "synthetic" documents, such as those created via document.implementation.createHTMLDocument().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative change topic: dialog The <dialog> element topic: popover The popover attribute and friends
Development

Successfully merging a pull request may close this issue.

6 participants