Bug 227800

Summary: Add topLayerElements() and activeModalDialog() to Document
Product: WebKit Reporter: Tim Nguyen (:ntim) <ntim>
Component: DOMAssignee: Tim Nguyen (:ntim) <ntim>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, cmarcelo, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, koivisto, ntim, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 84635, 84796, 110952, 227534, 227839    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Tim Nguyen (:ntim) 2021-07-08 10:02:50 PDT
Just add the DOM bits, so they can be used for bug 227534 and bug 110952, before the rendering bits get implemented.
Comment 1 Radar WebKit Bug Importer 2021-07-08 10:07:13 PDT
<rdar://problem/80330046>
Comment 2 Tim Nguyen (:ntim) 2021-07-09 02:46:59 PDT
Created attachment 433206 [details]
Patch
Comment 3 Antti Koivisto 2021-07-09 03:44:54 PDT
Comment on attachment 433206 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:8419
> +void Document::removeFromTopLayer(Element* element)
> +{
> +    size_t position = m_topLayerElements.find(element);
> +    if (position != notFound)
> +        m_topLayerElements.remove(position);
> +
> +    element->invalidateStyle();
> +}

With ListHasSet you can just do m_topLayerElements.remove(element).

> Source/WebCore/dom/Document.cpp:8426
> +    for (auto it = m_topLayerElements.rbegin(); it != m_topLayerElements.rend(); ++it) {
> +        if (is<HTMLDialogElement>(*it))
> +            return downcast<HTMLDialogElement>((*it).get());
> +    }

for (auto& element : m_topLayerElement) {
    ...
}

> Source/WebCore/dom/Document.h:1506
> +    void addToTopLayer(Element*);
> +    void removeFromTopLayer(Element*);

These shouldn't be null so please use Element&.

> Source/WebCore/dom/Document.h:2198
> +    Vector<RefPtr<Element>> m_topLayerElements;

You might want ListHashSet for O(1) removals and membership testing (though a reasonable case shouldn't have many of these).

> Source/WebCore/html/HTMLDialogElement.cpp:126
> +            if (m_isModal)
> +                document().removeFromTopLayer(this);
> +
>              m_isModal = false;

could put m_isModal = false inside the branch as well.
Comment 4 Antti Koivisto 2021-07-09 03:50:29 PDT
> for (auto& element : m_topLayerElement) {
>     ...
> }

that would be

for (auto& element : WTF::makeReversedRange(m_topLayerElement)) {
    ...
}
Comment 5 Tim Nguyen (:ntim) 2021-07-09 04:46:48 PDT
Created attachment 433208 [details]
Patch
Comment 6 Antti Koivisto 2021-07-09 04:52:04 PDT
Comment on attachment 433208 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:8416
> +void Document::removeFromTopLayer(Element& element)
> +{
> +    m_topLayerElements.remove(&element);
> +
> +    element.invalidateStyle();
> +}

You shouldn't invalidate style here unless the element was actually found from m_topLayerElements (this is being called unconditionally from removedFromAncestor).
Comment 7 Tim Nguyen (:ntim) 2021-07-09 04:57:48 PDT
Created attachment 433209 [details]
Patch
Comment 8 Tim Nguyen (:ntim) 2021-07-09 05:01:34 PDT
Created attachment 433210 [details]
Patch
Comment 9 Tim Nguyen (:ntim) 2021-07-09 05:05:34 PDT
Committed r279780 (239547@main): <https://commits.webkit.org/239547@main>