Remove the pending dialog stack in favor of the top layer#2341
Conversation
| dialog">block</span> a <code>Document</code>, the previous element, if any, stops blocking the | ||
| <code>Document</code>.</p> | ||
| <p>A <code>Document</code> <var>document</var> is <dfn>blocked by a modal dialog</dfn> | ||
| <var>subject</var> if <var>subject</var> is the topmost <code>dialog</code> element in |
There was a problem hiding this comment.
With a dialog in a dialog, I think that nothing prevents dialog.firstChild.requestFullscreen() while this "blocked by a modal dialog" definition holds true for the outermost dialog, which would make it such that the innermost dialog fullscreen element becomes the topmost dialog element in document's top layer.
The vague "may provide a user interface" that applies when blocked by a modal dialog makes me unsure if this is a problem. I can't immediately find a place where Blink does something like this. @upsuper, does this map onto something in Gecko?
There was a problem hiding this comment.
I don't immediately recall anything like that in the WIP impl. Actually I'm confused why user agent needs to provide such user interface.
And yeah, I think the sentence here could be problematic. The document should be blocked by the modal dialog only when the dialog is the topmost element in the top layer, not just the topmost dialog element, otherwise if an element outside the dialog requests fullscreen, the fullscreen element would be blocked.
upsuper
left a comment
There was a problem hiding this comment.
It looks otherwise fine to me. The sentence about "blocked by a modal dialog" looks problematic to me.
| dialog">block</span> a <code>Document</code>, the previous element, if any, stops blocking the | ||
| <code>Document</code>.</p> | ||
| <p>A <code>Document</code> <var>document</var> is <dfn>blocked by a modal dialog</dfn> | ||
| <var>subject</var> if <var>subject</var> is the topmost <code>dialog</code> element in |
There was a problem hiding this comment.
I don't immediately recall anything like that in the WIP impl. Actually I'm confused why user agent needs to provide such user interface.
And yeah, I think the sentence here could be problematic. The document should be blocked by the modal dialog only when the dialog is the topmost element in the top layer, not just the topmost dialog element, otherwise if an element outside the dialog requests fullscreen, the fullscreen element would be blocked.
|
Chrome does implement the
in particular via the Esc key. Try it out at https://output.jsbin.com/jutokaloyi Regarding whether "blocked by a modal dialog" should be true only when the topmost element of the top layer is a dialog, or whether it should be true when the top layer contains a dialog at all, I went with the latter, because it seems very weird that fullscreening something suddenly causes things below the dialog to become non-inert. Remember that "blocked by a modal dialog" is mostly about making things inert. |
|
Thanks @domenic, this makes sense to me:
I have nothing further on this PR. @upsuper? |
|
OK, I guess this is fine for now. I think we would eventually make it be "blocked by the top layer". Having this as an intermediate state of the spec is fine to me. |
|
Great. I'll be able to merge this as soon as we have a PR to web-platform-tests. |
|
|
||
| <li><p>Push <var>subject</var> onto <var>subject</var>'s | ||
| <span>node document</span>'s <span>pending dialog stack</span>.</p></li> | ||
| <li><p><span data-x="top-layer-add">Add</span> <var>subject</var> onto <var>subject</var>'s |
There was a problem hiding this comment.
In whatwg/fullscreen#91 I'm adding an assert that elements aren't added to top layer twice, a change from the previous behavior or moving elements within top layer. Here, can you prepend "If top layer does not already contain subject"?
|
What kinds of tests are needed here? The one I can think of is:
That shouldn't throw an exception or hit the assert (obviously), and should add back the open attribute. Then there's also the question of reordering within top layer, which shouldn't happen. I think that could be tested using Any volunteers to write the tests? I'd be happy to review. |
9a11d63 to
5835a07
Compare
|
Those tests sound good to me. I thought for a bit we were changing the inert behavior of fullscreen, but I guess the conclusion was not to do that, so currently "blocked by a modal dialog" only applies to dialogs, not to other things in the top layer. I do hope someone is up for writing those tests. |
Follow whatwg/html#2341. Only the "multiple dialogs" test is testing something that changed in that PR, ensuring that elements cannot move within the top layer. That test will cause a harness error in Chromium because the d11.close() cleanup step will throw an exception: https://bugs.chromium.org/p/chromium/issues/detail?id=638943
Follows whatwg/html#2341. Only the "multiple dialogs" test is testing something that changed in that PR, ensuring that elements cannot move within the top layer. That test will cause a harness error in Chromium because the d11.close() cleanup step will throw an exception: https://bugs.chromium.org/p/chromium/issues/detail?id=638943
|
OK, web-platform-tests/wpt#7249 is reviewed, and this too, so merging away. |
Follows whatwg/html#2341. Only the "multiple dialogs" test is testing something that changed in that PR, ensuring that elements cannot move within the top layer. That test will cause a harness error in Chromium because the d11.close() cleanup step will throw an exception: https://bugs.chromium.org/p/chromium/issues/detail?id=638943
Follows whatwg/html#2341. Only the "multiple dialogs" test is testing something that changed in that PR, ensuring that elements cannot move within the top layer. That test will cause a harness error in Chromium because the d11.close() cleanup step will throw an exception: https://bugs.chromium.org/p/chromium/issues/detail?id=638943
Follows whatwg/html#2341. Only the "multiple dialogs" test is testing something that changed in that PR, ensuring that elements cannot move within the top layer. That test will cause a harness error in Chromium because the d11.close() cleanup step will throw an exception: https://bugs.chromium.org/p/chromium/issues/detail?id=638943
This avoids the problems noted in #2268 with attempting to keep the two
different stacks in sync, and overall simplifies the model. Fixes #2268.
This commit also updates the definition of "blocked by a modal dialog"
to account for shadow DOM.
/cc @upsuper @foolip @nt1m. I'm hoping that one of the Gecko implementers asking for this change can help contribute the appropriate web platform test.