Bug 227800 - Add topLayerElements() and activeModalDialog() to Document
Summary: Add topLayerElements() and activeModalDialog() to Document
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Nguyen (:ntim)
URL:
Keywords: InRadar
Depends on:
Blocks: dialog-element 84796 110952 227534 227839
  Show dependency treegraph
 
Reported: 2021-07-08 10:02 PDT by Tim Nguyen (:ntim)
Modified: 2021-07-09 06:57 PDT (History)
10 users (show)

See Also:


Attachments
Patch (5.70 KB, patch)
2021-07-09 02:46 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (5.65 KB, patch)
2021-07-09 04:46 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (5.69 KB, patch)
2021-07-09 04:57 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (5.67 KB, patch)
2021-07-09 05:01 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>