Bug 215714 – Range::contains does not work correctly when the common ancestor node is a Document.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andres Gonzalez]

Range::contains(const Range&) should compare the Documents to which the Ranges belong, not the ownerDocuments.

[Andres Gonzalez]

Created attachment 406971 [details]
Patch

[Darin Adler]

Comment on attachment 406971 [details]
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.

[Andres Gonzalez]

Created attachment 406985 [details]
Patch

[Andres Gonzalez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 406971 [details]
> 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

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

Fixed. This is happening in every message body in the Mail app with VoiceOver.
> 
> > 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.

Reverted.

[Darin Adler]

(In reply to Andres Gonzalez from comment #4)
> This is happening in every message body in the Mail app with
> VoiceOver.

Yes, but I think that’s because of some accessibility code that currently happens to use a range that covers the whole document but could instead use a range that covers the contents of the body element. The hierarchy is: document -> document element -> body element -> content and it's likely all sorts of code is arbitrarily making different choices of how to make an "everything" range.

[EWS]

Committed r266004: <https://trac.webkit.org/changeset/266004>

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

[Radar WebKit Bug Importer]

<rdar://problem/67547934>