Add popover attribute #8221
Add popover attribute #8221
Conversation
|
I see that there are many build errors after build.sh says "Running conformance checker...", but when I run build.sh locally, it just stops after "Success!". How do I run the conformance checker when building locally? |
|
The conformance checker is at https://github.com/validator/validator and has a variety of installation options. You need to run it on the output |
Thanks, I was able to run it! It looks like the PR builds now |
domenic
left a comment
There was a problem hiding this comment.
I managed to get a start on this review today... still more to go, but hopefully this helps.
|
Thanks for the review! I'm going to be out until monday so I'll try to get to it then |
|
Thanks for the review @domenic! I have addressed all your comments |
Kaiido
left a comment
There was a problem hiding this comment.
Not an authoritative review; but some questions that popped up while quickly reading the PR.
Since these attributes only apply to HTMLButtonElement and HTMLInputElement, it doesn't make sense to put them in the IDL for all Elements: whatwg/html#8221 (comment) Change-Id: I7038aefedbff78ec3af97b23c7a51a64cb0f275d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3894822 Commit-Queue: Joey Arhar <jarhar@chromium.org> Reviewed-by: Nate Fischer <ntfschr@chromium.org> Reviewed-by: David Baron <dbaron@chromium.org> Cr-Commit-Position: refs/heads/main@{#1048107}
|
@domenic mind taking another look? Anything else I can do to help move this forward? |
|
It's on my list, sorry! I had a week of TPAC then a week of vacation, so I'm currently down to 20 flagged work emails and 24 flagged GitHub emails :). |
|
Drive-by: element and attribute indices haven't been updated as far as I can tell. (FWIW, I plan on doing a more careful review.) |
domenic
left a comment
There was a problem hiding this comment.
Lots of editorial issues; as always, if you can be sure to do an extra sweep to make sure you catch multiple instances, that saves a lot of time.
On normative issues, I think these are the biggest outstanding ones:
- show/hide naming (under discussion in Open UI I guess, not sure how that's going)
- Element vs. HTMLElement; i.e., behavior for unknown, SVG, and MathML elements
- Some confusion about spec text for some animation pausing stuff
- Light dismiss spec intercepting capture events on Document is quite unusual; @annevk's help there would be appreciated steering us in the right direction
Looks like we might rename them to popupshow and popuphide: openui/open-ui#607
@mfreed7 Why did you implement all of the popup behavior in Element instead of HTMLElement? What do you think the behavior should be for unknown, SVG, and MathML elements? |
This was moved from the HTML spec PR for the popup attribute based on this advice: whatwg/html#8221 (comment) TODO add a better description of this
My general logic has been "why not?". I.e. we had the same debate in the TAG about limiting this feature to only some element types. And if there's a good reason to limit it, great. But if the only reason is that we don't know what it'll be used for, I think web developers will figure that out. For example, why not a pop-up SVG? I could think of some use cases where an icon (a "like button" for example) is made with SVG and would like to be a pop-up. Side note: this is currently broken in Chromium, but pending this discussion, I'll fix that. Just a missing set of rules in the UA stylesheet. |
The popover stack is constructed by attributes set on buttons. When those buttons are modified, it can break connections in the stack. This patch adds checks to spots where buttons can be modified in order to fix up the list by closing all popovers when a connection has been broken. This patch also moves the disabled check for popover*target attributes which Anne asked for here: whatwg/html#8221 (comment) Bug: 1307772, 1408546 Change-Id: I129cf2768abc717292b86ea26f7522019ce36d33
…y, a=testonly Automatic update from web-platform-tests Do not fire `beforetoggle` asynchronously Per the discussion at [1], we have decided not to fire async beforetoggle events at all. This means that no event will be fired in this case: ```javascript myPopover.showPopover(); myPopover.remove(); ``` [1] whatwg/html#8221 (comment) Bug: 1307772 Change-Id: Ie6d0f24f1181131fd6e15732020c0575cd3ba865 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4146026 Commit-Queue: Joey Arhar <jarhar@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Auto-Submit: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1090506} -- wpt-commits: 7c3aacf158efb050bdb81a1dd97ac324eaee799a wpt-pr: 37814
Feature proposal issue: #7785
Explainer: https://open-ui.org/components/popup.research.explainer
/browsers.html ( diff )
/dnd.html ( diff )
/dom.html ( diff )
/form-elements.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/input.html ( diff )
/interaction.html ( diff )
/interactive-elements.html ( diff )
/rendering.html ( diff )
/semantics-other.html ( diff )
/webappapis.html ( diff )
/popover.html ( diff )