Skip to content

Commit

Permalink
[Unified Text Replacement] willBeginTextReplacementSession sometime…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
rr-codes committed May 20, 2024
1 parent e6c01f0 commit 90bbdbb
Show file tree
Hide file tree
Showing 6 changed files with 66 additions and 18 deletions.
15 changes: 14 additions & 1 deletion Source/WebCore/editing/TextIterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include "HTMLBodyElement.h"
#include "HTMLElement.h"
#include "HTMLFrameOwnerElement.h"
#include "HTMLImageElement.h"
#include "HTMLInputElement.h"
#include "HTMLLegendElement.h"
#include "HTMLMeterElement.h"
Expand Down Expand Up @@ -350,6 +351,8 @@ static Node* firstNode(const BoundaryPoint& point)
TextIterator::TextIterator(const SimpleRange& range, TextIteratorBehaviors behaviors)
: m_behaviors(behaviors)
{
ASSERT(!m_behaviors.contains(TextIteratorBehavior::EmitsObjectReplacementCharacters) || !m_behaviors.contains(TextIteratorBehavior::EmitsObjectReplacementCharactersForImagesOnly));

range.start.protectedDocument()->updateLayoutIgnorePendingStylesheets();

m_startContainer = range.start.container.ptr();
Expand Down Expand Up @@ -772,7 +775,17 @@ bool TextIterator::handleReplacedElement()

m_hasEmitted = true;

if (m_behaviors.contains(TextIteratorBehavior::EmitsObjectReplacementCharacters)) {
auto shouldEmitObjectReplacementCharacter = [&] {
if (m_behaviors.contains(TextIteratorBehavior::EmitsObjectReplacementCharacters))
return true;

if (m_behaviors.contains(TextIteratorBehavior::EmitsObjectReplacementCharactersForImagesOnly) && is<HTMLImageElement>(m_currentNode.get()))
return true;

return false;
}();

if (shouldEmitObjectReplacementCharacter) {
emitCharacter(objectReplacementCharacter, m_currentNode->protectedParentNode(), protectedCurrentNode(), 0, 1);
// Don't process subtrees for embedded objects. If the text there is required,
// it must be explicitly asked by specifying a range falling inside its boundaries.
Expand Down
2 changes: 2 additions & 0 deletions Source/WebCore/editing/TextIteratorBehavior.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ enum class TextIteratorBehavior : uint16_t {
EntersImageOverlays = 1 << 10,

IgnoresUserSelectNone = 1 << 11,

EmitsObjectReplacementCharactersForImagesOnly = 1 << 12,
};

using TextIteratorBehaviors = OptionSet<TextIteratorBehavior>;
Expand Down
9 changes: 5 additions & 4 deletions Source/WebCore/editing/cocoa/AttributedString.mm
Original file line number Diff line number Diff line change
Expand Up @@ -194,14 +194,15 @@
return attachment;
}, [] (const TextAttachmentFileWrapper& value) -> RetainPtr<id> {
RetainPtr<NSData> data = value.data ? bridge_cast((value.data).get()) : nil;
RetainPtr<NSFileWrapper> fileWrapper = nil;
if (!value.preferredFilename.isNull()) {
fileWrapper = adoptNS([[NSFileWrapper alloc] initRegularFileWithContents:data.get()]);

RetainPtr fileWrapper = adoptNS([[NSFileWrapper alloc] initRegularFileWithContents:data.get()]);
if (!value.preferredFilename.isNull())
[fileWrapper setPreferredFilename:filenameByFixingIllegalCharacters((NSString *)value.preferredFilename)];
}

auto textAttachment = adoptNS([[PlatformNSTextAttachment alloc] initWithFileWrapper:fileWrapper.get()]);
if (!value.accessibilityLabel.isNull())
((NSTextAttachment*)textAttachment.get()).accessibilityLabel = (NSString *)value.accessibilityLabel;

return textAttachment;
#if ENABLE(MULTI_REPRESENTATION_HEIC)
}, [] (const MultiRepresentationHEICAttachmentData& value) -> RetainPtr<id> {
Expand Down
7 changes: 5 additions & 2 deletions Source/WebCore/editing/cocoa/HTMLConverter.mm
Original file line number Diff line number Diff line change
Expand Up @@ -2399,8 +2399,11 @@ AttributedString editingAttributedString(const SimpleRange& range, IncludeImages
for (TextIterator it(range); !it.atEnd(); it.advance()) {
auto node = it.node();

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

auto currentTextLength = it.text().length();
if (!currentTextLength)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,30 @@

namespace WebKit {

// MARK: Static utility helper methods.

WebCore::CharacterRange UnifiedTextReplacementController::characterRange(const WebCore::SimpleRange& scope, const WebCore::SimpleRange& range)
{
return WebCore::characterRange(scope, range, { WebCore::TextIteratorBehavior::EmitsObjectReplacementCharactersForImagesOnly });
}

uint64_t UnifiedTextReplacementController::characterCount(const WebCore::SimpleRange& range)
{
return WebCore::characterCount(range, { WebCore::TextIteratorBehavior::EmitsObjectReplacementCharactersForImagesOnly });
}

WebCore::SimpleRange UnifiedTextReplacementController::resolveCharacterRange(const WebCore::SimpleRange& scope, WebCore::CharacterRange range)
{
return WebCore::resolveCharacterRange(scope, range, { WebCore::TextIteratorBehavior::EmitsObjectReplacementCharactersForImagesOnly });
}

String UnifiedTextReplacementController::plainText(const WebCore::SimpleRange& range)
{
return WebCore::plainText(range, { WebCore::TextIteratorBehavior::EmitsObjectReplacementCharactersForImagesOnly });
}

// MARK: UnifiedTextReplacementController implementation.

UnifiedTextReplacementController::UnifiedTextReplacementController(WebPage& webPage)
: m_webPage(webPage)
{
Expand Down Expand Up @@ -83,11 +107,11 @@
auto selectedTextRange = document->selection().selection().firstRange();

auto attributedStringFromRange = WebCore::editingAttributedString(*contextRange);
auto selectedTextCharacterRange = WebCore::characterRange(*contextRange, *selectedTextRange);
auto selectedTextCharacterRange = UnifiedTextReplacementController::characterRange(*contextRange, *selectedTextRange);

if (session) {
auto attributedStringCharacterCount = attributedStringFromRange.string.length();
auto contextRangeCharacterCount = WebCore::characterCount(*contextRange);
auto contextRangeCharacterCount = UnifiedTextReplacementController::characterCount(*contextRange);

// Postcondition: the selected text character range must be a valid range within the
// attributed string formed by the context range; the length of the entire context range
Expand Down Expand Up @@ -140,12 +164,12 @@

auto locationWithOffset = replacementData.originalRange.location + additionalOffset;

auto resolvedRange = resolveCharacterRange(*sessionRange, { locationWithOffset, replacementData.originalRange.length });
auto resolvedRange = UnifiedTextReplacementController::resolveCharacterRange(*sessionRange, { locationWithOffset, replacementData.originalRange.length });

replaceContentsOfRangeInSession(session, resolvedRange, replacementData.replacement);

auto newRangeWithOffset = WebCore::CharacterRange { locationWithOffset, replacementData.replacement.length() };
auto newResolvedRange = resolveCharacterRange(*sessionRange, newRangeWithOffset);
auto newResolvedRange = UnifiedTextReplacementController::resolveCharacterRange(*sessionRange, newRangeWithOffset);

auto originalString = [context.attributedText.nsAttributedString() attributedSubstringFromRange:replacementData.originalRange];

Expand Down Expand Up @@ -329,7 +353,7 @@

document->selection().clear();

auto sessionRangeCharacterCount = WebCore::characterCount(*sessionRange);
auto sessionRangeCharacterCount = UnifiedTextReplacementController::characterCount(*sessionRange);

if (UNLIKELY(range.length + sessionRangeCharacterCount < contextTextCharacterCount)) {
RELEASE_LOG_ERROR(UnifiedTextReplacement, "UnifiedTextReplacementController::textReplacementSessionDidReceiveTextWithReplacementRange (%s) => the range offset by the character count delta must have a non-negative size (context range length: %u, range.length %llu, session length: %llu)", session.uuid.toString().utf8().data(), contextTextCharacterCount, range.length, sessionRangeCharacterCount);
Expand Down Expand Up @@ -386,7 +410,7 @@
ASSERT(!characterRanges->isEmpty());

auto replaceCharacterRange = subCharacterRange(range, characterRanges->last().range);
return WebCore::resolveCharacterRange(*sessionRange, replaceCharacterRange);
return UnifiedTextReplacementController::resolveCharacterRange(*sessionRange, replaceCharacterRange);
}();

#if PLATFORM(MAC)
Expand All @@ -401,7 +425,7 @@
return;
}

auto replacedRangeAfterReplace = WebCore::resolveCharacterRange(*sessionRangeAfterReplace, characterRangeWithDelta);
auto replacedRangeAfterReplace = UnifiedTextReplacementController::resolveCharacterRange(*sessionRangeAfterReplace, characterRangeWithDelta);

auto finalTextIndicatorUUID = createTextIndicator(replacedRangeAfterReplace, TextIndicatorStyle::Final);

Expand Down Expand Up @@ -464,7 +488,7 @@
markers.forEach<WebCore::DocumentMarkerController::IterationDirection::Backwards>(*sessionRange, { WebCore::DocumentMarker::Type::UnifiedTextReplacement }, [&](auto& node, auto& marker) {
auto rangeToReplace = WebCore::makeSimpleRange(node, marker);

auto currentText = WebCore::plainText(rangeToReplace);
auto currentText = UnifiedTextReplacementController::plainText(rangeToReplace);

auto oldData = std::get<WebCore::DocumentMarker::UnifiedTextReplacementData>(marker.data());
auto previousText = oldData.originalText;
Expand Down Expand Up @@ -689,7 +713,7 @@
if (!sessionRange)
continue;

return WebCore::resolveCharacterRange(*sessionRange, characterRanges[index].range);
return UnifiedTextReplacementController::resolveCharacterRange(*sessionRange, characterRanges[index].range);
}

return std::nullopt;
Expand Down Expand Up @@ -810,9 +834,9 @@
return;
}

auto sessionRangeCount = WebCore::characterCount(*sessionRange);
auto sessionRangeCount = UnifiedTextReplacementController::characterCount(*sessionRange);

auto resolvedCharacterRange = WebCore::characterRange(*sessionRange, range);
auto resolvedCharacterRange = UnifiedTextReplacementController::characterRange(*sessionRange, range);
document->selection().setSelection({ range });

replacementOperation(document->editor());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,11 @@ class UnifiedTextReplacementController final {
WebCore::CharacterRange range;
};

static WebCore::CharacterRange characterRange(const WebCore::SimpleRange& scope, const WebCore::SimpleRange&);
static WebCore::SimpleRange resolveCharacterRange(const WebCore::SimpleRange& scope, WebCore::CharacterRange);
static uint64_t characterCount(const WebCore::SimpleRange&);
static String plainText(const WebCore::SimpleRange&);

std::optional<std::tuple<WebCore::Node&, WebCore::DocumentMarker&>> findReplacementMarkerContainingRange(const WebCore::SimpleRange&) const;
std::optional<std::tuple<WebCore::Node&, WebCore::DocumentMarker&>> findReplacementMarkerByUUID(const WebCore::SimpleRange& outerRange, const WTF::UUID& replacementUUID) const;

Expand Down

0 comments on commit 90bbdbb

Please sign in to comment.