Skip to content

Ignore DOM state when hiding popovers if needed #9457

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Ignore DOM state when hiding popovers if needed #9457

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f60c3a0
Ignore DOM state when hiding popovers if needed
josepharhar Jun 23, 2023
acbbfdb
make removeattribute start firing events again
josepharhar Jul 11, 2023
d683a6b
fix args
josepharhar Jul 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 36 additions & 29 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1832,7 +1832,8 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute

<li><p>If <var>removedNode</var>'s <code data-x="attr-popover">popover</code> attribute is not in
the <span data-x="attr-popover-none-state">no popover state</span>, then run the <span>hide
popover algorithm</span> given <var>removedNode</var>, false, false, and false.</p></li>
popover algorithm</span> given <var>removedNode</var>, false, false, false, true, and
false.</p></li>
</ol>

<p>A <dfn id="insert-an-element-into-a-document" data-x="node is inserted into a document"
Expand Down Expand Up @@ -82411,7 +82412,7 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {

<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>
<var>element</var>, true, true, false, and true.</p></li>
Copy link

@ghost ghost Jul 12, 2023

Choose a reason for hiding this comment

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

This implies https://whatpr.org/html/9457/8430871...acbbfdb/popover.html#check-popover-validity will skip checking whether the popover attribute is in the "no popover" state. However, value can represent "auto" or "manual" and then the popover attribute should also correspond to it and hence should be checked, or?

Please be aware of the last paragraph of #9459 (comment).

Copy link
Contributor Author

@josepharhar josepharhar Jul 17, 2023

Choose a reason for hiding this comment

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

With the change in #9526 this should be fine

Copy link

@ghost ghost Jul 18, 2023 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

With the change in #9526 this should be fine

Depends on what "fine" means. That check should still pass, e.g. when the popover is open and the attribute state changes from "auto" to "manual" but with this patch the check wouldn't be executed.

Copy link
Contributor Author

@josepharhar josepharhar Jul 18, 2023

Choose a reason for hiding this comment

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

The reason that I added this here is because I want to make sure that the popover gets hidden even if the attribute is in the no popover state, so that the popover gets hidden when you call element.removeAttribute('popover').

However, value can represent "auto" or "manual" and then the popover attribute should also correspond to it and hence should be checked, or?

I suppose that we could pass false instead of true for ignoreDomState in the case that the new state is popover=auto or popover=manual, but what difference would it make? We still want to close the popover in these cases

but with this patch the check wouldn't be executed

You're talking about the check to see whether we are in the no popover state, right? In these cases we know that we are not in the no popover state already...

Copy link

@ghost ghost Jul 19, 2023

Choose a reason for hiding this comment

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

I suppose that we could pass false instead of true for ignoreDomState in the case that the new state is popover=auto or popover=manual, but what difference would it make?

It'd make the spec, and implementations, more robust. Otherwise, the latter's behaviors may differ, depending on their internals.

We still want to close the popover in these cases

Yes.

but with this patch the check wouldn't be executed

You're talking about the check to see whether we are in the no popover state, right?

Yes.

In these cases we know that we are not in the no popover state already...

Copy link

@ghost ghost Jul 19, 2023

Choose a reason for hiding this comment

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

I intend prototyping/implementing this in Firefox/Gecko, but it will take some time due to days off.

</ol>

<dl class="domintro">
Expand Down Expand Up @@ -82462,8 +82463,8 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
</li>

<li><p>If the result of running <span>check popover validity</span> given <var>element</var>,
false, <var>throwExceptions</var>, and null is false, then run <var>cleanupShowingFlag</var> and
return.</p></li>
false, <var>throwExceptions</var>, null, and false is false, then run
<var>cleanupShowingFlag</var> and return.</p></li>

<li><p><span>Assert</span>: <var>element</var> is not in <var>document</var>'s <span>top
layer</span>.</p></li>
Expand All @@ -82485,7 +82486,7 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {

<li>
<p>If the result of running <span>check popover validity</span> given <var>element</var>, false,
<var>throwExceptions</var>, and <var>document</var> is false, then run
<var>throwExceptions</var>, <var>document</var>, and false is false, then run
<var>cleanupShowingFlag</var> and return.</p>

<p class="note"><span>Check popover validity</span> is called again because firing the <code
Expand Down Expand Up @@ -82518,7 +82519,7 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {

<li>
<p>If the result of running <span>check popover validity</span> given <var>element</var>,
false, <var>throwExceptions</var>, and <var>document</var> is false, then run
false, <var>throwExceptions</var>, <var>document</var>, and false is false, then run
<var>cleanupShowingFlag</var> and return.</p>

<p class="note"><span>Check popover validity</span> is called again because running <span
Expand Down Expand Up @@ -82604,17 +82605,19 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
method steps are:</p>

<ol>
<li><p>Run the <span>hide popover algorithm</span> given <span>this</span>, true, true, and
true.</p></li>
<li><p>Run the <span>hide popover algorithm</span> given <span>this</span>, true, true, true, and
false.</p></li>
</ol>

<p>To <dfn data-x="hide popover algorithm">hide a popover</dfn> given an <span data-x="HTML
elements">HTML element</span> <var>element</var>, a boolean <var>focusPreviousElement</var>, a
boolean <var>fireEvents</var>, and a boolean <var>throwExceptions</var>:</p>
boolean <var>fireEvents</var>, a boolean <var>throwExceptions</var>, and a boolean
<var>ignoreDomState</var>:</p>

<ol>
<li><p>If the result of running <span>check popover validity</span> given <var>element</var>,
true, <var>throwExceptions</var>, and null is false, then return.</p></li>
true, <var>throwExceptions</var>, null, and <var>ignoreDomState</var> is false, then
return.</p></li>

<li><p>Let <var>document</var> be <var>element</var>'s <span>node document</span>.</p></li>

Expand Down Expand Up @@ -82644,8 +82647,8 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {

<li>
<p>If the result of running <span>check popover validity</span> given <var>element</var>,
true, and <var>throwExceptions</var> is false, then run <var>cleanupHidingFlag</var> and
return.</p>
true, <var>throwExceptions</var>, and <var>ignoreDomState</var> is false, then run
<var>cleanupHidingFlag</var> and return.</p>

<p class="note"><span>Check popover validity</span> is called again because running <span
data-x="hide-all-popovers-until">hide all popovers until</span> could have disconnected
Expand All @@ -82671,8 +82674,8 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {

<li>
<p>If the result of running <span>check popover validity</span> given <var>element</var>,
true, <var>throwExceptions</var>, and null is false, then run <var>cleanupHidingFlag</var> and
return.</p>
true, <var>throwExceptions</var>, null, and <var>ignoreDomState</var> is false, then run
<var>cleanupHidingFlag</var> and return.</p>

<p class="note"><span>Check popover validity</span> is called again because firing the <code
data-x="event-beforetoggle">beforetoggle</code> event could have disconnected
Expand Down Expand Up @@ -82720,8 +82723,8 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
<ol>
<li><p>If <span>this</span>'s <span>popover visibility state</span> is <span
data-x="popover-showing-state">showing</span>, and <var>force</var> is not present or false, then
run the <span>hide popover algorithm</span> given <span>this</span>, true, true, and
true.</p></li>
run the <span>hide popover algorithm</span> given <span>this</span>, true, true, true, and
false.</p></li>

<li><p>Otherwise, if <var>force</var> is not present or true, then run <span>show popover</span>
given <span>this</span> true, and null.</p></li>
Expand Down Expand Up @@ -82749,7 +82752,7 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {

<ol>
<li><p>Run the <span>hide popover algorithm</span> given <var>popover</var>,
<var>focusPreviousElement</var>, <var>fireEvents</var>, and false.</p></li>
<var>focusPreviousElement</var>, <var>fireEvents</var>, false, and false.</p></li>

<li><p>Set <var>popover</var> to <var>document</var>'s <span>topmost auto
popover</span>.</p></li>
Expand Down Expand Up @@ -82797,7 +82800,7 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
<ol>
<li><p>Run the <span>hide popover algorithm</span> given <var>document</var>'s <span>auto
popover list</span>'s last element, <var>focusPreviousElement</var>, <var>fireEvents</var>,
and false.</p></li>
false, and false.</p></li>
</ol>
</li>

Expand Down Expand Up @@ -82976,13 +82979,15 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {

<p>To <dfn>check popover validity</dfn> for an <span data-x="HTML elements">HTML element</span>
<var>element</var> given a boolean <var>expectedToBeShowing</var>, a boolean
<var>throwExceptions</var>, and a <code>Document</code> or null <var>expectedDocument</var>
perform the following steps. They throw an exception or return a boolean.</p>
<var>throwExceptions</var>, a <code>Document</code> or null <var>expectedDocument</var>, and a
boolean <var>ignoreDomState</var>, perform the following steps. They throw an exception or return
a boolean.</p>

<ol>
<li>
<p>If <var>element</var>'s <code data-x="attr-popover">popover</code> attribute is in the
<span data-x="attr-popover-none-state">no popover</span> state, then:</p>
<p>If <var>ignoreDomState</var> is false and <var>element</var>'s <code
data-x="attr-popover">popover</code> attribute is in the <span
data-x="attr-popover-none-state">no popover</span> state, then:</p>

<ol>
<li><p>If <var>throwExceptions</var> is true, then throw a
Expand Down Expand Up @@ -83010,10 +83015,11 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
<p>If one of the following conditions is true</p>

<ul>
<li><p><var>element</var> is not <span>connected</span></p></li>
<li><p><var>ignoreDomState</var> is false and <var>element</var> is not
<span>connected</span></p></li>

<li><p><var>expectedDocument</var> is not null and <var>element</var>'s <span>node
document</span> is not <var>expectedDocument</var></p></li>
<li><p><var>ignoreDomState</var> is false and <var>expectedDocument</var> is not null and
<var>element</var>'s <span>node document</span> is not <var>expectedDocument</var></p></li>

<li><p><var>element</var> is a <code>dialog</code> element and its <span>is modal</span> flag
is set to true.</li>
Expand Down Expand Up @@ -83165,12 +83171,12 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {

<li><p>If <var>popover</var>'s <span>popover visibility state</span> is <span
data-x="popover-showing-state">showing</span>, then run the <span>hide popover algorithm</span>
given <var>popover</var>, true, true, and false.</p></li>
given <var>popover</var>, true, true, false, and false.</p></li>

<li><p>Otherwise, if <var>popover</var>'s <span>popover visibility state</span> is <span
data-x="popover-hidden-state">hidden</span> and the result of running <span>check popover
validity</span> given <var>popover</var>, false, false, and null is true, then run <span>show
popover</span> given <var>popover</var>, false, and <var>node</var>.</p></li>
validity</span> given <var>popover</var>, false, false, null, and false is true, then run
<span>show popover</span> given <var>popover</var>, false, and <var>node</var>.</p></li>
</ol>

<p>To get the <dfn>popover target element</dfn> given a <code>Node</code> <var>node</var>, perform
Expand Down Expand Up @@ -83207,7 +83213,8 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
<span>topmost auto popover</span> showing, user agents may provide a user interface that, upon
activation, <span data-x="queue an element task">queues an element task</span> on the <span>user
interaction task source</span> given <span>topmost auto popover</span> to run the <span>hide
popover algorithm</span> given the <span>topmost auto popover</span>, true, true, and false.</p>
popover algorithm</span> given the <span>topmost auto popover</span>, true, true, false, and
false.</p>

<p>To <dfn>light dismiss open popovers</dfn>, given an <code>Event</code> <var>event</var>:</p>

Expand Down