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] visibility: hidden vs. other choices #492

Closed
mfreed7 opened this issue Mar 16, 2022 · 5 comments
Closed

[popup] visibility: hidden vs. other choices #492

mfreed7 opened this issue Mar 16, 2022 · 5 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.


The explainer says being not-open="" will use visibility: hidden.

I couldn't find any consideration for why this is the best choice. In particular:

  • not-open="" on <dialog> uses display: none.
  • not-open="" on <details> uses display: block; content-visibility: hidden;.

I don't know which is the best choice for popup, but I hope to see a section weighing the various tradeoffs.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 16, 2022

Summary log of the comments from the original issue:

@scottaohara:
the idea to use visibility: hidden came from the last discussion on how a popup would show/hide, and how a default display: none would make it more difficult for authors to animate/transition a popup into view with CSS.

visibility: hidden was suggested since it would allow for authors to animate, while still hiding the contents of the popup from assistive technologies.

Arguably a popup could still be given a display: none by default and authors could change the default styling to achieve the above - i've worked with teams that have done similar for dialog (and custom dialogs). But it's always a "i didn't know that was possible!" moment when they've been informed of this possibility. Anecdotally that seems to be due to a lack of understanding / awareness of visibility.

@domenic:

Arguably a popup could still be given a display: none by default and authors could change the default styling to achieve the above

Yeah, that was specifically the reasoning behind the current UA stylesheet for [hidden].

So this might be a kind of consistency-vs.-goodness tradeoff.

@mfreed7:
I agree that this is an issue. As @scottaohara said, the motivation was primarily animations. By using visibility, you can easily animate the open and close transitions. But I can see the point that this isn't consistent, and (as long as the UA stylesheet doesn't do display:none !important) it can be overridden by the developer with something like:

div[popup]:not([open]) {
  display: block;
  visibility: hidden;
  transition: whatever;
}

So I think I'm fine changing this back to display:none, if the above makes sense.

@domenic:
Any thoughts on display: block; content-visibility: hidden;?

@mfreed7:

Any thoughts on display: block; content-visibility: hidden;?

Hmm, it's a bit odd in this case, because content-visibility:hidden still renders the box for the element, right? I.e. <div popup style="width:100px;height:100px;content-visibility:hidden"> still renders a 100x100 box somewhere, and just doesn't render any children. I like the general idea of using content-visibility as a way to preserve the layout information for a popup that comes and goes, but it'll need a way around this I think.

@domenic:
That's true... I wonder how <details> gets away with that.

@scottaohara
Copy link
Collaborator

FWIW, i do agree that for consistency sake display: none would be fine, and then author guidance could be provided (a note, or in MDN, for example) on using visibility: hidden instead for animation purposes.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 24, 2022

As I'm implementing the prototype, visibility:hidden ends up feeling very odd. The layout space is still maintained for the "hidden" popup, and I'm pretty sure that's almost never desired. Developers will end up adding their own display:none.

I'm inclined to just change the explainer to display:none (not !important). Any complaints?

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 28, 2022

I've put up a PR to make this change, on the optimistic assumption that people agree with me.

@mfreed7 mfreed7 added the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label Mar 30, 2022
@mfreed7
Copy link
Collaborator Author

mfreed7 commented Apr 8, 2022

I'm going to just make this change and close the issue. It seems to work well, at least for now, in the prototype, and I haven't heard any complaints.

@mfreed7 mfreed7 closed this as completed Apr 8, 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
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