[Unified Text Replacement] `willBeginTextReplacementSession` sometimes does not return a context if the text lengths differ #28722

rr-codes · 2024-05-17T18:57:35Z

`90bbdbb`

[Unified Text Replacement] `willBeginTextReplacementSession` sometimes does not return a context if the text lengths differ
https://bugs.webkit.org/show_bug.cgi?id=274325
rdar://128285414

Reviewed by Aditya Keerthi.

The length of the string produced by `editingAttributedString` and the character count produced by
`TextIterator` can sometimes differ. This can happen when there are images in the HTML, as `editingAttributedString`
handles them in a special case.

Fix by adding a new `TextIteratorBehavior` option that acts the same as `EmitsObjectReplacementCharacters`,
but only for images, and then use this behavior anywhere in UnifiedTextReplacementController that
needs character ranges or counts.

Also fix a few issues with attachments in attributed strings:

* When converting an `AttributedString` to an `NSAttributedString` with an attachment, if the attachment
does not have a "preferred" filename, an empty file wrapper will be created. Fix by always creating a file
wrapper.

* When converting HTML with an image to an attributed string, the position of the resulting attachment in
the produced attributed string is sometimes incorrect. This is because the length of the attachment itself
is never taken into account, and so consequent parts of the string will be inserted in too early of a position.
Fix by accounting for the length of the attachment.

* Source/WebCore/editing/TextIterator.cpp:
(WebCore::TextIterator::TextIterator):
(WebCore::TextIterator::handleReplacedElement):
* Source/WebCore/editing/TextIteratorBehavior.h:
* Source/WebCore/editing/cocoa/AttributedString.mm:
(WebCore::toNSObject):
* Source/WebCore/editing/cocoa/HTMLConverter.mm:
(WebCore::editingAttributedString):
* Source/WebKit/WebProcess/WebPage/Cocoa/UnifiedTextReplacementController.mm:
(WebKit::UnifiedTextReplacementController::characterRange):
(WebKit::UnifiedTextReplacementController::characterCount):
(WebKit::UnifiedTextReplacementController::resolveCharacterRange):
(WebKit::UnifiedTextReplacementController::plainText):
(WebKit::UnifiedTextReplacementController::willBeginTextReplacementSession):
(WebKit::UnifiedTextReplacementController::textReplacementSessionDidReceiveReplacements):
(WebKit::UnifiedTextReplacementController::textReplacementSessionDidReceiveTextWithReplacementRange):
(WebKit::UnifiedTextReplacementController::textReplacementSessionPerformEditActionForPlainText):
(WebKit::UnifiedTextReplacementController::contextRangeForSessionOrRangeWithUUID const):
(WebKit::UnifiedTextReplacementController::replaceContentsOfRangeInSessionInternal):
* Source/WebKit/WebProcess/WebPage/UnifiedTextReplacementController.h:

Canonical link: https://commits.webkit.org/279002@main

5c4632a

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-05-17T18:57:41Z

EWS run on previous version of this PR (hash 1d99dd6)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	🛠 ios	🛠 mac	🛠 wpe	🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	🧪 wpe-wk2	🧪 wincairo-tests
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	🧪 api-mac	🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt	🧪 mac-wk1	✅ 🛠 wpe-cairo
	🧪 api-ios	🧪 mac-wk2	✅ 🛠 gtk
	~~🛠 tv~~	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 gtk-wk2
	~~🛠 tv-sim~~	🧪 mac-wk2-stress	🧪 api-gtk
	~~🛠 watch~~
	~~🛠 watch-sim~~

Source/WebKit/WebProcess/WebPage/Cocoa/UnifiedTextReplacementController.mm

webkit-early-warning-system · 2024-05-17T20:39:46Z

EWS run on previous version of this PR (hash d747836)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 wincairo~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2	~~🧪 wincairo-tests~~
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	✅ 🛠 wpe-cairo
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🛠 gtk
	~~🛠 tv~~	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 gtk-wk2
	~~🛠 tv-sim~~	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	~~🛠 watch~~
	~~🛠 watch-sim~~

Source/WebCore/editing/TextIterator.cpp

Source/WebCore/editing/TextIteratorBehavior.h

Source/WebKit/WebProcess/WebPage/Cocoa/UnifiedTextReplacementController.mm

Source/WebCore/editing/TextIterator.cpp

webkit-early-warning-system · 2024-05-17T22:07:25Z

EWS run on previous version of this PR (hash 30a4f24)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	❌ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	❌ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	❌ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	❌ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	❌ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-05-18T05:26:49Z

