Bug 227907 – Check for dialog existence in top layer in showModal/close

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

[Tim Nguyen (:ntim)]

Current behaviour is wrong, especially for showModal, since it will wrongly shuffle around the order, if the dialog is already in the top layer.

This needs a isInTopLayer method.

[Radar WebKit Bug Importer]

<rdar://problem/80523132>

[Tim Nguyen (:ntim)]

Created attachment 435479 [details]
Patch

[Tim Nguyen (:ntim)]

Committed r281014 (240504@main): <https://commits.webkit.org/240504@main>

[Darin Adler]

Comment on attachment 435479 [details]
Patch

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

> Source/WebCore/html/HTMLDialogElement.cpp:69
> +    if (!isInTopLayer())
> +        document().addToTopLayer(*this);

This is the double hashing solution, looking in topLayerElements once to see if the element is there and then doing the same hashing in topLayerElements again to add the element. If the addToTopLayer performed this check, it could almost certainly do it without hashing twice.

> Source/WebCore/html/HTMLDialogElement.cpp:89
> +    if (isInTopLayer())
> +        document().removeFromTopLayer(*this);

I don’t see why this change is needed. This would have no effect even if top layer rendering is implemented.

[Tim Nguyen (:ntim)]

(In reply to Darin Adler from comment #4)
> Comment on attachment 435479 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435479&action=review
> 
> > Source/WebCore/html/HTMLDialogElement.cpp:69
> > +    if (!isInTopLayer())
> > +        document().addToTopLayer(*this);
> 
> This is the double hashing solution, looking in topLayerElements once to see
> if the element is there and then doing the same hashing in topLayerElements
> again to add the element. If the addToTopLayer performed this check, it
> could almost certainly do it without hashing twice.

addToTopLayer adds to the top layer if not in it, and moves to the last position if it is in it. This is what the top layer spec says: "to add an element to the top layer, remove it first then add it again".

Moving this check in addToTopLayer would change that behaviour (which made the test fail). It's also worth noting that the new fullscreen API should re-use this top layer concept.

> > Source/WebCore/html/HTMLDialogElement.cpp:89
> > +    if (isInTopLayer())
> > +        document().removeFromTopLayer(*this);
> 
> I don’t see why this change is needed. This would have no effect even if top
> layer rendering is implemented.

I guess it doesn't matter much right now, this is just following spec words:

https://html.spec.whatwg.org/multipage/interactive-elements.html#dom-dialog-close
If subject is in its Document's top layer, then remove it.

It might matter a bit more if we add more things in the `removeFromTopLayer` function (e.g. fullscreen API related, rendering related, etc.).

[Darin Adler]

Comment on attachment 435479 [details]
Patch

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

I understand your responses, but I’m still a little disappointed that we end up doing double hashing.

>>> Source/WebCore/html/HTMLDialogElement.cpp:69
>>> +        document().addToTopLayer(*this);
>> 
>> This is the double hashing solution, looking in topLayerElements once to see if the element is there and then doing the same hashing in topLayerElements again to add the element. If the addToTopLayer performed this check, it could almost certainly do it without hashing twice.
> 
> addToTopLayer adds to the top layer if not in it, and moves to the last position if it is in it. This is what the top layer spec says: "to add an element to the top layer, remove it first then add it again".
> 
> Moving this check in addToTopLayer would change that behaviour (which made the test fail). It's also worth noting that the new fullscreen API should re-use this top layer concept.

Understood.

We could still refactor to avoid double hashing, it’s just a bit more complex. We’d need to write a "addToTopLayer if not already present" function, and factor things so that it’s only a single hash table lookup. One way to do that is an argument that changes the behavior of addToTopLayer.

>>> Source/WebCore/html/HTMLDialogElement.cpp:89
>>> +        document().removeFromTopLayer(*this);
>> 
>> I don’t see why this change is needed. This would have no effect even if top layer rendering is implemented.
> 
> I guess it doesn't matter much right now, this is just following spec words:
> 
> https://html.spec.whatwg.org/multipage/interactive-elements.html#dom-dialog-close
> If subject is in its Document's top layer, then remove it.
> 
> It might matter a bit more if we add more things in the `removeFromTopLayer` function (e.g. fullscreen API related, rendering related, etc.).

Sure, but if we add such things, we’d just need to make sure that removeFromTopLayer does nothing if it’s not a top layer. That’s going to be really convenient since hash table removal functions already tell you if the thing was removed or not.

[Tim Nguyen (:ntim)]

(In reply to Darin Adler from comment #6)
> Comment on attachment 435479 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435479&action=review
> 
> I understand your responses, but I’m still a little disappointed that we end
> up doing double hashing.
> 
> >>> Source/WebCore/html/HTMLDialogElement.cpp:69
> >>> +        document().addToTopLayer(*this);
> >> 
> >> This is the double hashing solution, looking in topLayerElements once to see if the element is there and then doing the same hashing in topLayerElements again to add the element. If the addToTopLayer performed this check, it could almost certainly do it without hashing twice.
> > 
> > addToTopLayer adds to the top layer if not in it, and moves to the last position if it is in it. This is what the top layer spec says: "to add an element to the top layer, remove it first then add it again".
> > 
> > Moving this check in addToTopLayer would change that behaviour (which made the test fail). It's also worth noting that the new fullscreen API should re-use this top layer concept.
> 
> Understood.
> 
> We could still refactor to avoid double hashing, it’s just a bit more
> complex. We’d need to write a "addToTopLayer if not already present"
> function, and factor things so that it’s only a single hash table lookup.
> One way to do that is an argument that changes the behavior of addToTopLayer.

That sounds reasonable. I filed bug 229149 as followup. I'm unlikely going to take it before I finish the <dialog> work, but feel free to take it.

[Darin Adler]

(In reply to Tim Nguyen (:ntim) from comment #7)
> That sounds reasonable. I filed bug 229149 as followup. I'm unlikely going
> to take it before I finish the <dialog> work, but feel free to take it.

Honestly not sure how important it is, but I might do it.