Bug 215714 - Range::contains does not work correctly when the common ancestor node is a Document.
Summary: Range::contains does not work correctly when the common ancestor node is a Do...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-20 14:14 PDT by Andres Gonzalez
Modified: 2020-08-21 04:43 PDT (History)
13 users (show)

See Also:


Attachments
Patch (10.39 KB, patch)
2020-08-20 14:49 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (9.03 KB, patch)
2020-08-20 17:44 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2020-08-20 14:14:13 PDT
Range::contains(const Range&) should compare the Documents to which the Ranges belong, not the ownerDocuments.
Comment 1 Andres Gonzalez 2020-08-20 14:49:29 PDT
Created attachment 406971 [details]
Patch
Comment 2 Darin Adler 2020-08-20 15:02:26 PDT
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.
Comment 3 Andres Gonzalez 2020-08-20 17:44:29 PDT
Created attachment 406985 [details]
Patch
Comment 4 Andres Gonzalez 2020-08-20 17:51:26 PDT
(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.
Comment 5 Darin Adler 2020-08-20 18:55:15 PDT
(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.
Comment 6 EWS 2020-08-21 04:42:54 PDT
Committed r266004: <https://trac.webkit.org/changeset/266004>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406985 [details].
Comment 7 Radar WebKit Bug Importer 2020-08-21 04:43:14 PDT
<rdar://problem/67547934>