-
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
The popup attribute should allow the empty string value #533
Comments
Thanks for opening this issue @domenic. After listening to your arguments, I think I agree that it sounds like a good option to make One question: if we go this route, and recalling our resolution to only IDL-reflect valid values, what should this return? myPopup.popup = 'invalid value';
myPopup.popup; // ??? It would seem "bad" to return the empty string, since that would round trip unfortunately: myPopup.popup = 'invalid value';
myPopup.popup = myPopup.popup;
myPopup.popup; // 'popup' (or whatever #491 decides) It seems like |
For reflection, you have two choices in the current spec:
The former seems like it would work well if you added a keyword for the no-popup state (e.g., We could also explore adding new reflection variations, but I'm not sure that'd be necessary. |
I think this makes the most sense to me. There'll need to be an enum value (there already is in Chromium) that means "none", so let's expose it directly. I'm going to add this to the Agenda for next week, since we just resolved to rename the main case to Proposed resolution: For the Any objections? |
There's a third choice
crossOrigin has this setup, and also has the empty string being a state that's different from the missing value default. Limited to only known values is good for feature-testing, so IMO it's a choice between having Returning null for the no-popup state and non-empty strings for the other states allows for truthy/falsy checks in JS, which seems a bit nicer in terms of developer ergonomics. |
Simon, I can't find that in the reflection section for some reason... Is crossOrigin just not well specified? Otherwise, yeah, that does seem a bit nicer. |
Ohh I like the third choice. I do agree returning Proposed resolution: For the |
@domenic I think it falls under
but indeed it seems like there's a spec bug since Searching for " |
I, perhaps incorrectly, assumed there was an implied "and the IDL attribute is limited to only known values" in that paragraph. In non-spec language, here are my assumptions about what we want:
|
Would this look like the right WebIDL for the above table? [Unscopable, CEReactions, Reflect, ReflectOnly=("auto","hint","async"), ReflectEmpty="auto"] attribute DOMString? popup; There's no way to use |
The table looks right to me. As for the WebIDL, The current spec text for reflecting nullable enumerated attributes says to return |
Right, of course. Thanks.
Is that something that seems easy to add to the spec? @domenic thoughts? |
Yeah, it's easy to add, and in fact something we need to do because currently the spec for |
So I tried to work on a spec edit here and it appears crossOrigin is already working OK. Basically it appears nullable DOMString enumerated attributes are always implicitly "limited to known values". Whenever you are in the missing value default state, then you get back So for the table above I think you want the following spec:
|
Great, thanks! |
Per the [1] resolution, `popup=popup` has been renamed to `popup=auto`. Additionally, per the discussion at [2], it is likely that "boolean- like" behavior will also be adopted, such that `<div popup>` means the same thing as `<div popup=auto>`. This CL implements both of these changes. Note that this CL has one case that still needs to be fixed: <div id=foo popup=invalid> <script> foo.popup === null; // false, should be true </script> To fix the above, I need to figure out how to specify the ReflectMissing and ReflectInvalid values so that they mean "null". [1] openui/open-ui#491 (comment) [2] openui/open-ui#533 Bug: 1307772 Change-Id: I6037c5322f7408ebd2c91690f89ecbc513c66bdb
The Open UI Community Group just discussed
The full IRC log of that discussion<gregwhitworth> Topic: [Popup] The popup attribute should allow the empty string value<gregwhitworth> github: https://github.com//issues/533 <gregwhitworth> masonf: last week we resolved to have popup=auto <gregwhitworth> masonf: this was opened to allow a boolean like syntax <gregwhitworth> you could just have popup="" or just popup <una> q+ <gregwhitworth> masonf: that would mean the same thing as popup=auto, there is nice easy way to spec this <gregwhitworth> masonf: in participation of this resolution I have implemented this <gregwhitworth> masonf: curious what people think <gregwhitworth> ack una <JonathanNeal> +1 to `popup`/`popup=""` being the same as `popup="auto"`. <gregwhitworth> una: I really like this <gregwhitworth> una: do we still need popup="auto" <JonathanNeal> q+ <gregwhitworth> una: do we still need it if we agree on this? <JonathanNeal> ack Jonathan, Mason has what I was going to share covered. <gregwhitworth> masonf: there is an IDL reflection, you get a string - it feels better to get auto rather than "" <JonathanNeal> ack JonathanNeal <gregwhitworth> masonf: it only reflects valid values and see if it is supported for feature detection <gregwhitworth> una: I don't mean to open this bag of worms <gregwhitworth> una: we use default in CSSWG <JonathanNeal> In CSS, I think we do also use `auto`, e.g. `margin: auto`. <gregwhitworth> una: we decided to not go with default but there seems to be more precedence for default rather than auto <gregwhitworth> masonf: we did discuss this for CSS which means the "normal thing" <gregwhitworth> gregwhitworth: +1 to having a boolean approach <gregwhitworth> una: +1, I like just popup <masonf> Proposed resolution: For the popup attribute, an empty string (`<div popup="">`) value should map to `popup=auto`. The IDL will be limited to only known values, meaning it will return `null` for both invalid content attribute values and when the `popup` attribute is missing entirely. Do *not* add a specific attribute value meaning "no popup", e.g. `popup=none`. <gregwhitworth> una: we have this with checked and there are multiple values <dandclark> Resolution LGTM <gregwhitworth> masonf: this is an enumerated values/cases, so it's not pure boolean <gregwhitworth> q? <JonathanNeal> +1 to the proposed resolution. +1 to what Una said, too. I heart boolean attributes. <masonf> RESOLVED: For the popup attribute, an empty string (`<div popup="">`) value should map to `popup=auto`. The IDL will be limited to only known values, meaning it will return `null` for both invalid content attribute values and when the `popup` attribute is missing entirely. Do *not* add a specific attribute value meaning "no popup", e.g. `popup=none`. |
Per the [1] resolution, `popup=popup` has been renamed to `popup=auto`. Additionally, per the [2] resolution, "boolean-like" behavior is also supported, such that `<div popup>` means the same thing as `<div popup=auto>`. This CL implements both of these changes. Note that this CL has one case that still needs to be fixed: <div id=foo popup=invalid> <script> foo.popup === null; // false, should be true </script> To fix the above, I need to figure out how to specify the ReflectMissing and ReflectInvalid values so that they mean "null". [1] openui/open-ui#491 (comment) [2] openui/open-ui#533 (comment) Bug: 1307772 Change-Id: I6037c5322f7408ebd2c91690f89ecbc513c66bdb
Per the [1] resolution, `popup=popup` has been renamed to `popup=auto`. Additionally, per the [2] resolution, "boolean-like" behavior is also supported, such that `<div popup>` means the same thing as `<div popup=auto>`. This CL implements both of these changes. Note that this CL has one case that still needs to be fixed: <div id=foo popup=invalid> <script> foo.popup === null; // false, should be true </script> To fix the above, I need to figure out how to specify the ReflectMissing and ReflectInvalid values so that they mean "null". [1] openui/open-ui#491 (comment) [2] openui/open-ui#533 (comment) Bug: 1307772 Change-Id: I6037c5322f7408ebd2c91690f89ecbc513c66bdb
Per the [1] resolution, `popup=popup` has been renamed to `popup=auto`. Additionally, per the [2] resolution, "boolean-like" behavior is also supported, such that `<div popup>` means the same thing as `<div popup=auto>`. This CL implements both of these changes. Note that this CL has one case that still needs to be fixed: <div id=foo popup=invalid> <script> foo.popup === null; // false, should be true </script> To fix the above, I need to figure out how to specify the ReflectMissing and ReflectInvalid values so that they mean "null". [1] openui/open-ui#491 (comment) [2] openui/open-ui#533 (comment) Bug: 1307772 Change-Id: I6037c5322f7408ebd2c91690f89ecbc513c66bdb
Per the [1] resolution, `popup=popup` has been renamed to `popup=auto`. Additionally, per the [2] resolution, "boolean-like" behavior is also supported, such that `<div popup>` means the same thing as `<div popup=auto>`. This CL implements both of these changes. Note that this CL has one case that still needs to be fixed: <div id=foo popup=invalid> <script> foo.popup === null; // false, should be true </script> To fix the above, I need to figure out how to specify the ReflectMissing and ReflectInvalid values so that they mean "null". [1] openui/open-ui#491 (comment) [2] openui/open-ui#533 (comment) Bug: 1307772 Change-Id: I6037c5322f7408ebd2c91690f89ecbc513c66bdb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3668816 Auto-Submit: Mason Freed <masonf@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Reviewed-by: Aaron Leventhal <aleventhal@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/main@{#1009383}
Per the [1] resolution, `popup=popup` has been renamed to `popup=auto`. Additionally, per the [2] resolution, "boolean-like" behavior is also supported, such that `<div popup>` means the same thing as `<div popup=auto>`. This CL implements both of these changes. Note that this CL has one case that still needs to be fixed: <div id=foo popup=invalid> <script> foo.popup === null; // false, should be true </script> To fix the above, I need to figure out how to specify the ReflectMissing and ReflectInvalid values so that they mean "null". [1] openui/open-ui#491 (comment) [2] openui/open-ui#533 (comment) Bug: 1307772 Change-Id: I6037c5322f7408ebd2c91690f89ecbc513c66bdb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3668816 Auto-Submit: Mason Freed <masonf@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Reviewed-by: Aaron Leventhal <aleventhal@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/main@{#1009383}
… just `popup`, a=testonly Automatic update from web-platform-tests Convert `popup=popup` to `popup=auto` or just `popup` Per the [1] resolution, `popup=popup` has been renamed to `popup=auto`. Additionally, per the [2] resolution, "boolean-like" behavior is also supported, such that `<div popup>` means the same thing as `<div popup=auto>`. This CL implements both of these changes. Note that this CL has one case that still needs to be fixed: <div id=foo popup=invalid> <script> foo.popup === null; // false, should be true </script> To fix the above, I need to figure out how to specify the ReflectMissing and ReflectInvalid values so that they mean "null". [1] openui/open-ui#491 (comment) [2] openui/open-ui#533 (comment) Bug: 1307772 Change-Id: I6037c5322f7408ebd2c91690f89ecbc513c66bdb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3668816 Auto-Submit: Mason Freed <masonf@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Reviewed-by: Aaron Leventhal <aleventhal@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/main@{#1009383} -- wpt-commits: 71cf6a42d3335c0c55cfe5b18c3b250c21a0ffbc wpt-pr: 34217
FWIW, I don't have permission to reopen, but it seems a bit more consistent to me to treat invalid popup values (like It seems a bit of a better default, specially if we implement new popup types in the future? |
So the point of the existing behavior is forward compatibility, generally. The thing that worries me about making Can I ask the flip-side question? Why do you want to support |
Just to clarify, your concern is (let's call this "scenario A"):
Is that right? If I got that right, let's consider two other scenarios. Scenario B:
Scenario C:
Scenario B and C seem a bit worse to me that scenario A. In the (unlikely) case of A happening, we have alternatives (easiest one being "choose another string that's not in-use"), while B and C seem a bit less in our control, and worse for users.
Preventing scenario B and C, basically, which I think are worse than scenario A. But it might be the case that there's another scenario that I'm not considering. I think there's value in consistently hiding all elements with How bad B/C are depends a bit on how we expect authors to style popups. If we expect authors to effectively do:
Then the concerns are a bit less, I think (but then again, maybe we should put that in a UA sheet). When discussing #561, it was my assumption that authors would just style the element regularly: #mypopup {
display: flex;
....
} And what would take care of controlling the visibility of the popup would be something along the lines of: [popup]:-internal-popup-hidden { display: none !important } in the user-agent sheet. But I see that's not the case (there's no |
You make some good points. I had been more worried about Scenario A, since it seems more likely to me. I.e. a developer who reads about the I'm ok with making the Invalid Value Default equal to "auto", meaning function supportsPopUpType(type) {
const div = document.createElement('div');
div.popUp = type;
return div.popUp === type.toLowerCase();
} Not as easy as a null check, but not terrible. This would also mean we could go back to a non-nullable DOMString for the Your other point about the missing |
That change makes sense to me. I wonder about the Invalid Value Default choice of manual vs. auto? Manual seems like maybe a better fallback behavior for "we didn't understand what you asked for", but that's just my first impression; I didn't analyze all of scenario A/B/C for each choice. |
Hmm, I tend to agree with this. I originally said we should default to |
Seems like we might already be close to consensus. I'll put this on the agenda for next week. |
+1 to |
The Open UI Community Group just discussed
The full IRC log of that discussion<hdv> topic: [popup] The popup attribute should allow the empty string value #533<hdv> github: https://github.com//issues/533 <hdv> masonf: we talked about this one several times in the past… current behavior is if you assign a value to `popup` attribute that is invalid, then it is invalid and will result in no popup behavior to happen, so it will be a normal element, but won't have popup behavior <hdv> masonf: we have been discussing the standard UA stylesheet for a popup <hdv> masonf: the proposal is: if you use any attribute value that is invalid for popup, you would get the popup=manual behavior <hdv> masonf: this doesn't break feature detection, you could still do that and the issue has code for how to do it <JonathanNeal> q? <hdv> masonf: so I would like to change from not supporting invalid popup values to supporting them as if they were popup=manual <hdv> JonathanNeal: any comments? <masonf> Proposed resolution: Change the Invalid Value Default for `popup` to be `manual`. <dandclark> Seems reasonable to me. <JonathanNeal> I find it reasonable, as well. <hdv> bkardell_: I thought this was an interesting discussion, I wanted to think about it but didn't have time for it this week <hdv> +1 seems reasonable to me <hdv> JonathanNeal: let me phrase it differently, does anyone have objections ? <hdv> bkardell_: only that we spent a long time debating and got to the opposite conclusion, I would like to understand why that was wrong… it's pretty minor, but I would not want us to revert again <hdv> masonf: I think when we discussed it the first time we were kind of learning together how these kinds of attributes work… having looked at it more it looks like it is actually possible to feature detect <hdv> masonf: I can also add Domenic was one of the people pushing for the old resolution and he agrees now with the new one <JonathanNeal> q? <hdv> JonathanNeal: based on that, would you want to go ahead with resolution? <hdv> masonf: maybe we resolve now and we can always revisit later? <flackr> +1 to the proposal <hdv> JonathanNeal: feel free to jump in anyone if you feel against this resolution <masonf> RESOLVED: Change the Invalid Value Default for `popup` to be `manual`. |
+1 to this as well. my only concern was that if |
Right. I like
|
Per the resolution [1], invalid values should now be treated as if they were `popup=manual`. This lines up with the related change [2] to the UA stylesheet for pop-up. I also took the opportunity to expand the "basic" test to include adding the `popup` attribute on all (visible) element types. [1] openui/open-ui#533 (comment) [2] https://chromium-review.googlesource.com/c/chromium/src/+/3840811 Bug: 1307772 Change-Id: I6d5bf956e344f9256ab9cc1bf3d30655c8dee5bf
Per the resolution [1], invalid values should now be treated as if they were `popup=manual`. This lines up with the related change [2] to the UA stylesheet for pop-up. I also took the opportunity to expand the "basic" test to include adding the `popup` attribute on all (visible) element types. [1] openui/open-ui#533 (comment) [2] https://chromium-review.googlesource.com/c/chromium/src/+/3840811 Bug: 1307772 Change-Id: I6d5bf956e344f9256ab9cc1bf3d30655c8dee5bf
Per the resolution [1], invalid values should now be treated as if they were `popup=manual`. This lines up with the related change [2] to the UA stylesheet for pop-up. I also took the opportunity to expand the "basic" test to include adding the `popup` attribute on all (visible) element types. [1] openui/open-ui#533 (comment) [2] https://chromium-review.googlesource.com/c/chromium/src/+/3840811 Bug: 1307772 Change-Id: I6d5bf956e344f9256ab9cc1bf3d30655c8dee5bf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3858445 Commit-Queue: Mason Freed <masonf@chromium.org> Reviewed-by: David Baron <dbaron@chromium.org> Auto-Submit: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1039838}
Per the resolution [1], invalid values should now be treated as if they were `popup=manual`. This lines up with the related change [2] to the UA stylesheet for pop-up. I also took the opportunity to expand the "basic" test to include adding the `popup` attribute on all (visible) element types. [1] openui/open-ui#533 (comment) [2] https://chromium-review.googlesource.com/c/chromium/src/+/3840811 Bug: 1307772 Change-Id: I6d5bf956e344f9256ab9cc1bf3d30655c8dee5bf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3858445 Commit-Queue: Mason Freed <masonf@chromium.org> Reviewed-by: David Baron <dbaron@chromium.org> Auto-Submit: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1039838}
Per the resolution [1], invalid values should now be treated as if they were `popup=manual`. This lines up with the related change [2] to the UA stylesheet for pop-up. I also took the opportunity to expand the "basic" test to include adding the `popup` attribute on all (visible) element types. [1] openui/open-ui#533 (comment) [2] https://chromium-review.googlesource.com/c/chromium/src/+/3840811 Bug: 1307772 Change-Id: I6d5bf956e344f9256ab9cc1bf3d30655c8dee5bf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3858445 Commit-Queue: Mason Freed <masonf@chromium.org> Reviewed-by: David Baron <dbaron@chromium.org> Auto-Submit: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1039838}
… to "manual", a=testonly Automatic update from web-platform-tests Change the `popup` invalid value default to "manual" Per the resolution [1], invalid values should now be treated as if they were `popup=manual`. This lines up with the related change [2] to the UA stylesheet for pop-up. I also took the opportunity to expand the "basic" test to include adding the `popup` attribute on all (visible) element types. [1] openui/open-ui#533 (comment) [2] https://chromium-review.googlesource.com/c/chromium/src/+/3840811 Bug: 1307772 Change-Id: I6d5bf956e344f9256ab9cc1bf3d30655c8dee5bf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3858445 Commit-Queue: Mason Freed <masonf@chromium.org> Reviewed-by: David Baron <dbaron@chromium.org> Auto-Submit: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1039838} -- wpt-commits: 00fb4540b2e367a635280ebb4267256e69d536af wpt-pr: 35629
Per the [1] resolution, `popup=popup` has been renamed to `popup=auto`. Additionally, per the [2] resolution, "boolean-like" behavior is also supported, such that `<div popup>` means the same thing as `<div popup=auto>`. This CL implements both of these changes. Note that this CL has one case that still needs to be fixed: <div id=foo popup=invalid> <script> foo.popup === null; // false, should be true </script> To fix the above, I need to figure out how to specify the ReflectMissing and ReflectInvalid values so that they mean "null". [1] openui/open-ui#491 (comment) [2] openui/open-ui#533 (comment) Bug: 1307772 Change-Id: I6037c5322f7408ebd2c91690f89ecbc513c66bdb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3668816 Auto-Submit: Mason Freed <masonf@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Reviewed-by: Aaron Leventhal <aleventhal@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/main@{#1009383} NOKEYCHECK=True GitOrigin-RevId: 4010500cd24efb96413178987cfb35a0c2419192
Per the resolution [1], invalid values should now be treated as if they were `popup=manual`. This lines up with the related change [2] to the UA stylesheet for pop-up. I also took the opportunity to expand the "basic" test to include adding the `popup` attribute on all (visible) element types. [1] openui/open-ui#533 (comment) [2] https://chromium-review.googlesource.com/c/chromium/src/+/3840811 Bug: 1307772 Change-Id: I6d5bf956e344f9256ab9cc1bf3d30655c8dee5bf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3858445 Commit-Queue: Mason Freed <masonf@chromium.org> Reviewed-by: David Baron <dbaron@chromium.org> Auto-Submit: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1039838} NOKEYCHECK=True GitOrigin-RevId: 323e813eb8163ac256bbc9a36e097bf55f805f6c
I had an offline discussion with @mfreed7 regarding invalid, empty, and missing values which I wanted to relay here. Summary: I think
popup
(or, equivalently,popup=""
) should be allowed and should be equivalent topopup=popup
. This does not cause forward-compat issues and makes the API more pleasant.As background, remember that in HTML,
attribute
is the same asattribute=""
. There's nothing special about an attribute with no equal sign; it's just a shorthand for setting the attribute's value to the empty string. In particular there's no special booleanness vs. stringness for attributes; attribute values are strings, if the attribute is present at all. (Some parts of the spec will interpret certain attributes as booleans, by saying any string value, including the empty string, is treated as "true", and a missing attribute is treated as "false".)Also as background, the
popup
attribute is slated to be an enumerated attribute. These have states and keywords. Keywords are the developer-facing API, and are the strings that appear inside the attribute value. States are the underlying model.It looks like you have the following states: no-popup, popup, hint, and async. (Some or all of these might be renamed per #491 and #532, but I'll use these names for now.)
Then the question is how to map attribute presence/values to those states. It seems like there's broad agreement on the following:
hint
will map to the hint stateasync
will map to the async statefoo
) will map to the no-popup state, for forward compatibility. (In spec terms, this is the invalid value default.)The question is what values should map to the popup state. I think the empty string should map to that state. That gives a nice DX, where the default behavior is expressed with just
<div popup>
. It has many precedents in other enumerated attributes:crossorigin
,translate
,hidden
,contenteditable
, andspellcheck
are all cases where the empty string maps to some sort of "default useful state", and that default useful state is different than the missing value default.Separately, you probably want an explicit non-empty string keyword to also map to the popup state. Let's call it
popup
, although per #491 both the state and keyword will probably be renamed.There is another option, which is to just have the empty string be the only keyword mapping to the popup state. In that case the bikeshedding in #491 becomes much less urgent because the state name is a spec-internal concept and not part of the web developer-exposed interface. But all the example attributes I listed above have a non-empty string keyword as well as the empty string, and it's probably good to stay consistent.
The text was updated successfully, but these errors were encountered: