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

[popup] Should showPopup() and hidePopup() throw? #511

Closed
mfreed7 opened this issue Apr 1, 2022 · 2 comments
Closed

[popup] Should showPopup() and hidePopup() throw? #511

mfreed7 opened this issue Apr 1, 2022 · 2 comments
Labels
popover The Popover API

Comments

@mfreed7
Copy link
Collaborator

mfreed7 commented Apr 1, 2022

I think this is mostly an ergonomics question. But what should happen here?

<div>Not a popup</div>
<div popup=popup>Valid popup</div>

<script>
const nonPopup = document.querySelector(':not([popup])');
const popup = document.querySelector('[popup]');
nonPopup.showPopup(); // Exception? Console warning? Nothing?
nonPopup.hidePopup(); // Same questions
popup.hidePopup(); // What about here? The popup isn't showing yet.

Thoughts?

@mfreed7 mfreed7 added the popover The Popover API label Apr 1, 2022
@mfreed7 mfreed7 added the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label Apr 11, 2022
@mfreed7
Copy link
Collaborator Author

mfreed7 commented Apr 14, 2022

So we just had a discussion at OpenUI, and somehow the minutes didn't get posted. Here is a copy/paste from the IRC.

The Open UI Community Group just discussed 511, and agreed to the following:

  • RESOLVED: the 'showPopup' and 'hidePopup' methods should throw in invalid situations.
The full IRC log of that discussion
[11:06]  masonf: #511 we move the popup to an element meaning it can apply to anything. There's 2 JS APIs showPopup and Hidepopup
[11:07]  present+
[11:07]  q+
[11:07] * Zakim sees JonathanNeal on the speaker queue
[11:08]  masonf: If we remove the attribute of a showing popup, it will be hidden
[11:09]  if you did .showModal() on an element that isn't a dialog, you get an "Uncaught TypeError" / .showModal is not a function
[11:09]  emilio: I vote for throwing, specially if they are developer errors
[11:09]  JonathanNeal: Are there prior examples of methods in the DOM which are similar to the popup methods that have made these decisions before
[11:10] == davatron5000 [[email protected]] has joined #openui
[11:10]  present+
[11:11]  I would lean on throwing, too, based on bkardell_’s reference.
[11:11]  ack JonathanNeal
[11:11] * Zakim sees no one on the speaker queue
[11:11]  brian: The only thing I can think of is detached shadows
[11:11]  Proposed resolution: the 'showPopup' and 'hidePopup' methods should throw in invalid situations.
[11:11]  s/detached shadows/attachShadow
[11:11]  Jonathan: Mason will you be seeking a resolution to have this throw?
[11:11]  +1 to throwing errors
[11:11]  +1 to this. I agree with `attachShadow` being a good example to follow.
[11:12]  +1
[11:12]  Jonathan: We have a resolution that showPopup and hidePopup should throw in invalid situations
[11:12]  RESOLVED: the 'showPopup' and 'hidePopup' methods should throw in invalid situations.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Apr 14, 2022

Based on the resolution above, I'm going to close this issue as resolved.

@mfreed7 mfreed7 closed this as completed Apr 14, 2022
@mfreed7 mfreed7 removed the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label Apr 27, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 28, 2022
With this CL, both `showPopup()` and `hidePopup()` will throw exceptions
in invalid situations, including:
 - called on non-popup elements
 - called when the popup is in an invalid state (e.g. showPopup on an
   already-showing popup).

See this issue:
  openui/open-ui#511 (comment)

Bug: 1307772
Change-Id: Ib6b623fdcc0fcf7de68f496163df7b47b1c24732
aarongable pushed a commit to chromium/chromium that referenced this issue Apr 28, 2022
With this CL, both `showPopup()` and `hidePopup()` will throw exceptions
in invalid situations, including:
 - called on non-popup elements
 - called when the popup is in an invalid state (e.g. showPopup on an
   already-showing popup).

See this issue:
  openui/open-ui#511 (comment)

Bug: 1307772
Change-Id: Ib6b623fdcc0fcf7de68f496163df7b47b1c24732
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3610726
Reviewed-by: Joey Arhar <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#997279}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 28, 2022
With this CL, both `showPopup()` and `hidePopup()` will throw exceptions
in invalid situations, including:
 - called on non-popup elements
 - called when the popup is in an invalid state (e.g. showPopup on an
   already-showing popup).

See this issue:
  openui/open-ui#511 (comment)

Bug: 1307772
Change-Id: Ib6b623fdcc0fcf7de68f496163df7b47b1c24732
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3610726
Reviewed-by: Joey Arhar <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#997279}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 28, 2022
With this CL, both `showPopup()` and `hidePopup()` will throw exceptions
in invalid situations, including:
 - called on non-popup elements
 - called when the popup is in an invalid state (e.g. showPopup on an
   already-showing popup).

See this issue:
  openui/open-ui#511 (comment)

Bug: 1307772
Change-Id: Ib6b623fdcc0fcf7de68f496163df7b47b1c24732
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3610726
Reviewed-by: Joey Arhar <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#997279}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 16, 2022
…valid situations, a=testonly

Automatic update from web-platform-tests
Make showPopup and hidePopup throw in invalid situations

With this CL, both `showPopup()` and `hidePopup()` will throw exceptions
in invalid situations, including:
 - called on non-popup elements
 - called when the popup is in an invalid state (e.g. showPopup on an
   already-showing popup).

See this issue:
  openui/open-ui#511 (comment)

Bug: 1307772
Change-Id: Ib6b623fdcc0fcf7de68f496163df7b47b1c24732
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3610726
Reviewed-by: Joey Arhar <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#997279}

--

wpt-commits: 7e280cd5814a822cb05b7116eb2cbaf736af2803
wpt-pr: 33848
jamienicol pushed a commit to jamienicol/gecko that referenced this issue May 25, 2022
…valid situations, a=testonly

Automatic update from web-platform-tests
Make showPopup and hidePopup throw in invalid situations

With this CL, both `showPopup()` and `hidePopup()` will throw exceptions
in invalid situations, including:
 - called on non-popup elements
 - called when the popup is in an invalid state (e.g. showPopup on an
   already-showing popup).

See this issue:
  openui/open-ui#511 (comment)

Bug: 1307772
Change-Id: Ib6b623fdcc0fcf7de68f496163df7b47b1c24732
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3610726
Reviewed-by: Joey Arhar <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#997279}

--

wpt-commits: 7e280cd5814a822cb05b7116eb2cbaf736af2803
wpt-pr: 33848
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
With this CL, both `showPopup()` and `hidePopup()` will throw exceptions
in invalid situations, including:
 - called on non-popup elements
 - called when the popup is in an invalid state (e.g. showPopup on an
   already-showing popup).

See this issue:
  openui/open-ui#511 (comment)

Bug: 1307772
Change-Id: Ib6b623fdcc0fcf7de68f496163df7b47b1c24732
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3610726
Reviewed-by: Joey Arhar <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#997279}
NOKEYCHECK=True
GitOrigin-RevId: dd87eda26f8c0a4acbd5b129e567ce6650a0c9fc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
popover The Popover API
Projects
None yet
Development

No branches or pull requests

1 participant