Bug 229488 – Re-generalize top layer element concept

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

After https://commits.webkit.org/r281548, the concept of top layer is confined to dialog element.
Re-generalize this concept for all elements so that we can use it in the full screen element.

We'd introduce a new node flag to avoid incurring any perf cost.

[Ryosuke Niwa]

Created attachment 436377 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 436377 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436377&action=review

> Source/WebCore/dom/Document.cpp:8474
>      m_topLayerElements.appendOrMoveToLast(element);

RELEASE_ASSERT(&element.document() == this && element.isConnected() && !element.isInTopLayer());
m_topLayerElements.append(element);

Since we're asserting `!element.isInTopLayer()`, let's use append() given the moveToLast case should not be possible.

moveToLast was supposed to be to follow spec words: "To add an element to a top layer, remove it from top layer and then append it to top layer."

Though according to both the dialog and fullscreen specs, movingToLast is not a case that can't happen: dialog requires checking dialog not being in top layer before adding it in, fullscreen does not allow dialogs to be fullscreened. Some other specs use the top layer (google's <popup> element), but they're not heavily implemented yet.

[Antti Koivisto]

Comment on attachment 436377 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436377&action=review

> Source/WebCore/dom/Element.cpp:3398
> +    invalidateStyle();

I think this can use invalidateStyleInternal(). The difference is that invalidateStyle() may also invalidate siblings. That is only needed for things that flip pseudo classes (yes, these need better names).

[Ryosuke Niwa]

(In reply to Tim Nguyen (:ntim) from comment #2)
> Comment on attachment 436377 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436377&action=review
> 
> > Source/WebCore/dom/Document.cpp:8474
> >      m_topLayerElements.appendOrMoveToLast(element);
> 
> RELEASE_ASSERT(&element.document() == this && element.isConnected() &&
> !element.isInTopLayer());
> m_topLayerElements.append(element);
> 
> Since we're asserting `!element.isInTopLayer()`, let's use append() given
> the moveToLast case should not be possible.

Yeah, I was thinking that. Will do.

(In reply to Antti Koivisto from comment #3)
> Comment on attachment 436377 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436377&action=review
> 
> > Source/WebCore/dom/Element.cpp:3398
> > +    invalidateStyle();
> 
> I think this can use invalidateStyleInternal(). The difference is that
> invalidateStyle() may also invalidate siblings. That is only needed for
> things that flip pseudo classes (yes, these need better names).

Okay, will do. I guess I'd let it cycle through EWS again to be safe.

[Ryosuke Niwa]

Huh, I just noticed that we're leaking top layer elements. I'd fix that as well.

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #5)
> Huh, I just noticed that we're leaking top layer elements. I'd fix that as
> well.

Clarify: we'd be leaking the entire document whenever there is a top layer element when the document loses its last ref.

[Ryosuke Niwa]

Created attachment 436804 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 436804 [details]
Patch for landing

Wait for EWS.

[EWS]

Found 5 new test failures: imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-cancel-with-select.html, imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-return-value.html, imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/element-removed-from-top-layer-has-original-position.html, imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/modal-dialog-display-contents.html, imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/top-layer-display-none.html

[Ryosuke Niwa]

Created attachment 436823 [details]
Patch for landing

[Ryosuke Niwa]

Created attachment 436824 [details]
Patch for landing

[EWS]

Committed r281793 (241130@main): <https://commits.webkit.org/241130@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436824 [details].

[Radar WebKit Bug Importer]

<rdar://problem/82568313>

[Tim Nguyen (:ntim)]

*** Bug 229149 has been marked as a duplicate of this bug. ***