Skip to content

Commit 14e5876

Browse files
alanbaradlayalanbaradlay
committed
Text highlighting range is wrong for truncated RTL text.
https://bugs.webkit.org/show_bug.cgi?id=304475 Reviewed by Antti Koivisto. Expand on 263418@main by moving truncation handling over to TextBoxSelectableRange to ensure all callers get correct start/end positions when RTL content is truncated. (imported/w3c/web-platform-tests/css/css-ui/text-overflow-028.html now passes the selection test too not just the painting one. -263418@main only fixed painting) Tests: imported/w3c/web-platform-tests/css/css-ui/text-overflow-with-selection-ref.html imported/w3c/web-platform-tests/css/css-ui/text-overflow-with-selection.html * LayoutTests/imported/w3c/web-platform-tests/css/css-ui/text-overflow-with-selection-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-ui/text-overflow-with-selection-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-ui/text-overflow-with-selection.html: Added. * Source/WebCore/layout/integration/inline/InlineIteratorBoxModernPath.h: (WebCore::InlineIterator::BoxModernPath::writingMode const): (WebCore::InlineIterator::BoxModernPath::selectableRange const): * Source/WebCore/layout/integration/inline/InlineIteratorBoxModernPathInlines.h: (WebCore::InlineIterator::BoxModernPath::writingMode const): Deleted. * Source/WebCore/rendering/TextBoxPainter.cpp: (WebCore::TextBoxPainter::paintForegroundAndDecorations): * Source/WebCore/rendering/TextBoxSelectableRange.h: (WebCore::TextBoxSelectableRange::clamp const): Canonical link: https://commits.webkit.org/304821@main
1 parent fe7e0f5 commit 14e5876

File tree

7 files changed

+64
-14
lines changed

7 files changed

+64
-14
lines changed

‎LayoutTests/imported/w3c/web-platform-tests/css/css-ui/text-overflow-with-selection-expected.html‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<!DOCTYPE html>
2+
<meta charset="utf-8">
3+
<style>
4+
div { font-family: monospace; }
5+
</style>
6+
7+
<p>The test passes if the following text is visible and fully selected below: …56 FEDCBA</p>
8+
<div id=container>…56 FEDCBA</div>
9+
<script>
10+
let range = new Range();
11+
range.setStart(container.firstChild, 1);
12+
range.setEnd(container.firstChild, 10);
13+
document.getSelection().addRange(range);
14+
</script>

‎LayoutTests/imported/w3c/web-platform-tests/css/css-ui/text-overflow-with-selection-ref.html‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<!DOCTYPE html>
2+
<meta charset="utf-8">
3+
<style>
4+
div { font-family: monospace; }
5+
</style>
6+
7+
<p>The test passes if the following text is visible and fully selected below: …56 FEDCBA</p>
8+
<div id=container>…56 FEDCBA</div>
9+
<script>
10+
let range = new Range();
11+
range.setStart(container.firstChild, 1);
12+
range.setEnd(container.firstChild, 10);
13+
document.getSelection().addRange(range);
14+
</script>

‎LayoutTests/imported/w3c/web-platform-tests/css/css-ui/text-overflow-with-selection.html‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<!DOCTYPE html>
2+
<meta charset="utf-8">
3+
<title>CSS Basic User Interface Test: text-overflow applies visually to bidi and selectable.</title>
4+
<link rel="match" href="reference/text-overflow-with-selection-ref.html">
5+
<meta name="fuzzy" content="maxDifference=0-128; totalPixels=0-58" />
6+
<style>
7+
div {
8+
font-family: monospace;
9+
width: 10ch;
10+
overflow: hidden;
11+
text-overflow: ellipsis;
12+
white-space: pre;
13+
}
14+
</style>
15+
16+
<p>The test passes if the following text is visible and fully selected below: …56 FEDCBA</p>
17+
<div id=container dir=rtl><bdo dir=rtl>ABCDEF</bdo> 123456</div>
18+
<script>
19+
let range = new Range();
20+
range.setStart(container, 0);
21+
range.setEnd(container, 2);
22+
document.getSelection().addRange(range);
23+
</script>

‎Source/WebCore/layout/integration/inline/InlineIteratorBoxModernPath.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include <WebCore/InlineIteratorBoxLegacyPath.h>
3030
#include <WebCore/LayoutElementBox.h>
3131
#include <WebCore/LayoutIntegrationInlineContent.h>
32+
#include <WebCore/RenderBlockFlow.h>
3233
#include <WebCore/TextBoxSelectableRange.h>
3334

