[webkit-reviews] review denied: [Bug 230686] Replace Node::isInert() with RenderStyle::effectiveInert() : [Attachment 439263] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Tim Nguyen (:ntim)
<ntim at apple.com>'s request for review:
Bug 230686: Replace Node::isInert() with RenderStyle::effectiveInert()
https://bugs.webkit.org/show_bug.cgi?id=230686

Attachment 439263: Patch

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




--- Comment #4 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 439263
  --> https://bugs.webkit.org/attachment.cgi?id=439263
Patch

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

> Source/WebCore/dom/Element.cpp:3537
> +    // FIXME: Replace inert/modal dialog logic with style->effectiveInert()
after fixing invalidation
> +    auto elementHasInertAttribute = [](Element& element) -> bool {
> +	   return element.document().settings().inertAttributeEnabled() &&
is<HTMLElement>(element) && element.hasAttribute(HTMLNames::inertAttr);
> +    };
> +
> +    if (elementHasInertAttribute(const_cast<Element&>(*this)))
> +	   return false;
> +
> +    Node* activeModalDialog = document().activeModalDialog();
> +    if (activeModalDialog &&
!activeModalDialog->containsIncludingShadowDOM(this))
> +	   return false;

This doesn't look correct. I can look at some point into resolving this
correctly via style.

Meanwhile I think you should leave Node::isInert() equivalent in place (that is
compute inertness separately from DOM) and use that in isFocusable() as before.

Or you could just let whatever focusability-with-inert-test you are trying to
fix regress. It is probably not critical at this point.