Skip to content

Reinstate the hierarchy restrictions#91

Closed
foolip wants to merge 4 commits into
masterfrom
hierarchy-restrictions
Closed

Reinstate the hierarchy restrictions#91
foolip wants to merge 4 commits into
masterfrom
hierarchy-restrictions

Conversation

@foolip

@foolip foolip commented May 22, 2017

Copy link
Copy Markdown
Member

This reinstates parts of the fullscreen element ready check removed in
commit 766dc87 and
commit 9592913, with some
simplification of the iframe ancestor check.


Preview | Diff

This reinstates parts of the fullscreen element ready check removed in
commit 766dc87 and
commit 9592913, with some
simplification of the iframe ancestor check.
@foolip

foolip commented May 22, 2017

Copy link
Copy Markdown
Member Author

@upsuper, in addition to these changes, something needs to be done to uphold the invariant when mixed with dialog. I see two options:

What does Gecko do?

@upsuper

upsuper commented May 23, 2017

Copy link
Copy Markdown
Member

Sadly, Gecko still doesn't have dialog... so this is not applicable.

I don't recall exactly what the problems are for that...

It seems one of the problem would be that showModal may be called on a dialog element which is already a fullscreen element?

For that, I would probably prefer the second way, specifically, we pop out fullscreen elements until the dialog is no longer a fullscreen element, and then we add the element back as a dialog.

But thinking about this a bit further, this option is probably unnecessarily complicated, because that means we would need to consider dispatching event, and even unfullscreen the window, for a showModal call, which sounds unfortunate.

Given this, I guess we should make HTML avoid invoking top-layer-add when the element is already a fullscreen element by rejecting it with a exception, and I think we should assert in top-layer-add that the element to be append is not currently an element in the top layer.

If we add that assertion, we would also need to think about what if a modal dialog requests fullscreen. Maybe we should reject as well. I don't think those cases are common, and I don't have idea how authors may find those cases useful...

Comment thread fullscreen.bs
<li><p><var>element</var> is <a>connected</a>.

<li><p><var>element</var>'s <a>node document</a>'s <a>fullscreen element</a> is null, or is an
<a>inclusive ancestor</a> of <var>element</var>.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You also need to invoke this check recursively for its browsing context container if any.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh, right, the ready check used to be recursive but I removed that in #52.

@foolip

foolip commented May 23, 2017

Copy link
Copy Markdown
Member Author

I guess we should make HTML avoid invoking top-layer-add when the element is already a fullscreen element by rejecting it with a exception

Yep, we could add another "InvalidStateError" case to https://html.spec.whatwg.org/#dom-dialog-showmodal

If we add that assertion, we would also need to think about what if a modal dialog requests fullscreen. Maybe we should reject as well.

Actually, if we add a check for dialog in requestFullscreen(), then no check should be needed in showModal(). With the current state of things, no dialog elements in top layer would be fullscreen, and all non-dialog elements would be.

@upsuper

upsuper commented May 23, 2017

Copy link
Copy Markdown
Member

Actually, if we add a check for dialog in requestFullscreen(), then no check should be needed in showModal(). With the current state of things, no dialog elements in top layer would be fullscreen, and all non-dialog elements would be.

I mean we check whether the element is a modal dialog, not that whether it is a dialog element. But well, I guess we can just simple reject any dialog element from entering fullscreen if that sounds simpler... but could that be breaking?

@foolip

foolip commented May 23, 2017

Copy link
Copy Markdown
Member Author

It could be, I'll add use counters in Blink both for when the element is a dialog element and when it is additionally in the top layer, which would mean that it's showing as modal. I think it's fairly unlikely that this would break real content, but we could leave it as an open issue until the results are in, or make a tentative change and revert if necessary.

@foolip

foolip commented May 23, 2017

Copy link
Copy Markdown
Member Author

@foolip

foolip commented May 23, 2017

Copy link
Copy Markdown
Member Author

OK, I've pushed something that should work. I worry a bit about making the ready check recursive again, because it's a cross-process access and can't actually be checked synchronously. In other words, hierarchy restrictions is a simplifications in some ways, but not in others. However, I lean towards having them for the time being.

@upsuper, are you OK with the dialog bit, or should I add a TODO there and file a separate issue, blocked on usage data? Yet another possibility would be to change https://fullscreen.spec.whatwg.org/#top-layer-add to do nothing if the element already exists, so that things can never move in the top layer. Then showModal() would need to handle that situation instead.

@upsuper

upsuper commented May 24, 2017

Copy link
Copy Markdown
Member

I think this is fine. I'd still want to also change the "top-layer-add" algorithm to not move things around, but instead assert that the element to add is not in the top layer currently. It seems to me the HTML spec doesn't allow a open dialog to be opened again, so it wouldn't break that assertion either.

@foolip

foolip commented May 24, 2017

Copy link
Copy Markdown
Member Author

Yeah, I think you're right that such an assert would hold without any other changes. I've pushed the assert you suggested, and think that whatwg/html#2341 might need a tweak for that.

MXEBot pushed a commit to mirror/chromium that referenced this pull request May 25, 2017
To answer questions in whatwg/fullscreen#91.

BUG=402376

Change-Id: Ibb7d2dcc271bdf18b094b5d100db52e60be9ca9a
Reviewed-on: https://chromium-review.googlesource.com/512163
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474270}
foolip added a commit that referenced this pull request Sep 26, 2017
This way, interactions between the algorithms for fullscreen and
dialog are simplified. Example concern:
#102 (comment)

This also makes it easier to reinstate hierarchy restrictions:
#91

Tests: https://chromium-review.googlesource.com/c/chromium/src/+/684435
foolip added a commit that referenced this pull request Sep 26, 2017
This way, interactions between the algorithms for fullscreen and
dialog are simplified. Example concern:
#102 (comment)

This also makes it easier to reinstate hierarchy restrictions:
#91

Tests: https://chromium-review.googlesource.com/c/chromium/src/+/684435
annevk pushed a commit that referenced this pull request Sep 27, 2017
This way, interactions between the algorithms for fullscreen and
dialog are simplified. Example concern:
#102 (comment)

This also makes it easier to reinstate hierarchy restrictions:
#91

Tests: https://chromium-review.googlesource.com/c/chromium/src/+/684435
@foolip

foolip commented Aug 29, 2018

Copy link
Copy Markdown
Member Author

Abandoning this.

@foolip foolip closed this Aug 29, 2018
@foolip foolip deleted the hierarchy-restrictions branch August 29, 2018 13:16
@upsuper

upsuper commented Sep 20, 2018

Copy link
Copy Markdown
Member

Hmmm... why do we abounding this?

I think this was raised due to concern on fullscreenchange event for exiting fullscreen wouldn't go through every fullscreen element in the stack when there are multiple leaf fullscreen elements, which I raised in #89 (comment) and you agreed that it makes sense.

Web platform test fullscreen/api/element-ready-check-fullscreen-element-sibling-manual.html checks this behavior and it's currently failing on Gecko as we still enforce the hierarchy restrictions, and given that concern I think that still makes sense. So we should revive this PR and change that test.

@foolip

foolip commented Sep 21, 2018

Copy link
Copy Markdown
Member Author

Sorry, I moved a bit fast when abandoning old PRs after #128 obsoleted a lot of them. I'm a bit swamped until mid-next-week, but will come around to #140.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants