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] Display Ordering: will this cause open="" attributes to get removed during HTML parsing? #494

Closed
mfreed7 opened this issue Mar 16, 2022 · 7 comments
Labels
popover The Popover API

Comments

@mfreed7
Copy link
Collaborator

mfreed7 commented Mar 16, 2022

This issue was reported by @domenic here, and is being moved to the OpenUI repo.


https://github.com/mfreed7/popup#display-ordering

It is possible for an HTML document to contain multiple elements with popup=popup. In this case, only the last such element on the page will remain open when the page is fully loaded. This is because each successive popup element will force the previous one closed via the “one at a time” behavior.

The implication of this, combined with https://github.com/mfreed7/popup#dismiss-behavior, is that at the end of parsing only a single open="" attribute will be present, and others will be removed.

If this is the case, it'd be good to call it out explicitly. Also, there is some worry about modifications to the DOM during parsing, and the associated events; see whatwg/html#4500 /cc @annevk @emilio. But probably it's no worse than custom elements, so I think that's not a real issue, just something we need to step lightly around...

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 16, 2022

@annevk said:

Note that custom elements don't end up executing script in all documents they end up in, e.g., with XMLHttpRequest or DOMParser. The problem with <details open> is that it does, iirc.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 16, 2022

Note that the updated explainer removes the open attribute completely. So I think the original issue might be moot. It adds an initiallyopen attribute, but I think we can specify that to just open the last such element on the page, rather than actually opening and then force-closing each subsequent popup. There might still be event-related issues such as whatwg/html#4500. For these reasons, I'm leaving this issue open for comments.

@annevk
Copy link

annevk commented Mar 17, 2022

(FWIW, I think defaultopen would be clearer and easier to spell.)

@domenic
Copy link
Contributor

domenic commented Mar 17, 2022

I think we can specify that to just open the last such element on the page, rather than actually opening and then force-closing each subsequent popup.

How does this work in the case of slow-loading pages? E.g.

<popup initiallyopen>...</popup>

<!-- network stalls for 2 seconds -->

<popup initiallyopen>...</popup>

<!-- network stalls for 2 seconds -->

<popup initiallyopen>...</popup>

<!-- network stalls for 10 seconds -->

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 24, 2022

(FWIW, I think defaultopen would be clearer and easier to spell.)

The initiallyopen content attribute was proposed here, there were some good bikeshed-type comments made here, and resolved to be called initiallyopen here. But I'm not against reopening this can of worms, if you like. If so, would you mind opening a new issue?

How does this work in the case of slow-loading pages? E.g. {network stalls between initiallyopen elements}

Yep, this is an excellent point. I guess that puts us back to actually opening and closing each initiallyopen popup in sequence. Likely that's not too big of a deal, since I think the multiple initiallyopen elements case should be fairly corner case, right? That's also how the existing <popup> element prototype works.

Based on the above comments, I'm inclined to close this issue. Feel free to re-open if you disagree.

@mfreed7 mfreed7 closed this as completed Mar 24, 2022
@annevk
Copy link

annevk commented Mar 24, 2022

Filed #500.

@mfreed7 mfreed7 added the popover The Popover API label Mar 28, 2022
@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 28, 2022

Coming back to this closed issue: I forgot one key detail in the above discussion. For <popup>, we actually specified that the first popup to contain initiallyopen "wins". That both a) makes a little more sense for a developer, since first-in-tree seems better than last-in-tree, and b) makes the implementation issues such as network loading delays mostly go away. That behavior is tested in this WPT. I plan to just keep that behavior for the popup attribute.

mfreed7 pushed a commit to mfreed7/open-ui that referenced this issue Mar 30, 2022
This PR fixes the text around the ordering of `initiallyopen`,
stating that only the **first** popup will be shown, rather than
the last. See [this comment](openui#494 (comment)).
andrico1234 pushed a commit that referenced this issue Mar 31, 2022
This PR fixes the text around the ordering of `initiallyopen`,
stating that only the **first** popup will be shown, rather than
the last. See [this comment](#494 (comment)).

Co-authored-by: Mason Freed <[email protected]>
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

3 participants