-
Notifications
You must be signed in to change notification settings - Fork 193
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] Do we need the "open" content attribute? #311
Comments
We heard this feedback from a few different sources:
I think those requests capture the extent of my awareness of use cases here:
Before digging into the various technical complexities Mason listed out + finding a resolution for these, I think it would be good for the group to discuss whether there are other use cases for the |
Thanks for these pointers! A few replies below...
Here, I note that the feedback is really to make
I like this option the best, by far.
Here, I think a) just means what is said below: the ability to open the popup "on page load". If the intention is to allow
Discussed a bit above, and in my OP, but can you help me understand how/why this is easier/better than just
Agreed +100 here. I really don't want to cut off use cases, if there are good ones. I just want to avoid complexity where it isn't needed. |
To add on to the example I presented during the discussion, I think we can avoid a lot of the complexities here by following the pattern used by checked on input elements. In particular,
|
Thanks to everyone for the great discussion on this issue. I want to add some comments about what I heard:
The general tone seemed (to me at least!) to be that it was "ok" not to have a full-featured |
Thanks so much for dropping in a summary of today's discussion, @mfreed7! This tracks with what I heard as well, and a great place for us to pick up on the next telecon to try and reach some resolutions. To the rest of the group: as Mason mentioned, please feel free to chime in with use cases if we missed any or you were not able to make today's call! P.S. Updated the issue title 😉 |
I like the In particular, although the general idea of separating the content attribute and IDL attribute like checkbox does, the specific choices checkbox made are not good and cause a lot of developer confusion. For checkbox, the A clean model would be something like an (As for |
I just wanted to chime in and say that for the styling argument, we could simply expose the state through a pseudo class as is the norm. Using a live attribute seems wrong as state has been communicated through pseudo classes for some time now. It would also allow for consistency with other elements such as detail. Edited to remove triangle brackets. |
With regards to styling - I completely agree with @BrentARitchie by defining a new pseudo class that applies to certain elements (eg: @domenic thanks for linking in the TAG review as that does highlight a headache we should avoid. |
Thanks for all of the comments here. I just wanted to highlight one of them - above. @BrentARitchie and @gregwhitworth can you clarify (e.g. with an example) what could be done with a |
In response to @mfreed7, I created a basic demo for a use case using dialog. It has many of the same concerns we are talking about with the Popup element. The the demo is at https://codepen.io/saveoney/full/bGgvQNZ. Essentially what I am doing is making the Dialog partially visible (bottom left) to denote that something needs attention. When the dialog is clicked the rest of the content is then shown as normal. This is only a single use case. For the popup specifically, I could envision part of it being shown as a hint and then expanded/opened as needed. This technique uses what is already available to cut down on DOM nodes and JavaScript. To note, this demo does not consider accessibility, and is slightly broken in that respect, but fixable with a little more code. I also don't claim that this is an example of great technique, just something conceivable in the real-world. |
This was discussed in today's telecon - here are the resolutions:
and here is the resolution regarding styling:
@melanierichards to open a new issue regarding the pseudo selectors. |
This add an attribute called initiallyopen to the <popup> element. When this attribute is present on page load, the first such element will be shown by default. See [1] for the conversation and Open-UI resolution. This CL also re-points the explainer links for all popup tests to the new Open-UI repo. [1] openui/open-ui#311 (comment) Bug: 1168738 Change-Id: Ib95d12234f359d503dc168e6799dd2a17b0dc18e
This add an attribute called initiallyopen to the <popup> element. When this attribute is present on page load, the first such element will be shown by default. See [1] for the conversation and Open-UI resolution. This CL also re-points the explainer links for all popup tests to the new Open-UI repo. [1] openui/open-ui#311 (comment) Bug: 1168738 Change-Id: Ib95d12234f359d503dc168e6799dd2a17b0dc18e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2832157 Reviewed-by: Ionel Popescu <iopopesc@microsoft.com> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/master@{#874037}
This add an attribute called initiallyopen to the <popup> element. When this attribute is present on page load, the first such element will be shown by default. See [1] for the conversation and Open-UI resolution. This CL also re-points the explainer links for all popup tests to the new Open-UI repo. [1] openui/open-ui#311 (comment) Bug: 1168738 Change-Id: Ib95d12234f359d503dc168e6799dd2a17b0dc18e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2832157 Reviewed-by: Ionel Popescu <iopopesc@microsoft.com> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/master@{#874037}
As the lone dissenter from last week's meeting, I wanted to log an opinion in regards to the styling options question. As an author, I value consistency. In my day-to-day building systems, I encounter a lot one-off inconsistencies in the platform. Especially when crossing between HTML, CSS, and JS. Not to create too big of a strawman, but in order to lazyload an asset, you do the following: So, as an author, the currently existing way of styling off of An This element in particular introduces a handful of 🆕 attributes and pseudo-classes.
I'm not saying each of these low-level APIs don't have value or solve problems, I'm only pointing out we're inventing quite a few new concepts without a broader plan or strategy. After reading a handful of Github issues, specifically whatwg/html#5802, I understand the specific problems with |
Agreed. Given details & summary are widely supported the attribute probably can't be removed and while
Can you scope in on what you mean by "broader plan or strategy" as it pertains to this issue? |
…p> element, a=testonly Automatic update from web-platform-tests Add initiallyopen attribute to the <popup> element This add an attribute called initiallyopen to the <popup> element. When this attribute is present on page load, the first such element will be shown by default. See [1] for the conversation and Open-UI resolution. This CL also re-points the explainer links for all popup tests to the new Open-UI repo. [1] openui/open-ui#311 (comment) Bug: 1168738 Change-Id: Ib95d12234f359d503dc168e6799dd2a17b0dc18e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2832157 Reviewed-by: Ionel Popescu <iopopesc@microsoft.com> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/master@{#874037} -- wpt-commits: 9144cfb5bfbeaa6f919bd02ebe53ccbbd408beb4 wpt-pr: 28566
Hey folks, for the purposes of agenda-building, anything we want to revisit or discuss on this issue at this week's telecon? |
* [Popup spec] Add brief intro with HTML-esque metadata * [Popup] Fix up frontmatter * [Popup] Add spec outline * [Popup] Add initiallyopen attribute (addresses #311) * [Popup] Add show() method * [Popup] Convert a note to a todo * [Popup] Add popup attribute * [Popup] Hide currently-shown popup elements steps * [Popup] Setting focus when the popup element is shown * [Popup] Hiding a popup element * [Popup] Componentize 'showing a popup element steps' * [Popup] Various and sundry edits * [Popup] Address first ~half of first round of feedback * [Popup] 2021.07.01 checkpoint in addressing PR feedback (also addresses #358) * [Popup] 2021.07.02 spec updates * [Popup] First batch of updates to HTML editor feedback * Apply suggestions from HTML editor code review Co-authored-by: Domenic Denicola <d@domenic.me> * 2021.08.24 batch of popup updates for HTML editor feedback Co-authored-by: Domenic Denicola <d@domenic.me>
From #416 (comment) I would prefer shorter and easy-to-spell names for HTML, generally. (c.f. misspellings of
|
The Open UI Community Group just discussed
The full IRC log of that discussion<gregwhitworth> Topic: Do we need the "open" content attribute?<gregwhitworth> github: https://github.com//issues/311 <hdv> masonf: #311 is an old issue but it still applies, I feel, in the same ways <hdv> masonf: the proposal has an open attribute that is present wehn the popup is visible and not present when it is invisible <hdv> s/wehn/when <hdv> masonf: what I'm starting to think… we probably should keep all of the resolutions we made on #311 <hdv> masonf: including we should not have an open attribute that is live, but instead we should have an initiallyOpen attribute, a bit liked checked, it just controls what happens on page load and is not live <flackr> +1 <hdv> masonf: and then we would have something in CSS that is a pseudo state that reflects whether the thing is open or not <hdv> masonf: those were the three resolutions we had for this in #311 <flackr> q+ <scotto> that all makes sense to me <gregwhitworth> ack flackr <hdv> flackr: the one thing I wanted to ask, about the top layer behaviour… is that CSS for something that is on the top layer, or something that is tied into a popup being open <hdv> masonf: whether it is in the top layer is strictly up to the UA <hdv> masonf: developers would be free to do what they want in CSS land but nothing they would do there would affect whether it was in the top layer <davidluhr> q+ <hdv> masonf: whether it should be display none important so that you couldn't override, or maybe without important so you could override it… that's still up for discussion <scotto> q+ <hdv> flackr: I'm leaning towards having something that is overridable <hdv> masonf: me too <gregwhitworth> ack davidluhr <hdv> davidluhr: my understanding is that `open` has some challenges because <dialog> doesn't handle the accessibility well… are we avoiding something that would be more consistent because of problems in an existing element? <hdv> masonf: there is definitely a consistency argument; we have many things to handle openness in the platform, this may be one of them <hdv> masonf: the last time we discussed this was in #311… we probably want to move things like <dialog> to this 'better' way <hdv> masonf: there are architectural things… accessibility is an issue. it is hard to have the UA manage attributes <hdv> masonf: whereas in CSS it would 'just work' <gregwhitworth> ack scotto <hdv> scotto: I wanted to +1 the idea of the simple-ish UA styling <gregwhitworth> q+ <hdv> scotto: Greg, I remember you mentioned before it might be useful for responsive design purposes to allow a popup to display inline, that could arguably be done with display block… I think that is important to support <hdv> scotto: I know I was one of the people mentioning the open attribute last time… after reading up on it this morning I would agree not to use `open` here <hdv> scotto: +1 to not using `open` but using CSS <hdv> gregwhitworth: +1 as well… to your question, davidluhr, Domenic Denicola has a great writeup in a TAG review somewhere about why you would not want content attributes like in details/summary <hdv> gregwhitworth: very worthwile reading <hdv> gregwhitworth: we do have to have that base UA stylesheet solution of enabling browser makers to style to their heart <hdv> gregwhitworth: spicy sections do have some style things, but overall it is still basic, but they handle responsive design and accessibility out of the box <hdv> gregwhitworth: we don't want to get to the point where we, like, ship selectmenu and have it so that people can't change some things, like set something to display grid if they want to do that <BoCupp> re: using CSS instead, was there an answer to the question about why popup:open would be useful over just a plain popup selector given popup elements are only rendered when open <hdv> gregwhitworth: so I think we need that standardised UA stylesheet that is truly just 'base' <BoCupp> q+ <hdv> gregwhitworth: I agree with the direction you're taking on this, masonf <gregwhitworth> ack gregwhitworth <gregwhitworth> ack BoCupp <hdv> BoCupp: I joined a little later so may have missed some of it… I hear about CSS being preferred to the content attribute <hdv> BoCupp: why do we need a pseudo class, when it is only rendered when it is open? <hdv> masonf: I think we will not use display: none!important in the UA stylesheet, therefore we would need a pseudo state <hdv> BoCupp: so it being in the top layer mean that it is managed by the browser, but visibiltiy is not? <hdv> masonf: a pseudoclass for it being in the top layer, and a stylesheet rule that says things that are in the top layer as display: none/visibility: hidden, but without !important <hdv> masonf: and that would also mean you could animate it if you like <hdv> BoCupp: there's a lot to think about <hdv> masonf: so most of that is identical to the discussion we already had in #311, there doesn't seem to be a big difference <hdv> flackr: if you did animate it, you might see a flicker <hdv> flackr: if during the animation it would go out of the top layer <hdv> masonf: part of the proposal is that the UA manages being in the top layer… might be an issue? <hdv> flackr: I don't hav e a good answer <hdv> flackr: we did talk about some reasonable timeout that could help with this <hdv> s/hav e/have <masonf> Proposed resolution: The popup API should adopt generally the same resolutions reached in https://github.com//issues/311, e.g. an `initiallyopen` attribute, no live `open` content attribute, a CSS pseudo class for `:open`, etc. <hdv> gregwhitworth: seems like there is general agreement but sub issues that might need some more discussion <masonf> ...subject to sub-issues being discussed. <gregwhitworth> q? <BoCupp> q? <BoCupp> q+ <masonf> maybe change that to `:toplayer` because I think that's better. Subject to bikeshedding. <gregwhitworth> ack BoCupp <hdv> BoCupp: I'd like a little more to consider that the UA is going to render the… I'm just processing that <hdv> BoCupp: would it be without precedence to say that the presence of that attribute means that the display property could be ignored? <hdv> gregwhitworth: what I'm hearing is… maybe we should split #311 to three different issues, then gain resolutions on those sub issues? <hdv> masonf: 3 parts are: `initiallyOpen`, the `open` content attribute and some CSS for the pseudo states <masonf> Proposed resolution: The popup API should not have a "live" `open` content attribute that reflects open/closed state. Proposed resolution: The popup API should have an `initiallyopen` content attribute that controls initial openness. <hdv> BoCupp: `intiiallyOpen` seems fine to me… might need a little more time for the CSS for pseudo class <masonf> RESOLVED: The popup API should not have a "live" `open` content attribute that reflects open/closed state. <BoCupp> q? <masonf> RESOLVED: The popup API should have an `initiallyopen` content attribute that controls initial openness. |
Given the resolutions above, I'm going to close this issue. I've opened a fresh issue to discuss the third part of this issue, the CSS pseudo class for top layer status. Thanks! |
Well, I don't have issue closing permissions, so I'll just say that I think this issue can be closed, and hope someone does that. 😀 |
This add an attribute called initiallyopen to the <popup> element. When this attribute is present on page load, the first such element will be shown by default. See [1] for the conversation and Open-UI resolution. This CL also re-points the explainer links for all popup tests to the new Open-UI repo. [1] openui/open-ui#311 (comment) Bug: 1168738 Change-Id: Ib95d12234f359d503dc168e6799dd2a17b0dc18e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2832157 Reviewed-by: Ionel Popescu <iopopesc@microsoft.com> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/master@{#874037} GitOrigin-RevId: 9d0421abc7790a8e3447ae6151c87557e9081146
On MSEdgeExplainers #467, @mfreed7 asked:
The text was updated successfully, but these errors were encountered: