Modal containers are incorrectly detected in navigation elements and fixed document elements

• Component: Platform
• Status: RESOLVED FIXED

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 447948 [details]
Depends on #234652

[Radar WebKit Bug Importer]

<rdar://problem/87030613>

[Darin Adler]

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?

[Wenson Hsieh]

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.

[Darin Adler]

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.

[Wenson Hsieh]

(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.

[Wenson Hsieh]

Created attachment 448161 [details]
Replace revealModalContainer calls with RAII helper

[Wenson Hsieh]

Created attachment 448163 [details]
Use WTF_MAKE_NONCOPYABLE

[EWS]

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].