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

Update with recent OpenUI discussion results #593

Merged
merged 4 commits into from
Sep 8, 2022

Conversation

mfreed7
Copy link
Collaborator

@mfreed7 mfreed7 commented Aug 23, 2022

The largest change is the result of this discussion about the differences between <dialog> and popup=manual. There's a new section attempting to describe those differences. This PR also includes recent changes to the show event, and an update on the interaction between top layer element types.

This PR also removes an obsolete section of the <selectmenu> page that talks about replaceable shadow DOM.

Closes #581

Copy link
Collaborator

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one comment. thanks!

research/src/pages/popup/popup.research.explainer.mdx Outdated Show resolved Hide resolved
@@ -357,7 +381,7 @@ The intention of the above set of behaviors is to return focus to the previously
A new attribute, `anchor`, can be used on a pop-up element to refer to the pop-up's "anchor". The value of the attribute is a string idref corresponding to the `id` of another element (the "anchor element"). This anchoring relationship is used for two things:

1. Establish the provided anchor element as an [“ancestor”](#nearest-open-ancestral-pop-up) of this pop-up, for light-dismiss behaviors. In other words, the `anchor` attribute can be used to form nested pop-ups.
2. The referenced anchor element could be used by the [Anchor Positioning feature](https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/CSSAnchoredPositioning/explainer.md).
2. The referenced anchor element could be used by the **Anchor Positioning feature**. A (dated) explainer can be found [here](https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/CSSAnchoredPositioning/explainer.md), or a more up-to-date draft spec [here](https://tabatkins.github.io/specs/css-anchor-position/).


## Backdrop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only other thing i was thinking about that should probably be called out is in relation to the backdrop.

I do think there will be use cases where popups can present with a backdrop that obscures the background, and with light dismiss behaviors, this will help ensure that if someone wants to get away from the popup and engage with the content beneath, they'll be able to do so with relative ease.

However, it seems like this could be problematic with a popup=manual where there is no light dismiss behavior. someone could keyboard escape and then be prevented from seeing where keyboard focus has moved to on the page.

Outside of author guidance to say "dont' use backdrop on popup=manual or "if you need to fully obscure the content beneath a popup, maybe consider whether that popup should be a modal dialog or not?"

Alternatively, outside of barring popup=manual from having ::backdrop - would it be possible to ensure that if focus were to have left a popup in this state, that the backdrop would be reset to transparent? So it would only obscure the background so long as focus was within the popup?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting point, and I've added a few more sentences to guide away from this behavior for popup=manual.

The only thing I'd add is that not only will keyboard navigation be broken in this case, but so will mouse (and all other) navigation. If the page is obscured, it won't be visible, but pointer-events:none !important will still ensure the ::backdrop can't trap clicks either. Perhaps that makes this less of a risky thing, since the site will be broken for all users, right?

I'm afraid of trying to bar popup=manual from having ::backdrop, since it'll be the only top layer element not to have a ::backdrop, and perhaps there are valid use cases?

Copy link
Collaborator

@scottaohara scottaohara Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thumbs up to the added guidance, thank you.

i also think there could be a use case for a backdrop with a popup=manual, so yeh i don't think it should be barred. But here's an example of what I was thinking would be a potentially safer default - where if focus is within or hovered over the popup, the backdrop gets the styling. But if focus is outside of the popup - and importantly - if focus is outside of the popup BUT mouse hover is still over the popup (as someone with low/no mobility may not be able to easily move their mouse and it unfortunately would just be atop a popup) the backdrop goes away since focus gets detected outside the popup, meaning it needs to be visible.

https://codepen.io/scottohara/pen/xxWoxGL

edit: had a bug in the CSS. I think I got it to work the way I intended now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool demo! Helps me see your point, for sure.

This is an interesting suggestion. I'm not sure there's precedent for anything like this, where UA styling (and lack of developer stylability) of an element depends on where the mouse or focus is. Do you know of anything we could refer to like that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, i'm going to have to think on that if anything like this has been done at the platform level. This is more like something i have had to inform developers on how to do, because of custom components they've built, or customizations they've made to the platform features. I'll keep thinking though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks. Let's keep discussing, and I'm happy to incorporate anything we come up with. But ok to land this PR in the meantime?

Copy link
Contributor

@hidde hidde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a few wording comments

- A `<div popup=manual>` is **in the top layer**, so it draws on top of other content. The same is not true for a non-modal `<dialog>`. This is likely the most impactful difference, as it tends to be difficult to ensure that a non-modal `<dialog>` is painted on top of other page content.
- A `<dialog>` element always has **`role=dialog`**, while the `popup` attribute can be applied to the **most-semantically-relevant HTML element**, including the `<dialog>` element itself: `<dialog popup>`.
- The pop-up API comes with some **"nice to have's"**:
- pop-ups are easy to animate both the show and hide transitions. JS is required to animate `dialog.close()`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I understand correctly, they are easy to animate with CSS, maybe we could explicitly mention that to contrast with dialog and the JS requirement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I addressed your comment - LMK if not.

research/src/pages/popup/popup.research.explainer.mdx Outdated Show resolved Hide resolved
research/src/pages/popup/popup.research.explainer.mdx Outdated Show resolved Hide resolved
The largest change is the result of [this discussion](openui#581) about the differences between dialog and popup=manual. There's a new section attempting to describe those differences. This PR also includes recent changes to the `show` event, and an update on the interaction between top layer element types.

This PR also removes an obsolete section of the `<selectmenu>`
page that talks about replaceable shadow DOM.
@mfreed7
Copy link
Collaborator Author

mfreed7 commented Aug 30, 2022

Any other comments here?

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Sep 6, 2022

Are we ok to land this PR? @andrico1234

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Sep 8, 2022

Are we ok to land this PR? @andrico1234

@gregwhitworth can you please help get this pushed? I keep having to refer to this PR rather than just directly to the explainer. I believe all of the comments have been addressed.

@gregwhitworth gregwhitworth merged commit c61b228 into openui:main Sep 8, 2022
@gregwhitworth
Copy link
Member

@mfreed7 done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[popup] Add further clarity that popup is not (presently?) for modal dialogs
4 participants