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.

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

Add popover open check in attribute change steps #9526

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Jul 17, 2023

Fixes #9459

Chromium has already had this check for some time, and without it firefox hits an assertion:
#9459

(See WHATWG Working Mode: Changes for more details.)


/popover.html ( diff )

Fixes whatwg#9459

Chromium has already had this check for some time, and without it
firefox hits an assertion:
whatwg#9459
@ziransun
Copy link

Firfox will introduce this check at https://phabricator.services.mozilla.com/D181880. Thanks @josepharhar!

<li><p>If <var>oldValue</var> and <var>value</var> are in different <span
data-x="attr-popover">states</span>, then run the <span>hide popover algorithm</span> given
<var>element</var>, true, true, and false.</p></li>
<li><p>If <var>element</var>'s <span>popover visibility state</span> is in the <span
Copy link
Member

Choose a reason for hiding this comment

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

The change proposed in #9459 (comment) is slightly different. Was the second check skip intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah I see you mentioned checking the popover showing or hiding flag.
Does your jsfiddle from this comment show why checking the popover showing or hiding flag helps?

Copy link
Member

Choose a reason for hiding this comment

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

@josepharhar: sorry, due to the complexity of the topic and lack of time I currently can't answer that question.

@domenic domenic added the topic: popover The popover attribute and friends label Jul 24, 2023
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.

@nt1m would you be able to review this? It looks editorially fine, but would be nice if it at least one more implementer signed off too.

@annevk annevk requested a review from nt1m September 19, 2023 09:32
@domenic domenic merged commit 0d0ac9f into whatwg:main Oct 24, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: popover The popover attribute and friends
Development

Successfully merging this pull request may close these issues.

When attribute value changes, do we run hiding popover algorithm regardless?
5 participants