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] What are the precedence rules for togglepopup, showpopup, and hidepopup #523

Closed
mfreed7 opened this issue Apr 21, 2022 · 4 comments
Labels
popover The Popover API

Comments

@mfreed7
Copy link
Collaborator

mfreed7 commented Apr 21, 2022

See #508 for context, but the popup now supports three content attributes:

  • togglepopup=popup_id: If popup_id refers to a showing popup, hide it. If popup_id refers to a hidden popup, show it.
  • showpopup=popup_id: If popup_id refers to a hidden popup, show it.
  • hidepopup=popup_id: If popup_id refers to a showing popup, hide it.

The question becomes: what happens when one element has more than one of these attributes set? What if they point to different idrefs?

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Apr 22, 2022

Posted by @jh3y:

  • I like the idea of "toggle", "show", and "hide". For precedence, "show" or "hide" trump "toggle" IMO as they are more specific about what they are doing. If all three exist, it's a toggle. If "show" and "hide" exist, it's a non-issue too? Only issue is with varying IDs 🤔 If a button opens one and closes another, where does a11y come into that? Does it?

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Apr 22, 2022

I'd like to propose this as a behavior:

When an activating element (e.g. <button>) is activated:

  1. Let popup_idref be null, and let popup_action be empty.
  2. If the togglepopup attribute is present on the element:
    1. Let popup_idref be the value of the togglepopup property.
    2. Let popup_action be "toggle".
  3. If popup_action is empty, and the showpopup attribute is present on the element:
    1. Let popup_idref be the value of the showpopup property.
    2. Let popup_action be "show".
  4. If popup_action is empty or "show", and the hidepopup attribute is present on the element:
    1. If popup_idref is null, set it to the value of the hidepopup property.
    2. If the value of the hidepopup property is not equal to popup_idref, continue.
    3. If popup_action is "show", set popup_action to "toggle". Otherwise, set it to "hide".
  5. If popup_action is empty, stop this algorithm.
  6. Let popup_target be the result of running getElementById() on popup_idref.
  7. If popup_target does not exist, or does not have a popup attribute with a valid value, end this algorithm.
  8. If popup_target is in the "showing" state, and popup_action is "toggle" or "hide", then call popup_target.hidePopup() and end this algorithm.
  9. If popup_target is in the "hidden" state, and popup_action is "toggle" or "show", then call popup_target.showPopup() and end this algorithm.

And that should:

  1. Make togglepopup trump the other two.
  2. Make showpopup "win" in terms of idrefs, in case it differs with hidepopup.
  3. If both showpopup and hidepopup are present, they need to point to the same idref, and then you'll get togglepopup behavior.
  4. There's only ever one popup_target.

Thoughts? Bugs in the algorithm?

@mfreed7 mfreed7 added 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 27, 2022
This CL adds 'showpopup' and 'hidepopup' as two more invoking
attributes for the Popup API. Details of the expected behavior
are discussed here:

  openui/open-ui#523 (comment)

Bug: 1307772
Change-Id: Ie6c0d72b36cedd827e0b484b5635fcc6b99fb8f6
@css-meeting-bot
Copy link

The Open UI Community Group just discussed [popup] Add ::backdrop to popup to enable modal popup #519, and agreed to the following:

  • RESOLVED: We accept the precedence rules from https://github.com/openui/open-ui/issues/523#issuecomment-1106686358.
The full IRC log of that discussion <hdv> Topic: [popup] Add ::backdrop to popup to enable modal popup #519
<hdv> github: https://github.com/openui/open-ui/blob/main/meetings/telecon/2022-04-21.md
<hdv> github: https://github.com//issues/523
<jh3y> wrt #519: +1 Good to match expectation from <dialog> usage
<hdv> masonf: the idea of togglepopup is that the idref you put on a button that it toggles that, but it needs a rule for what happens when there are multiple attributes
<hdv> masonf: what we need to resolve on is what happens exactly so that developers know what to expect
<flackr> q+
<JonathanNeal> q?
<hdv> masonf: I proposed some rules on the thread, summary: if you provide togglepopup, it wins, it ignores showpopup and hidepopup. If you use showpopup and hidepopup, with different ids, showpopup wins, if they have the same idref, it behaves like togglepopup
<hdv> flackr:
<hdv> flackr: I agree high level… in the proposed algorithm, if you show ID1 and hide ID2, you get toggle behavior?
<hdv> masonf: should be show behavior?
<hdv> flackr: ok I agree, if the IDs don't match we should not modify the behavior
<JonathanNeal> q?
<JonathanNeal> ack flackr
<hdv> masonf: let's try today to discuss just what we want, then we'll fix the algorithm later
<miriam> +1 I like the intended behavior
<hdv> JonathanNeal: is there agreement on the intended behavior and the idea for this to be resolved ?
<una> SGTM
<JonathanNeal> +1
<Alexander_Futekov> +1
<hdv> flackr: I think the algorithm is correct actually
<flackr> +1
<masonf> Proposed resolution: We accept the precedence rules from https://github.com//issues/523#issuecomment-1106686358.
<JonathanNeal> +1
<masonf> RESOLVED: We accept the precedence rules from https://github.com//issues/523#issuecomment-1106686358.
<hdv> masonf: if you find a bug in the algorithm please tell me

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 29, 2022
This CL adds 'showpopup' and 'hidepopup' as two more invoking
attributes for the Popup API. Details of the expected behavior
are discussed here:

  openui/open-ui#523 (comment)

