Bug 157611 – indexForVisiblePosition should use the root editable element as the scope

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

We should be using the highest editing host as the scope in indexForVisiblePosition when it's called inside an editable region.
This is blocking the work to make VoiceOver work with undo/redo after the rewrite in the bug 153361.

[Radar WebKit Bug Importer]

<rdar://problem/26239047>

[Ryosuke Niwa]

Created attachment 278700 [details]
Cleanup

[Ryosuke Niwa]

Created attachment 278701 [details]
Removed redundant assertion

[Darin Adler]

Comment on attachment 278701 [details]
Removed redundant assertion

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

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:100
> +        // Workaround the fact indexForVisiblePosition can return a larger index due to TextIterator

The verb is two word: "work around". The noun is "workaround". Here we are using it as a verb.

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:102
> +        // FIXME: Add a new TextIterator beavhior to suppress this behavior.

Spelling error here in the first "behavior".

I find the two uses of the word "behavior" here confusing, so I would use the identifier TextIteratorBehavior to be clearer.

> Source/WebCore/editing/htmlediting.cpp:1137
> +    Document& document = *position.document();

auto& perhaps

> Source/WebCore/editing/htmlediting.cpp:1141
> +        scope = downcast<ContainerNode>(editableRoot);

If this is a safe cast, then why does highestEditableRoot return a Node* and not a ContanerNode*?

> Source/WebCore/editing/htmlediting.cpp:1149
> +    RefPtr<Range> range = Range::create(document, firstPositionInNode(scope.get()), position.parentAnchoredEquivalent());

Should use auto here instead of RefPtr<Range>. I believe this will make range be a Ref instead of a RefPtr.

[Ryosuke Niwa]

Comment on attachment 278701 [details]
Removed redundant assertion

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

Thanks for the review

>> Source/WebCore/editing/htmlediting.cpp:1141
>> +        scope = downcast<ContainerNode>(editableRoot);
> 
> If this is a safe cast, then why does highestEditableRoot return a Node* and not a ContanerNode*?

We should probably change highestEditableRoot to return a ContainerNode* later.

[Ryosuke Niwa]

Committed r200787: <http://trac.webkit.org/changeset/200787>