Bug 237242 - Invoking "Markup Image" should preserve the existing selection range
Summary: Invoking "Markup Image" should preserve the existing selection range
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-26 16:54 PST by Wenson Hsieh
Modified: 2022-02-27 19:26 PST (History)
10 users (show)

See Also:


Attachments
Patch (17.56 KB, patch)
2022-02-27 10:30 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix watchOS build (17.74 KB, patch)
2022-02-27 10:35 PST, Wenson Hsieh
darin: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (18.56 KB, patch)
2022-02-27 15:25 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2022-02-26 16:54:42 PST
.
Comment 1 Wenson Hsieh 2022-02-27 10:30:39 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2022-02-27 10:35:12 PST
Created attachment 453350 [details]
Fix watchOS build
Comment 3 Darin Adler 2022-02-27 11:07:01 PST
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.
Comment 4 Wenson Hsieh 2022-02-27 12:20:07 PST
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.
Comment 5 Wenson Hsieh 2022-02-27 15:25:14 PST
Created attachment 453356 [details]
Patch
Comment 6 EWS 2022-02-27 19:25:33 PST
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].
Comment 7 Radar WebKit Bug Importer 2022-02-27 19:26:28 PST
<rdar://problem/89537723>