[webkit-reviews] review granted: [Bug 215714] Range::contains(const Range&) should compare the Documents to which the Ranges belong, not the ownerDocuments. : [Attachment 406971] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andres Gonzalez
<andresg_22 at apple.com>'s request for review:
Bug 215714: Range::contains(const Range&) should compare the Documents to which
the Ranges belong, not the ownerDocuments.
https://bugs.webkit.org/show_bug.cgi?id=215714

Attachment 406971: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 406971
  --> https://bugs.webkit.org/attachment.cgi?id=406971
Patch

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

> Source/WebCore/ChangeLog:3
> +	   Range::contains(const Range&) should compare the Documents to which
the Ranges belong, not the ownerDocuments.

This bug title is not quite right. The ranges’ ownerDocuments would be fine to
compare and if that’s what the code was doing, we’d be fine. The mistake is to
compare ownerDocuments of the common ancestor *nodes*. The bug title gives the
wrong idea about that.

I would use a title more like this:

     Range::contains does not work correctly when the common ancestor is a
Document

> Source/WebCore/ChangeLog:12
> +	   The problem was that for two Ranges a and b, where a is the document
> +	   object range and b any other range belonging to the same document,
> +	   a.contains(b) would return false. This is counter intuitive and
incorrect

This overstates the case. The bug occurs only when the common ancestor was is
document. If there was any other common ancestor (and there almost always is,
typically the document element) then it worked fine.

This only occurs when ranges have start and end points that are outside the
document element, an unusual pattern that doesn’t come up much. An alternate
fix would be to change the accessibility code to not form ranges that are so
unusual. Might be worth doing that anyway.

> Source/WebCore/accessibility/AccessibilityObject.cpp:647
> -    auto node = this->node();
> -    if (!node)
> -	   return { };
> -    return AXObjectCache::rangeForNodeContents(*node);
> +    if (auto node = this->node())
> +	   return AXObjectCache::rangeForNodeContents(*node);
> +    return { };

Not sure this is an improvement. The early return style is something I
personally like, where the main logic stays on the left and is not nested
inside an if statement, even though that style doesn’t scope the local
variable.