Bug: 1307772
Change-Id: Ie6c0d72b36cedd827e0b484b5635fcc6b99fb8f6
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 29, 2022
This CL adds 'showpopup' and 'hidepopup' as two more invoking
attributes for the Popup API. Details of the expected behavior
are discussed here:

  openui/open-ui#523 (comment)

Bug: 1307772
Change-Id: Ie6c0d72b36cedd827e0b484b5635fcc6b99fb8f6
@mfreed7
Copy link
Collaborator Author

mfreed7 commented Apr 29, 2022

Based on the resolution, I'm going to close this issue. If you find issues/bugs in the proposed algorithm, please report them as a new issue.

@mfreed7 mfreed7 closed this as completed Apr 29, 2022
@mfreed7 mfreed7 removed the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label Apr 29, 2022
aarongable pushed a commit to chromium/chromium that referenced this issue Apr 29, 2022
This CL adds 'showpopup' and 'hidepopup' as two more invoking
attributes for the Popup API. Details of the expected behavior
are discussed here:

  openui/open-ui#523 (comment)

Bug: 1307772
Change-Id: Ie6c0d72b36cedd827e0b484b5635fcc6b99fb8f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3609067
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#997841}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 29, 2022
This CL adds 'showpopup' and 'hidepopup' as two more invoking
attributes for the Popup API. Details of the expected behavior
are discussed here:

  openui/open-ui#523 (comment)

Bug: 1307772
Change-Id: Ie6c0d72b36cedd827e0b484b5635fcc6b99fb8f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3609067
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#997841}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 29, 2022
This CL adds 'showpopup' and 'hidepopup' as two more invoking
attributes for the Popup API. Details of the expected behavior
are discussed here:

  openui/open-ui#523 (comment)

Bug: 1307772
Change-Id: Ie6c0d72b36cedd827e0b484b5635fcc6b99fb8f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3609067
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#997841}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 16, 2022
…ributes for popup, a=testonly

Automatic update from web-platform-tests
Add showpopup and hidepopup invoking attributes for popup

This CL adds 'showpopup' and 'hidepopup' as two more invoking
attributes for the Popup API. Details of the expected behavior
are discussed here:

  openui/open-ui#523 (comment)

Bug: 1307772
Change-Id: Ie6c0d72b36cedd827e0b484b5635fcc6b99fb8f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3609067
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#997841}

--

wpt-commits: 4c555ebf12a62220184683c976c6ab4f537775a4
wpt-pr: 33807
jamienicol pushed a commit to jamienicol/gecko that referenced this issue May 25, 2022
…ributes for popup, a=testonly

Automatic update from web-platform-tests
Add showpopup and hidepopup invoking attributes for popup

This CL adds 'showpopup' and 'hidepopup' as two more invoking
attributes for the Popup API. Details of the expected behavior
are discussed here:

  openui/open-ui#523 (comment)

Bug: 1307772
Change-Id: Ie6c0d72b36cedd827e0b484b5635fcc6b99fb8f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3609067
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#997841}

--

wpt-commits: 4c555ebf12a62220184683c976c6ab4f537775a4
wpt-pr: 33807
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This CL adds 'showpopup' and 'hidepopup' as two more invoking
attributes for the Popup API. Details of the expected behavior
are discussed here:

  openui/open-ui#523 (comment)

Bug: 1307772
Change-Id: Ie6c0d72b36cedd827e0b484b5635fcc6b99fb8f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3609067
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#997841}
NOKEYCHECK=True
GitOrigin-RevId: 817beb3da8b62eee30ce5fa17273faf80676c66e
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

2 participants