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

Hide popovers when making an element fullscreen #204

Merged
merged 16 commits into from
May 22, 2023
Merged

Hide popovers when making an element fullscreen #204

merged 16 commits into from
May 22, 2023

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Oct 18, 2022 •
edited by pr-preview bot
Loading

This is part of the new popover attribute: whatwg/html#8221

Preview | Diff

annevk
annevk reviewed Nov 14, 2022
View reviewed changes
fullscreen.bs Outdated Show resolved Hide resolved
fullscreen.bs Outdated Show resolved Hide resolved
fullscreen.bs Outdated Show resolved Hide resolved
@annevk annevk requested a review from foolip November 14, 2022 09:33
foolip
foolip reviewed Nov 24, 2022
View reviewed changes
fullscreen.bs Outdated Show resolved Hide resolved
josepharhar and others added 2 commits February 14, 2023 10:53
@josepharhar @annevk
Fix popover showing state reference
15648d0 
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@josepharhar @annevk
Replace pronouns
cf5bc17 
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@josepharhar josepharhar mentioned this pull request Feb 14, 2023
Closed
josepharhar added a commit to josepharhar/html that referenced this pull request Feb 14, 2023
@josepharhar
Export some popover dfns
00782c9 
These dfns need to be exported in order to be used in the fullscreen
spec: whatwg/fullscreen#204
@josepharhar josepharhar mentioned this pull request Feb 14, 2023
Closed
4 tasks
josepharhar added a commit to josepharhar/html that referenced this pull request Feb 14, 2023
@josepharhar
Add "hide all popovers" algorithm
ea851d1 
This was requested for cross referencing instead of "hide all popovers
until" here:
whatwg/fullscreen#204 (comment)

This patch also fixes a bug where a null endpoint is passed to hide all
popovers until, but hide all popovers until assumes endpoint is not null
by looking for endpoint's node document. The bug is fixed by adding a
required document parameter to hide all popovers until.
@josepharhar josepharhar mentioned this pull request Feb 14, 2023
Merged
4 tasks
josepharhar added a commit to josepharhar/html that referenced this pull request Feb 17, 2023
@josepharhar
Export some popover dfns
0b7026f 
These dfns need to be exported in order to be used in the fullscreen
spec: whatwg/fullscreen#204
annevk pushed a commit to whatwg/html that referenced this pull request Feb 23, 2023
@josepharhar
Add "hide all popovers" algorithm for Fullscreen
6bb5df0 
This also fixes a bug where a null endpoint is passed to hide all popovers until, but hide all popovers until assumes endpoint is not null by looking for endpoint's node document. The bug is fixed by letting hide all popovers until take a document.

And it also exports some definitions for Fullscreen.

Corresponding Fullscreen PR: whatwg/fullscreen#204.
@josepharhar josepharhar changed the title Hide pop-ups when making an element fullscreen Hide popovers when making an element fullscreen Mar 25, 2023
@josepharhar
Copy link
Contributor Author

This is ready for review now @annevk @foolip

foolip
foolip reviewed Apr 6, 2023
View reviewed changes
fullscreen.bs Outdated Show resolved Hide resolved
foolip
foolip reviewed Apr 6, 2023
View reviewed changes
fullscreen.bs Outdated Show resolved Hide resolved
foolip
foolip reviewed Apr 6, 2023
View reviewed changes
fullscreen.bs Outdated Show resolved Hide resolved
foolip
foolip reviewed Apr 6, 2023
View reviewed changes
fullscreen.bs Outdated Show resolved Hide resolved
foolip
foolip reviewed Apr 6, 2023
View reviewed changes
fullscreen.bs Outdated Show resolved Hide resolved
foolip
foolip reviewed Apr 6, 2023
View reviewed changes
fullscreen.bs Outdated Show resolved Hide resolved
foolip
foolip reviewed Apr 6, 2023
View reviewed changes
Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

This looks good to me now, just one remaining issue.

fullscreen.bs Outdated Show resolved Hide resolved
@foolip
Copy link
Member

foolip commented Apr 6, 2023

@josepharhar can you chase down Gecko and WebKit support for this change, and link to implementation bugs?

@josepharhar @foolip
add HTML element check
a1b9c10 
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
@josepharhar
Copy link
Contributor Author

can you chase down Gecko and WebKit support for this change, and link to implementation bugs?

I filed bugs and added them to the PR description.
@annevk @nt1m are you supportive of this change?

@josepharhar josepharhar mentioned this pull request Apr 17, 2023
Open
@tabatkins tabatkins mentioned this pull request Apr 20, 2023
Merged
4 tasks
@foolip
Copy link
Member

foolip commented Apr 25, 2023

@nt1m if I'm reading WebKit/WebKit#11155 correctly, you implemented this behavior in FullscreenManager.cpp, but I'm unsure if all of the isInPopoverShowingState() checks are intended to always pass, or if there are also cases where a fullscreen request will be rejected because of an open popover?

If that PR matches this spec, can we consider that support from WebKit to make this change? I'd like to get it merged because it makes #223 easier to understand, in particular it guarantees (in the spec) that certain combinations of fullscreen and popovers can't happen.

annevk
annevk approved these changes Apr 28, 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.

This seems okay. I'll ping @nt1m again as well.

fullscreen.bs Outdated Show resolved Hide resolved
@nt1m
Copy link
Member

nt1m commented Apr 28, 2023

@nt1m if I'm reading WebKit/WebKit#11155 correctly, you implemented this behavior in FullscreenManager.cpp, but I'm unsure if all of the isInPopoverShowingState() checks are intended to always pass, or if there are also cases where a fullscreen request will be rejected because of an open popover?

I think our implementation matches this PR, the isInPopoverShowingState() checks are for the "fullscreen element ready check". We could probably refactor to make this more clear, but that's how things are currently setup.

@annevk annevk merged commit afd56a3 into whatwg:main May 22, 2023
@josepharhar josepharhar deleted the popup branch January 29, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@foolip foolip foolip left review comments

@annevk annevk annevk approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@josepharhar @foolip @nt1m @annevk