Changeset 223840 in webkit


Ignore:
Timestamp:
Oct 23, 2017 10:28:18 AM (6 years ago)
Author:
dbates@webkit.org
Message:

Unreviewed, rolling out r223699.

Caused regressions with right-to-left text selection and
painting of markers in flipped writing mode and in overlapping
lines. Will investigate offline.

Reverted changeset:

"Share logic in InlineTextBox to compute selection rect"
https://bugs.webkit.org/show_bug.cgi?id=178232
https://trac.webkit.org/changeset/223699

Location:
trunk/Source/WebCore
Files:
3 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r223839 r223840  
     12017-10-23  Daniel Bates  <dabates@apple.com>
     2
     3        Unreviewed, rolling out r223699.
     4
     5        Caused regressions with right-to-left text selection and
     6        painting of markers in flipped writing mode and in overlapping
     7        lines. Will investigate offline.
     8
     9        Reverted changeset:
     10
     11        "Share logic in InlineTextBox to compute selection rect"
     12        https://bugs.webkit.org/show_bug.cgi?id=178232
     13        https://trac.webkit.org/changeset/223699
     14
    1152017-10-23  Youenn Fablet  <youenn@apple.com>
    216
  • trunk/Source/WebCore/rendering/InlineTextBox.cpp

    r223699 r223840  
    186186}
    187187
    188 // FIXME: This function should return the same rectangle as localSelectionRectWithClampedPositions(..., ..., SelectionRectRounding::None),
    189 // which represents the rectange of the selected text on the page. We need to update all callers to expect this change.
    190 // See <https://bugs.webkit.org/show_bug.cgi?id=138913> for more details.
    191 LayoutRect InlineTextBox::localSelectionRect(unsigned startPosition, unsigned endPosition) const
    192 {
    193     if (startPosition > endPosition || endPosition < m_start || startPosition > end() + 1)
     188// FIXME: Share more code with paintSelection().
     189LayoutRect InlineTextBox::localSelectionRect(unsigned startPos, unsigned endPos) const
     190{
     191    unsigned sPos = clampedOffset(startPos);
     192    unsigned ePos = clampedOffset(endPos);
     193
     194    if (sPos >= ePos && !(startPos == endPos && startPos >= start() && startPos <= (start() + len())))
    194195        return { };
    195     return localSelectionRectWithClampedPositions(clampedOffset(startPosition), clampedOffset(endPosition), SelectionRectRounding::EnclosingIntRect);
    196 }
    197 
    198 LayoutRect InlineTextBox::localSelectionRectWithClampedPositions(unsigned clampedStartPosition, unsigned clampedEndPosition, SelectionRectRounding roundingStrategy) const
    199 {
    200     if (clampedStartPosition > clampedEndPosition)
    201         return { };
    202 
    203     auto selectionTop = root().selectionTopAdjustedForPrecedingBlock();
    204     auto selectionBottom = this->selectionBottom();
     196
     197    LayoutUnit selectionTop = this->selectionTop();
     198    LayoutUnit selectionHeight = this->selectionHeight();
    205199
    206200    auto text = this->text();
    207201    TextRun textRun = createTextRun(text);
    208202
    209     // Use same y positioning and height as used for selected text on the page, so that when some or all of this
    210     // subrange is selected on the page there are no pieces sticking out.
    211     LayoutUnit deltaY = renderer().style().isFlippedLinesWritingMode() ? selectionBottom - logicalBottom() : logicalTop() - selectionTop;
    212     auto selectionHeight = root().selectionHeightAdjustedForPrecedingBlock();
    213     LayoutRect selectionRect { LayoutPoint { logicalLeft(), logicalTop() - deltaY }, LayoutSize { m_logicalWidth, selectionHeight } };
    214 
     203    LayoutRect selectionRect = LayoutRect(LayoutPoint(logicalLeft(), selectionTop), LayoutSize(m_logicalWidth, selectionHeight));
    215204    // Avoid computing the font width when the entire line box is selected as an optimization.
    216     if (clampedStartPosition || clampedEndPosition != textRun.length())
    217         lineFont().adjustSelectionRectForText(textRun, selectionRect, clampedStartPosition, clampedEndPosition);
    218 
    219     LayoutUnit selectionLeft;
    220     LayoutUnit selectionWidth;
    221     if (roundingStrategy == SelectionRectRounding::EnclosingIntRect) {
    222         auto snappedSelectionRect = enclosingIntRect(selectionRect);
    223         selectionLeft = snappedSelectionRect.x();
    224         selectionWidth = snappedSelectionRect.width();
    225         if (snappedSelectionRect.x() > logicalRight())
    226             selectionWidth = 0;
    227         else if (snappedSelectionRect.maxX() > logicalRight())
    228             selectionWidth = logicalRight() - snappedSelectionRect.x();
    229     } else {
    230         selectionLeft = selectionRect.x();
    231         selectionWidth = selectionRect.width();
    232     }
    233 
    234     LayoutPoint location;
    235     LayoutSize size;
    236     if (isHorizontal()) {
    237         location = { selectionLeft, selectionTop };
    238         size = { selectionWidth, selectionHeight };
    239     } else {
    240         location = { selectionTop, selectionLeft };
    241         size = { selectionHeight, selectionWidth };
    242     }
    243 
    244     return { location, size };
     205    if (sPos || ePos != textRun.length())
     206        lineFont().adjustSelectionRectForText(textRun, selectionRect, sPos, ePos);
     207    // FIXME: The computation of the snapped selection rect differs from the computation of this rect
     208    // in paintSelection(). See <https://bugs.webkit.org/show_bug.cgi?id=138913>.
     209    IntRect snappedSelectionRect = enclosingIntRect(selectionRect);
     210    LayoutUnit logicalWidth = snappedSelectionRect.width();
     211    if (snappedSelectionRect.x() > logicalRight())
     212        logicalWidth  = 0;
     213    else if (snappedSelectionRect.maxX() > logicalRight())
     214        logicalWidth = logicalRight() - snappedSelectionRect.x();
     215
     216    LayoutPoint topPoint = isHorizontal() ? LayoutPoint(snappedSelectionRect.x(), selectionTop) : LayoutPoint(selectionTop, snappedSelectionRect.x());
     217    LayoutUnit width = isHorizontal() ? logicalWidth : selectionHeight;
     218    LayoutUnit height = isHorizontal() ? selectionHeight : logicalWidth;
     219
     220    return LayoutRect(topPoint, LayoutSize(width, height));
    245221}
    246222
     
    430406    LayoutUnit paintStart = isHorizontal() ? paintInfo.rect.x() : paintInfo.rect.y();
    431407   
    432     LayoutPoint localPaintOffset { paintOffset };
     408    FloatPoint localPaintOffset(paintOffset);
    433409   
    434410    if (logicalStart >= paintEnd || logicalStart + logicalExtent <= paintStart)
     
    497473    if (paintInfo.phase != PaintPhaseSelection && paintInfo.phase != PaintPhaseTextClip && !isPrinting) {
    498474        if (containsComposition && !useCustomUnderlines)
    499             paintCompositionBackground(context, localPaintOffset);
    500 
    501         paintDocumentMarkers(context, localPaintOffset, true);
     475            paintCompositionBackground(context, boxOrigin);
     476
     477        paintDocumentMarkers(context, boxOrigin, true);
    502478
    503479        if (haveSelection && !useCustomUnderlines)
    504             paintSelection(context, localPaintOffset, selectionPaintStyle.fillColor);
     480            paintSelection(context, boxOrigin, selectionPaintStyle.fillColor);
    505481    }
    506482
     
    604580
    605581    if (paintInfo.phase == PaintPhaseForeground) {
    606         paintDocumentMarkers(context, localPaintOffset, false);
     582        paintDocumentMarkers(context, boxOrigin, false);
    607583
    608584        if (useCustomUnderlines)
     
    648624}
    649625
    650 void InlineTextBox::paintSelection(GraphicsContext& context, const LayoutPoint& paintOffset, const Color& textColor)
     626void InlineTextBox::paintSelection(GraphicsContext& context, const FloatPoint& boxOrigin, const Color& textColor)
    651627{
    652628#if ENABLE(TEXT_SELECTION)
     
    675651    // Note that if the text is truncated, we let the thing being painted in the truncation
    676652    // draw its own highlight.
    677     auto selectionRect = localSelectionRectWithClampedPositions(selectionStart, selectionEnd);
    678     selectionRect.moveBy(paintOffset);
     653    auto text = this->text();
     654    TextRun textRun = createTextRun(text);
     655
     656    const RootInlineBox& rootBox = root();
     657    LayoutUnit selectionBottom = rootBox.selectionBottom();
     658    LayoutUnit selectionTop = rootBox.selectionTopAdjustedForPrecedingBlock();
     659
     660    LayoutUnit deltaY = renderer().style().isFlippedLinesWritingMode() ? selectionBottom - logicalBottom() : logicalTop() - selectionTop;
     661    LayoutUnit selectionHeight = std::max<LayoutUnit>(0, selectionBottom - selectionTop);
     662
     663    LayoutRect selectionRect = LayoutRect(boxOrigin.x(), boxOrigin.y() - deltaY, m_logicalWidth, selectionHeight);
     664    lineFont().adjustSelectionRectForText(textRun, selectionRect, selectionStart, selectionEnd);
    679665
    680666    // FIXME: Support painting combined text.
    681     context.fillRect(snapRectToDevicePixelsWithWritingDirection(selectionRect, renderer().document().deviceScaleFactor(), isLeftToRightDirection()), c);
     667    context.fillRect(snapRectToDevicePixelsWithWritingDirection(selectionRect, renderer().document().deviceScaleFactor(), textRun.ltr()), c);
    682668#else
    683669    UNUSED_PARAM(context);
    684     UNUSED_PARAM(paintOffset);
     670    UNUSED_PARAM(boxOrigin);
    685671    UNUSED_PARAM(textColor);
    686672#endif
    687673}
    688674
    689 inline void InlineTextBox::paintTextSubrangeBackground(GraphicsContext& context, const LayoutPoint& paintOffset, const Color& color, unsigned startOffset, unsigned endOffset)
     675inline void InlineTextBox::paintTextSubrangeBackground(GraphicsContext& context, const FloatPoint& boxOrigin, const Color& color, unsigned startOffset, unsigned endOffset)
    690676{
    691677    startOffset = clampedOffset(startOffset);
     
    697683    updateGraphicsContext(context, TextPaintStyle { color }); // Don't draw text at all!
    698684
    699     auto selectionRect = localSelectionRectWithClampedPositions(startOffset, endOffset);
    700     selectionRect.moveBy(paintOffset);
     685    // Use same y positioning and height as for selection, so that when the selection and this subrange are on
     686    // the same word there are no pieces sticking out.
     687    LayoutUnit deltaY = renderer().style().isFlippedLinesWritingMode() ? selectionBottom() - logicalBottom() : logicalTop() - selectionTop();
     688    LayoutRect selectionRect = LayoutRect(boxOrigin.x(), boxOrigin.y() - deltaY, 0, selectionHeight());
     689
     690    auto text = this->text();
     691    TextRun textRun = createTextRun(text);
     692    lineFont().adjustSelectionRectForText(textRun, selectionRect, startOffset, endOffset);
    701693
    702694    // FIXME: Support painting combined text.
    703     context.fillRect(snapRectToDevicePixelsWithWritingDirection(selectionRect, renderer().document().deviceScaleFactor(), isLeftToRightDirection()), color);
    704 }
    705 
    706 void InlineTextBox::paintCompositionBackground(GraphicsContext& context, const LayoutPoint& paintOffset)
    707 {
    708     paintTextSubrangeBackground(context, paintOffset, renderer().frame().editor().compositionStart(), renderer().frame().editor().compositionEnd(), Color::compositionFill);
    709 }
    710 
    711 void InlineTextBox::paintTextMatchMarker(GraphicsContext& context, const LayoutPoint& paintOffset, const MarkerSubrange& subrange, bool isActiveMatch)
     695    context.fillRect(snapRectToDevicePixelsWithWritingDirection(selectionRect, renderer().document().deviceScaleFactor(), textRun.ltr()), color);
     696}
     697
     698void InlineTextBox::paintCompositionBackground(GraphicsContext& context, const FloatPoint& boxOrigin)
     699{
     700    paintTextSubrangeBackground(context, boxOrigin, renderer().frame().editor().compositionStart(), renderer().frame().editor().compositionEnd(), Color::compositionFill);
     701}
     702
     703void InlineTextBox::paintTextMatchMarker(GraphicsContext& context, const FloatPoint& boxOrigin, const MarkerSubrange& subrange, bool isActiveMatch)
    712704{
    713705    if (!renderer().frame().editor().markedTextMatchesAreHighlighted())
    714706        return;
    715707    auto highlightColor = isActiveMatch ? renderer().theme().platformActiveTextSearchHighlightColor() : renderer().theme().platformInactiveTextSearchHighlightColor();
    716     paintTextSubrangeBackground(context, paintOffset, highlightColor, subrange.startOffset, subrange.endOffset);
     708    paintTextSubrangeBackground(context, boxOrigin, highlightColor, subrange.startOffset, subrange.endOffset);
    717709}
    718710
     
    767759}
    768760
    769 void InlineTextBox::paintDocumentMarker(GraphicsContext& context, const LayoutPoint& paintOffset, const MarkerSubrange& subrange)
     761void InlineTextBox::paintDocumentMarker(GraphicsContext& context, const FloatPoint& boxOrigin, const MarkerSubrange& subrange)
    770762{
    771763    // Never print spelling/grammar markers (5327887)
     
    775767    if (m_truncation == cFullTruncation)
    776768        return;
     769
     770    float start = 0; // start of line to draw, relative to tx
     771    float width = m_logicalWidth; // how much line to draw
    777772
    778773    // Determine whether we need to measure text
     
    785780        markerSpansWholeBox = false;
    786781
    787     FloatPoint location;
    788     float width;
    789     if (markerSpansWholeBox) {
    790         location = locationIncludingFlipping();
    791         width = m_logicalWidth;
    792     } else {
     782    if (!markerSpansWholeBox) {
    793783        unsigned startPosition = clampedOffset(subrange.startOffset);
    794784        unsigned endPosition = clampedOffset(subrange.endOffset);
    795785
    796         // We round to the nearest enclosing integral rect to avoid truncating the dot decoration on Cocoa platforms.
    797         // FIXME: Find a better place to ensure that the Cocoa dots are not truncated.
    798         auto selectionRect = localSelectionRectWithClampedPositions(startPosition, endPosition, SelectionRectRounding::EnclosingIntRect);
    799         location = selectionRect.location();
    800         width = selectionRect.width();
    801     }
    802     location.moveBy(paintOffset);
    803 
     786        // Calculate start & width
     787        int deltaY = renderer().style().isFlippedLinesWritingMode() ? selectionBottom() - logicalBottom() : logicalTop() - selectionTop();
     788        int selHeight = selectionHeight();
     789        FloatPoint startPoint(boxOrigin.x(), boxOrigin.y() - deltaY);
     790        auto text = this->text();
     791        TextRun run = createTextRun(text);
     792
     793        LayoutRect selectionRect = LayoutRect(startPoint, FloatSize(0, selHeight));
     794        lineFont().adjustSelectionRectForText(run, selectionRect, startPosition, endPosition);
     795        IntRect markerRect = enclosingIntRect(selectionRect);
     796        start = markerRect.x() - startPoint.x();
     797        width = markerRect.width();
     798    }
     799   
    804800    auto lineStyleForSubrangeType = [] (MarkerSubrange::Type type) {
    805801        switch (type) {
     
    841837    }
    842838    // FIXME: Support painting combined text.
    843     location.move(0, underlineOffset);
    844     context.drawLineForDocumentMarker(location, width, lineStyleForSubrangeType(subrange.type));
    845 }
    846 
    847 void InlineTextBox::paintDocumentMarkers(GraphicsContext& context, const LayoutPoint& paintOffset, bool background)
     839    context.drawLineForDocumentMarker(FloatPoint(boxOrigin.x() + start, boxOrigin.y() + underlineOffset), width, lineStyleForSubrangeType(subrange.type));
     840}
     841
     842void InlineTextBox::paintDocumentMarkers(GraphicsContext& context, const FloatPoint& boxOrigin, bool background)
    848843{
    849844    if (!renderer().textNode())
     
    939934    for (auto& subrange : subdivide(subranges, OverlapStrategy::Frontmost)) {
    940935        if (subrange.type == MarkerSubrange::TextMatch)
    941             paintTextMatchMarker(context, paintOffset, subrange, subrange.marker->isActiveMatch());
     936            paintTextMatchMarker(context, boxOrigin, subrange, subrange.marker->isActiveMatch());
    942937        else
    943             paintDocumentMarker(context, paintOffset, subrange);
     938            paintDocumentMarker(context, boxOrigin, subrange);
    944939    }
    945940}
  • trunk/Source/WebCore/rendering/InlineTextBox.h

    r223699 r223840  
    111111    FloatRect calculateBoundaries() const override { return FloatRect(x(), y(), width(), height()); }
    112112
    113     virtual LayoutRect localSelectionRect(unsigned startPosition, unsigned endPosition) const;
     113    virtual LayoutRect localSelectionRect(unsigned startPos, unsigned endPos) const;
    114114    bool isSelected(unsigned startPosition, unsigned endPosition) const;
    115115    std::pair<unsigned, unsigned> selectionStartEnd() const;
     
    153153    void paintDecoration(GraphicsContext&, const TextRun&, const FloatPoint& textOrigin, const FloatRect& boxRect,
    154154        TextDecoration, TextPaintStyle, const ShadowData*, const FloatRect& clipOutRect);
    155     void paintSelection(GraphicsContext&, const LayoutPoint& paintOffset, const Color&);
     155    void paintSelection(GraphicsContext&, const FloatPoint& boxOrigin, const Color&);
    156156
    157     void paintDocumentMarker(GraphicsContext&, const LayoutPoint& paintOffset, const MarkerSubrange&);
    158     void paintDocumentMarkers(GraphicsContext&, const LayoutPoint& paintOffset, bool background);
    159     void paintTextMatchMarker(GraphicsContext&, const LayoutPoint& paintOffset, const MarkerSubrange&, bool isActiveMatch);
     157    void paintDocumentMarker(GraphicsContext&, const FloatPoint& boxOrigin, const MarkerSubrange&);
     158    void paintDocumentMarkers(GraphicsContext&, const FloatPoint& boxOrigin, bool background);
     159    void paintTextMatchMarker(GraphicsContext&, const FloatPoint& boxOrigin, const MarkerSubrange&, bool isActiveMatch);
    160160
    161     void paintCompositionBackground(GraphicsContext&, const LayoutPoint& paintOffset);
     161    void paintCompositionBackground(GraphicsContext&, const FloatPoint& boxOrigin);
    162162    void paintCompositionUnderlines(GraphicsContext&, const FloatPoint& boxOrigin) const;
    163163    void paintCompositionUnderline(GraphicsContext&, const FloatPoint& boxOrigin, const CompositionUnderline&) const;
    164164
    165     void paintTextSubrangeBackground(GraphicsContext&, const LayoutPoint& paintOffset, const Color&, unsigned startOffset, unsigned endOffset);
    166 
    167     enum class SelectionRectRounding { None, EnclosingIntRect };
    168     LayoutRect localSelectionRectWithClampedPositions(unsigned clampedStartPosition, unsigned clampedEndPosition, SelectionRectRounding = SelectionRectRounding::None) const;
     165    void paintTextSubrangeBackground(GraphicsContext&, const FloatPoint& boxOrigin, const Color&, unsigned startOffset, unsigned endOffset);
    169166
    170167    const RenderCombineText* combinedText() const;
Note: See TracChangeset for help on using the changeset viewer.