[webkit-reviews] review granted: [Bug 234669] Modal containers are incorrectly detected in navigation elements and fixed document elements : [Attachment 447948] Depends on #234652

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 234669: Modal containers are incorrectly detected in navigation elements
and fixed document elements
https://bugs.webkit.org/show_bug.cgi?id=234669

Attachment 447948: Depends on #234652

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 447948
  --> https://bugs.webkit.org/attachment.cgi?id=447948
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.attributeWithoutSynchronizat
ion(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?