3435
namespace WebCore {
@@ -57,7 +58,7 @@ class BoxModernPath {
5758
FloatRect visualRectIgnoringBlockDirection() const { return box().visualRectIgnoringBlockDirection(); }
5859

5960
inline bool isHorizontal() const;
60-
inline WritingMode writingMode() const;
61+
WritingMode writingMode() const { return box().writingMode(); }
6162
bool isLineBreak() const { return box().isLineBreak(); }
6263

6364
unsigned minimumCaretOffset() const { return isText() ? start() : 0; }
@@ -90,7 +91,8 @@ class BoxModernPath {
9091
length(),
9192
extraTrailingLength(),
9293
box.isLineBreak(),
93-
textContent.partiallyVisibleContentLength()
94+
textContent.partiallyVisibleContentLength(),
95+
formattingContextRoot().writingMode().bidiDirection() != direction()
9496
};
9597
}
9698

‎Source/WebCore/layout/integration/inline/InlineIteratorBoxModernPathInlines.h‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,6 @@ namespace InlineIterator {
3434

3535
inline bool BoxModernPath::isHorizontal() const { return box().isHorizontal(); }
3636

37-
inline WritingMode BoxModernPath::writingMode() const { return box().writingMode(); }
38-
3937
inline TextRun BoxModernPath::textRun(TextRunMode mode) const
4038
{
4139
auto& style = box().style();

‎Source/WebCore/rendering/TextBoxPainter.cpp‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -368,8 +368,6 @@ void TextBoxPainter::paintForegroundAndDecorations()
368368
auto shouldPaintSelectionForeground = m_haveSelection && !m_compositionWithCustomUnderlines;
369369
auto hasTextDecoration = !m_style->textDecorationLineInEffect().isNone();
370370
auto hasHighlightDecoration = m_document->hasHighlight() && !MarkedText::collectForHighlights(m_renderer, m_selectableRange, MarkedText::PaintPhase::Decoration).isEmpty();
371-
auto hasMismatchingContentDirection = m_renderer->containingBlock()->writingMode().bidiDirection() != textBox().direction();
372-
auto hasBackwardTrunctation = m_selectableRange.truncation && hasMismatchingContentDirection;
373371

374372
auto hasSpellingOrGrammarDecoration = [&] {
375373
auto markedTexts = MarkedText::collectForDocumentMarkers(m_renderer, m_selectableRange, MarkedText::PaintPhase::Decoration);
@@ -410,14 +408,11 @@ void TextBoxPainter::paintForegroundAndDecorations()
410408
return true;
411409
return false;
412410
};
413-
auto startPosition = [&] {
414-
return !hasBackwardTrunctation ? m_selectableRange.clamp(textBox().start()) : textBox().length() - *m_selectableRange.truncation;
415-
};
416-
auto endPosition = [&] {
417-
return !hasBackwardTrunctation ? m_selectableRange.clamp(textBox().end()) : textBox().length();
418-
};
411+
auto startPosition = m_selectableRange.clamp(textBox().start());
412+
auto endPosition = m_selectableRange.clamp(textBox().end());
413+
419414
if (!contentMayNeedStyledMarkedText()) {
420-
auto markedText = MarkedText { startPosition(), endPosition(), MarkedText::Type::Unmarked };
415+
auto markedText = MarkedText { startPosition, endPosition, MarkedText::Type::Unmarked };
421416
auto styledMarkedText = StyledMarkedText { markedText, StyledMarkedText::computeStyleForUnmarkedMarkedText(m_renderer, m_style, m_isFirstLine, m_paintInfo) };
422417
paintCompositionForeground(styledMarkedText);
423418
return;
@@ -426,7 +421,7 @@ void TextBoxPainter::paintForegroundAndDecorations()
426421
Vector<MarkedText> markedTexts;
427422
if (m_paintInfo.phase != PaintPhase::Selection) {
428423
// The marked texts for the gaps between document markers and selection are implicitly created by subdividing the entire line.
429-
markedTexts.append({ startPosition(), endPosition(), MarkedText::Type::Unmarked });
424+
markedTexts.append({ startPosition, endPosition, MarkedText::Type::Unmarked });
430425

431426
if (!m_isPrinting) {
432427
markedTexts.appendVector(MarkedText::collectForDocumentMarkers(m_renderer, m_selectableRange, MarkedText::PaintPhase::Foreground));

‎Source/WebCore/rendering/TextBoxSelectableRange.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,15 @@ struct TextBoxSelectableRange {
3535
const bool isLineBreak { false };
3636
// FIXME: Consider holding onto the truncation position instead. See webkit.org/b/164999
3737
const std::optional<unsigned> truncation { };
38+
const bool hasMismatchingContentDirection { false }; // where containing block's bidi direction != text box's direction.
3839

3940
unsigned clamp(unsigned offset) const
4041
{
4142
auto clampedOffset = std::clamp(offset, start, start + length) - start;
4243

44+
if (hasMismatchingContentDirection && truncation && *truncation)
45+
return std::max<unsigned>(clampedOffset, length - *truncation);
46+
4347
// FIXME: For some reason we allow the caret move to (invisible) fully truncated text. The zero test is to keep that behavior.
4448
if (truncation && *truncation)
4549
return std::min<unsigned>(clampedOffset, *truncation);

0 commit comments

Comments
 (0)