Bug 234669 - Modal containers are incorrectly detected in navigation elements and fixed document elements
Summary: Modal containers are incorrectly detected in navigation elements and fixed do...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on: 234652
Blocks:
  Show dependency treegraph
 
Reported: 2021-12-24 12:19 PST by Wenson Hsieh
Modified: 2022-01-02 15:06 PST (History)
6 users (show)

See Also:


Attachments
Depends on #234652 (11.03 KB, patch)
2021-12-24 15:06 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Replace revealModalContainer calls with RAII helper (15.71 KB, patch)
2022-01-01 13:14 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Use WTF_MAKE_NONCOPYABLE (15.88 KB, patch)
2022-01-01 14:03 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-12-24 12:19:38 PST
.
Comment 1 Wenson Hsieh 2021-12-24 15:06:51 PST
Created attachment 447948 [details]
Depends on #234652
Comment 2 Radar WebKit Bug Importer 2021-12-31 12:20:27 PST
<rdar://problem/87030613>
Comment 3 Darin Adler 2021-12-31 21:35:49 PST
Comment on attachment 447948 [details]
Depends on #234652

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

> Source/WebCore/page/ModalContainerObserver.cpp:136
> +static AccessibilityRole accessibilityRole(const HTMLElement& element)
> +{
> +    return AccessibilityObject::ariaRoleToWebCoreRole(element.attributeWithoutSynchronization(HTMLNames::roleAttr));
> +}

Not thrilled with the layering here. If the concept of a navigation element is needed outside the accessibility code, ideally we’d put the parsing of the role attribute in the core DOM implementation and have accessibility use that, rather than calling up to the accessibility code from the core DOM.

> Source/WebCore/page/ModalContainerObserver.cpp:316
> +            observer->revealModalContainer();

Seems unsafe to sprinkle this before 4 different return statements. Is there some more straightforward way to make sure we do this in all the appropriate cases?
Comment 4 Wenson Hsieh 2022-01-01 10:51:32 PST
Comment on attachment 447948 [details]
Depends on #234652

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

Thanks for the review!

>> Source/WebCore/page/ModalContainerObserver.cpp:136
>> +}
> 
> Not thrilled with the layering here. If the concept of a navigation element is needed outside the accessibility code, ideally we’d put the parsing of the role attribute in the core DOM implementation and have accessibility use that, rather than calling up to the accessibility code from the core DOM.

That makes sense. To clarify, are you suggesting that we should move `ariaRoleToWebCoreRole` into a method on `Element`, maybe as just `Element::accessibilityRole() const`, and then use that from both AX (and other parts of WebKit?).

If my understanding is correct, I'll work on that refactoring separately.

>> Source/WebCore/page/ModalContainerObserver.cpp:316
>> +            observer->revealModalContainer();
> 
> Seems unsafe to sprinkle this before 4 different return statements. Is there some more straightforward way to make sure we do this in all the appropriate cases?

It's a little bit tricky, since there are cases where we do want to continue hiding the detected modal container, depending on the client decision (and availability of controls we were able to detect in the "modal container").

However, after giving this some more thought, perhaps a RAII-like helper object might be appropriate here (with a method to explicitly prevent revealing the modal container upon destruction, that would be invoked in only one place). I'll do this refactoring as a part of this patch.
Comment 5 Darin Adler 2022-01-01 11:28:16 PST
Comment on attachment 447948 [details]
Depends on #234652

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

>>> Source/WebCore/page/ModalContainerObserver.cpp:136
>>> +}
>> 
>> Not thrilled with the layering here. If the concept of a navigation element is needed outside the accessibility code, ideally we’d put the parsing of the role attribute in the core DOM implementation and have accessibility use that, rather than calling up to the accessibility code from the core DOM.
> 
> That makes sense. To clarify, are you suggesting that we should move `ariaRoleToWebCoreRole` into a method on `Element`, maybe as just `Element::accessibilityRole() const`, and then use that from both AX (and other parts of WebKit?).
> 
> If my understanding is correct, I'll work on that refactoring separately.

That is my suggestion. Might need a little bit other straightening up to make it a good core DOM concept, but yes. It doesn’t absolutely *have* to be a member function of the Element class -- there may be some value of putting it in a separate header -- but it should not be in the accessibility directory, since the accessibility is generally layered on top of the core DOM.
Comment 6 Wenson Hsieh 2022-01-01 12:25:25 PST
(In reply to Darin Adler from comment #5)
> Comment on attachment 447948 [details]
> Depends on #234652
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447948&action=review
> 
> >>> Source/WebCore/page/ModalContainerObserver.cpp:136
> >>> +}
> >> 
> >> Not thrilled with the layering here. If the concept of a navigation element is needed outside the accessibility code, ideally we’d put the parsing of the role attribute in the core DOM implementation and have accessibility use that, rather than calling up to the accessibility code from the core DOM.
> > 
> > That makes sense. To clarify, are you suggesting that we should move `ariaRoleToWebCoreRole` into a method on `Element`, maybe as just `Element::accessibilityRole() const`, and then use that from both AX (and other parts of WebKit?).
> > 
> > If my understanding is correct, I'll work on that refactoring separately.
> 
> That is my suggestion. Might need a little bit other straightening up to
> make it a good core DOM concept, but yes. It doesn’t absolutely *have* to be
> a member function of the Element class -- there may be some value of putting
> it in a separate header -- but it should not be in the accessibility
> directory, since the accessibility is generally layered on top of the core
> DOM.

Okay! Filed https://bugs.webkit.org/show_bug.cgi?id=234787 for this.
Comment 7 Wenson Hsieh 2022-01-01 13:14:08 PST Comment hidden (obsolete)
Comment 8 Wenson Hsieh 2022-01-01 14:03:35 PST
Created attachment 448163 [details]
Use WTF_MAKE_NONCOPYABLE
Comment 9 EWS 2022-01-01 15:07:33 PST
Committed r287505 (245640@main): <https://commits.webkit.org/245640@main>

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