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

Initialize nestedHide variable in the "hide a popover" algorithm #9419

Conversation

mbrodesser-Igalia
Copy link
Member

@mbrodesser-Igalia mbrodesser-Igalia commented Jun 13, 2023

Fixes #9412.


/popover.html ( diff )

source Outdated
Comment on lines 82636 to 82639
<li><p>Let <var>nestedHide</var> be false.</p></li>

<li><p>If <var>element</var>'s <span>popover showing or hiding</span> is true, then set
<var>nestedHide</var> to true.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

Let's combine these two steps as

Let nestedHide be true if element's popover showing or hiding is true; otherwise false.

Copy link
Member Author

Choose a reason for hiding this comment

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

@annevk: Let nestedHide be the value of element's popover showing or hiding flag instead?

That flag isn't explicitly defined as a flag (https://html.spec.whatwg.org/#popover-showing-or-hiding), but it should be.

Copy link
Member

@annevk annevk Jun 14, 2023

Choose a reason for hiding this comment

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

Oh right, in that case it can be

Let nestedHide be element's popover showing or hiding.

I think.

(We're moving away from calling things flags and use the boolean type instead.)

Copy link
Member Author

Choose a reason for hiding this comment

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

(We're moving away from calling things flags and use the boolean type instead.)

Thanks, TIL.

Added 's value, for the rationale see the commit message.

@annevk annevk added the topic: popover The popover attribute and friends label Jun 13, 2023
…hub.com:mbrodesser-Igalia/html into hide_popover_algo_nested_hide_set_initial_value
…its flag

Initializing `nestedHide` hide with the value makes it clear that it's
not a reference to `popover showing or hiding`.
<li><p>If <var>element</var>'s <span>popover showing or hiding</span> is true, then set
<var>nestedHide</var> to true.</p></li>
<li><p>Let <var>nestedHide</var> be <var>element</var>'s <span>popover showing or
hiding</span>'s value.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

Why would we need "'s value" here but not in the step above?

Copy link
Member Author

Choose a reason for hiding this comment

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

"document" has to represent a reference to the node document, not a copy.
"nestedHide" has to be a copy of "popover showing or hiding". Changing the latter in step 4 shouldn't change the former.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably be better about documenting this in Infra, but we generally follow JS semantics so a copy would be implied here due to it holding a primitive type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, will remove "value" then.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably be better about documenting this in Infra, but we generally follow JS semantics so a copy would be implied here due to it holding a primitive type.

For the record, there's whatwg/infra#139 for it.

The standard follows the JS semantics, so a copy of the primitive is
implied.
@annevk annevk merged commit f49c3d9 into whatwg:main Jun 14, 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.

https://html.spec.whatwg.org/#hide-popover-algorithm lacks initial value for nestedHide variable
2 participants