Invoking "Markup Image" should preserve the existing selection range

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 453349 [details]
Patch

[Wenson Hsieh]

Created attachment 453350 [details]
Fix watchOS build

[Darin Adler]

Comment on attachment 453350 [details]
Fix watchOS build

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

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:494
> +    RefPtr<Element> selectionHost = originalSelection.rootEditableElement() ?: document->body();

Should be able to just write RefPtr here, not RefPtr<Element>.

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:518
> +        auto newSelectionRange = resolveCharacterRange(makeRangeSelectingNodeContents(*selectionHost), *rangeToRestore, iteratorOptions);

Seems like this will do the wrong thing if the replacement doesn’t have the same number of characters as the selection beforehand. We may need some other form of selection preservation to handle interesting cases well.

[Wenson Hsieh]

Comment on attachment 453350 [details]
Fix watchOS build

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

Thanks for the review!

>> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:494
>> +    RefPtr<Element> selectionHost = originalSelection.rootEditableElement() ?: document->body();
> 
> Should be able to just write RefPtr here, not RefPtr<Element>.

Fixed!

>> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:518
>> +        auto newSelectionRange = resolveCharacterRange(makeRangeSelectingNodeContents(*selectionHost), *rangeToRestore, iteratorOptions);
> 
> Seems like this will do the wrong thing if the replacement doesn’t have the same number of characters as the selection beforehand. We may need some other form of selection preservation to handle interesting cases well.

Ah, that's a good point!

For the time being, this should only ever be invoked in circumstances where we're swapping one image element with another (which should preserve the character count), but if this codepath is ever used to replace a node with text, this restoration logic won't work at all.

I'll add an early return for now to avoid trying to restore the selection if the number of visible characters underneath the editable root has changed, with a FIXME for smarter handling around this in the future, should we need it.

[Wenson Hsieh]

Created attachment 453356 [details]
Patch

[EWS]

Committed r290578 (247856@main): <https://commits.webkit.org/247856@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/89537723>