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.

Sign up for GitHub

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

Jump to bottom

Throw exception in originalType check #9058

Merged
merged 4 commits into from
May 26, 2023
Merged

Throw exception in originalType check #9058

merged 4 commits into from
May 26, 2023

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Mar 21, 2023 •
edited by annevk
Loading

Since exception throwing was moved into "check popover validity," we need to explicitly throw an exception for this originalType check in order to make sure that it actually throws an exception like it is supposed to.

Fixes #9037

  • At least two implementers are interested (and none opposed):
    • Chromium
    • WebKit
  • Tests are written and can be reviewed and commented upon at:
    • popover-attribute-basic.html
  • Implementation bugs are filed:
    • Chromium: Implemented
    • Gecko: …
    • WebKit: …

/popover.html ( diff )

@josepharhar
Throw exception in originalType check
204de34 
Since exception throwing was moved into "check popover validity," we
need to explicitly throw an exception for this originalType check in
order to make sure that it actually throws an exception like it is
supposed to.

Fixes whatwg#9037
@josepharhar josepharhar mentioned this pull request Mar 21, 2023
Closed
annevk
annevk reviewed Mar 22, 2023
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably not land this until #9045 is resolved?

cc @jakearchibald @mfreed7

source Outdated Show resolved Hide resolved
@jakearchibald
Copy link
Contributor

What would the non-throwing alternative be here? Silently abandoning the show operation is one possibility. Or allow it to complete, then run the hide operation.

@rwlbuis rwlbuis mentioned this pull request Mar 23, 2023
Merged
webkit-commit-queue pushed a commit to rwlbuis/WebKit that referenced this pull request Mar 23, 2023
@rwlbuis @nt1m
[popover] Handle showPopover changing popover attribute during before…
8cbe612 
…toggle

https://bugs.webkit.org/show_bug.cgi?id=254268

Reviewed by Tim Nguyen.

Handle showPopover changing popover attribute during beforetoggle by throwing an exception:
whatwg/html#9058

* LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-attribute-basic-expected.txt:
* LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/html/semantics/popovers/popover-attribute-basic-expected.txt:
* Source/WebCore/html/HTMLElement.cpp:
(WebCore::HTMLElement::showPopover):

Canonical link: https://commits.webkit.org/262026@main
@nt1m nt1m added the topic: popover The popover attribute and friends label Mar 29, 2023
@mfreed7
Copy link
Contributor

mfreed7 commented Mar 31, 2023

We should probably not land this until #9045 is resolved?

cc @jakearchibald @mfreed7

Correct me if I'm wrong, but #9045 doesn't specifically consider this case. This is a fairly corner case: while showing a popover, other descendant popovers are hidden, and one of the event handlers for those popovers changes the popover type of this popover.

What would the non-throwing alternative be here? Silently abandoning the show operation is one possibility. Or allow it to complete, then run the hide operation.

I (still) think silently abandoning the show makes the most sense in this corner case. Remember that most of these exceptions were added because there was a feeling (during the review for the original popover PR) that exceptions needed to be thrown everywhere when possible. I suppose #9045 is providing more evidence that we should err on the side of fewer exceptions.

@annevk
Copy link
Member

annevk commented Apr 5, 2023

Not specifically no, but if we are going to change what exceptions to throw we should consider all cases and change them all at once.

@josepharhar
Copy link
Contributor Author

Not specifically no, but if we are going to change what exceptions to throw we should consider all cases and change them all at once.

I also have a PR to make more exceptions changes here: #9182
Do you want me to close this one, that one, and #9045 and make a new one that includes all of them?

@annevk
Copy link
Member

annevk commented Apr 20, 2023

Having a single PR for all exception changes sounds like a good idea, yes. If it's easier to update one of the existing PRs that's fine too. Just need to update the commit message and the issues it will end up closing.

@nt1m
Copy link
Member

nt1m commented May 21, 2023

@josepharhar Is this still relevant?

@josepharhar
Copy link
Contributor Author

Yes, this is still relevant. Chromium has this change implemented here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=1501-1511;drc=dcd4fe5a57e77e434ad1df6a22fb7ee013fe9ed4

@annevk
Copy link
Member

annevk commented May 23, 2023

Can you rebase this then?

@josepharhar
Copy link
Contributor Author

I resolved the merge conflicts

@annevk annevk merged commit 1a7826b into whatwg:main May 26, 2023
@YusukeHirao YusukeHirao mentioned this pull request Sep 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk approved these changes

Assignees
No one assigned
Labels
topic: popover The popover attribute and friends
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WPT subtest testing beforetoggle during showPopover()
5 participants
@josepharhar @jakearchibald @mfreed7 @annevk @nt1m