EWS run on previous version of this PR (hash 4729c5e)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

rr-codes · 2024-05-18T05:27:33Z

Source/WebCore/editing/cocoa/AttributedString.mm

            [fileWrapper setPreferredFilename:filenameByFixingIllegalCharacters((NSString *)value.preferredFilename)];
-        }
+
        auto textAttachment = adoptNS([[PlatformNSTextAttachment alloc] initWithFileWrapper:fileWrapper.get()]);


intentionally added newlines for readability

pxlcoder · 2024-05-18T06:14:33Z

Source/WebCore/editing/cocoa/HTMLConverter.mm

+        if (RefPtr imageElement = dynamicDowncast<HTMLImageElement>(node); imageElement && includeImages == IncludeImages::Yes) {
+            RetainPtr attachmentAttributedString = attributedStringWithAttachmentForElement(*imageElement);
+            [string appendAttributedString:attachmentAttributedString.get()];
+            stringLength += [attachmentAttributedString length];


Is this compatible with:

auto currentTextLength = it.text().length();

and

[string setAttributes:attrs.get() range:NSMakeRange(stringLength, currentTextLength)]; below,

given that TextIterator it(range) does not have any additional behaviors specified?

Wouldn't it result in length mismatches?

it wasn't compatible before, but is now. This change doesn't actually affect the string lengths, just changes the position of where the text that comes after the attachment goes.

(My tests also exercise this thoroughly)

webkit-early-warning-system · 2024-05-20T03:45:55Z

EWS run on current version of this PR (hash 5c4632a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim

…s does not return a context if the text lengths differ https://bugs.webkit.org/show_bug.cgi?id=274325 rdar://128285414 Reviewed by Aditya Keerthi. The length of the string produced by `editingAttributedString` and the character count produced by `TextIterator` can sometimes differ. This can happen when there are images in the HTML, as `editingAttributedString` handles them in a special case. Fix by adding a new `TextIteratorBehavior` option that acts the same as `EmitsObjectReplacementCharacters`, but only for images, and then use this behavior anywhere in UnifiedTextReplacementController that needs character ranges or counts. Also fix a few issues with attachments in attributed strings: * When converting an `AttributedString` to an `NSAttributedString` with an attachment, if the attachment does not have a "preferred" filename, an empty file wrapper will be created. Fix by always creating a file wrapper. * When converting HTML with an image to an attributed string, the position of the resulting attachment in the produced attributed string is sometimes incorrect. This is because the length of the attachment itself is never taken into account, and so consequent parts of the string will be inserted in too early of a position. Fix by accounting for the length of the attachment. * Source/WebCore/editing/TextIterator.cpp: (WebCore::TextIterator::TextIterator): (WebCore::TextIterator::handleReplacedElement): * Source/WebCore/editing/TextIteratorBehavior.h: * Source/WebCore/editing/cocoa/AttributedString.mm: (WebCore::toNSObject): * Source/WebCore/editing/cocoa/HTMLConverter.mm: (WebCore::editingAttributedString): * Source/WebKit/WebProcess/WebPage/Cocoa/UnifiedTextReplacementController.mm: (WebKit::UnifiedTextReplacementController::characterRange): (WebKit::UnifiedTextReplacementController::characterCount): (WebKit::UnifiedTextReplacementController::resolveCharacterRange): (WebKit::UnifiedTextReplacementController::plainText): (WebKit::UnifiedTextReplacementController::willBeginTextReplacementSession): (WebKit::UnifiedTextReplacementController::textReplacementSessionDidReceiveReplacements): (WebKit::UnifiedTextReplacementController::textReplacementSessionDidReceiveTextWithReplacementRange): (WebKit::UnifiedTextReplacementController::textReplacementSessionPerformEditActionForPlainText): (WebKit::UnifiedTextReplacementController::contextRangeForSessionOrRangeWithUUID const): (WebKit::UnifiedTextReplacementController::replaceContentsOfRangeInSessionInternal): * Source/WebKit/WebProcess/WebPage/UnifiedTextReplacementController.h: Canonical link: https://commits.webkit.org/279002@main

webkit-commit-queue · 2024-05-20T17:35:10Z

Committed 279002@main (90bbdbb): https://commits.webkit.org/279002@main

Reviewed commits have been landed. Closing PR #28722 and removing active labels.

rr-codes requested review from cdumez and rniwa as code owners May 17, 2024 18:57

rr-codes self-assigned this May 17, 2024

rr-codes added the HTML Editing For bugs in HTML editing support (including designMode and contentEditable). label May 17, 2024

rr-codes commented May 17, 2024

View reviewed changes

Source/WebKit/WebProcess/WebPage/Cocoa/UnifiedTextReplacementController.mm Outdated Show resolved Hide resolved

rr-codes force-pushed the eng/128285414 branch from 1d99dd6 to d747836 Compare May 17, 2024 20:39

rr-codes requested review from whsieh, pxlcoder, aprotyas and megangardner May 17, 2024 20:40

pxlcoder reviewed May 17, 2024

View reviewed changes

Source/WebCore/editing/TextIterator.cpp Outdated Show resolved Hide resolved

pxlcoder reviewed May 17, 2024

View reviewed changes

Source/WebCore/editing/TextIteratorBehavior.h Outdated Show resolved Hide resolved

pxlcoder reviewed May 17, 2024

View reviewed changes

Source/WebKit/WebProcess/WebPage/Cocoa/UnifiedTextReplacementController.mm Show resolved Hide resolved

pxlcoder reviewed May 17, 2024

View reviewed changes

Source/WebCore/editing/TextIterator.cpp Outdated Show resolved Hide resolved

rr-codes force-pushed the eng/128285414 branch from d747836 to 30a4f24 Compare May 17, 2024 22:07

rr-codes requested a review from pxlcoder May 17, 2024 22:07

rr-codes force-pushed the eng/128285414 branch from 30a4f24 to 4729c5e Compare May 18, 2024 05:26

rr-codes commented May 18, 2024

View reviewed changes

pxlcoder reviewed May 18, 2024

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 18, 2024

rr-codes removed the merging-blocked Applied to prevent a change from being merged label May 18, 2024

rr-codes force-pushed the eng/128285414 branch from 4729c5e to 5c4632a Compare May 20, 2024 03:45

pxlcoder approved these changes May 20, 2024

View reviewed changes

rr-codes added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 20, 2024

webkit-commit-queue force-pushed the eng/128285414 branch from 5c4632a to 90bbdbb Compare May 20, 2024 17:35

webkit-commit-queue merged commit 90bbdbb into WebKit:main May 20, 2024

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Unified Text Replacement] `willBeginTextReplacementSession` sometimes does not return a context if the text lengths differ #28722

[Unified Text Replacement] `willBeginTextReplacementSession` sometimes does not return a context if the text lengths differ #28722

rr-codes commented May 17, 2024 •

edited by webkit-early-warning-system

webkit-early-warning-system commented May 17, 2024 •

edited

webkit-early-warning-system commented May 17, 2024 •

edited

webkit-early-warning-system commented May 17, 2024 •

edited

webkit-early-warning-system commented May 18, 2024 •

edited

rr-codes May 18, 2024

pxlcoder May 18, 2024

rr-codes May 18, 2024

rr-codes May 18, 2024

webkit-early-warning-system commented May 20, 2024 •

edited

webkit-commit-queue commented May 20, 2024

[Unified Text Replacement] willBeginTextReplacementSession sometimes does not return a context if the text lengths differ #28722

[Unified Text Replacement] willBeginTextReplacementSession sometimes does not return a context if the text lengths differ #28722

Conversation

rr-codes commented May 17, 2024 • edited by webkit-early-warning-system

90bbdbb

webkit-early-warning-system commented May 17, 2024 • edited

webkit-early-warning-system commented May 17, 2024 • edited

webkit-early-warning-system commented May 17, 2024 • edited

webkit-early-warning-system commented May 18, 2024 • edited

rr-codes May 18, 2024

Choose a reason for hiding this comment

pxlcoder May 18, 2024

Choose a reason for hiding this comment

rr-codes May 18, 2024

Choose a reason for hiding this comment

rr-codes May 18, 2024

Choose a reason for hiding this comment

webkit-early-warning-system commented May 20, 2024 • edited

webkit-commit-queue commented May 20, 2024

[Unified Text Replacement] `willBeginTextReplacementSession` sometimes does not return a context if the text lengths differ #28722

[Unified Text Replacement] `willBeginTextReplacementSession` sometimes does not return a context if the text lengths differ #28722

rr-codes commented May 17, 2024 •

edited by webkit-early-warning-system

`90bbdbb`

webkit-early-warning-system commented May 17, 2024 •

edited

webkit-early-warning-system commented May 17, 2024 •

edited

webkit-early-warning-system commented May 17, 2024 •

edited

webkit-early-warning-system commented May 18, 2024 •

edited

webkit-early-warning-system commented May 20, 2024 •

edited