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://un5x4n0kwa1t164zhzvdp2b4bu49r4r40ry9xdr.julianrbryant.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. |
|
FYI I just rewrote the "nearest open ancestral popover" (now "topmost popover ancestor") algorithm based on some feedback from foolip and a new implementation from Mason. It's mostly the same but should be easier to read and understand now |
annevk
left a comment
There was a problem hiding this comment.
I still found quite a few issues. I'm not entirely sure about the past issues as the force-pushing makes it hard to find those. Perhaps only force push every now and then and mainly stick to fixup commits?
I also noticed that the Elements index hasn't been updated. The input and button elements need to be modified.
Done
My process so far has basically just been squashing - every time I make changes, I create a new branch "popup.old.XX" and then squash everything down to one commit for the "popup" branch. If there is more than one commit to diff against then the diff view here where you leave comments will never load. I haven't actually done any actual rebasing to pick up upstream commits at all for months until this week because I wanted to deal with some merge conflicts and the conversation/feedback here was slowing down. I have made sure that every comment thread is marked as resolved, and if they aren't, I have been commenting on them. I haven't left a single one out. I don't think anyone besides you is actively reviewing, so at your request, I will not squash and force push. This will likely make the diff stop rendering/loading.
All the changes I have been making lately are in response to feedback in this PR. I have moved some things into followups: |
|
Oh wow indeed, I cannot seem to load individual commits even. That's rather broken. 😟 I can pull them locally though and the fixes made thus far look good. |
|
I'm really glad you added the asserts as they make it easier to spot bugs. E.g., I see nothing in "popover target attribute activation behavior" that prevents hitting step 2 of "show popover". |
|
@josepharhar confirmed out-of-band he thinks this is done-done now, so I took another look, especially at the "beforetoogle" event, which is new since I last reviewed. That setup makes sense to me, and AFAICT it's all wired up correctly against DOM, including the cancelable stuff. I'm going to go ahead and merge this now. It's a big change so no doubt some additional issues will be discovered as other implementers try to follow the spec, but I don't think this can be improved much by doing further review. |
|
@foolip we were still in a discussion upthread regarding some of the details here, including the name of the event. Could you please revert this change? This seems rather unusual. |
|
A lot of the activity is hidden behind "Load more…" so I thought there had been no further discussion in the past 3 weeks, but I see it's been going on the last few days. #8221 (comment) is that thread. |
|
Maybe given the size, we could just keep it landed and open a new PR with the event questions resolved? |
|
There are other errors too, such as |
|
@josepharhar could you please create a new PR and link it here? Thanks! |
Ok fair enough. I assume Github will allow this same PR to now be relanded with all the comments in place? |
Oh, guess not. |
|
I opened a new PR: #8717 |
@annevk can you clarify this one? I just expanded all of the comments and I don't see anything about this one. If this is true, I'd like to fix it in Chromium also. (Side note, this is a helpful script to expand all "Load More" sections. Very helpful on this PR.) |
|
I haven't done a full final review. I found that while looking at what got committed to determine how #8221 (comment) got resolved, which was never responded to. |
Apologies, I misinterpreted that comment. I believe that that the top layer assert can never be hit because the above call to "check popover validity" will return false in any case where the element is in the top layer, which will result in an exception being thrown instead of continuing to the assert. |
Feature proposal issue: #7785
Explainer: https://un5mvxtq4u1vaemmv4.julianrbryant.com/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 )