Bug 134568 – Selection rects sent to ServicesOverlayController are wrong

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Selection rects sent to ServicesOverlayController are wrong

<rdar://problem/16727796>

[Brady Eidson]

Created attachment 234290 [details]
Patch v1

[Tim Horton]

Comment on attachment 234290 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=234290&action=review

WebKit2 part is fine, but you're going to need rniwa/enrica/darin review for the other part.

> Source/WebCore/editing/SelectionRectGatherer.cpp:65
> +    // 1 rect selections need to be extended by the gap rects in both directions.

It seems unfortunate that this code isn't shared with the painting-time code that does the same thing.

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:55
> +SOFT_LINK(DataDetectors, DDHighlightCreateWithRectsInVisibleRectWithStyleAndDirection, DDHighlightRef, (CFAllocatorRef allocator, CGRect * rects, CFIndex count, CGRect globalVisibleRect, DDHighlightStyle style, Boolean withArrow, NSWritingDirection writingDirection, Boolean endsWithEOL, Boolean flipped), (allocator, rects, count, globalVisibleRect, style, withArrow, writingDirection, endsWithEOL, flipped))

star in "rects" is in the wrong place

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:138
> +    // DataDetectors needs these in the reverse order.

"why"

[Ryosuke Niwa]

Comment on attachment 234290 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=234290&action=review

> Source/WebCore/editing/SelectionRectGatherer.cpp:84
> +            switch (m_rects.size()) {
> +            case 1:
> +                m_rects[0].shiftXEdgeTo(rects.left().x());
> +                break;
> +            case 2:
> +                m_rects[1].shiftXEdgeTo(rects.left().x());
> +                break;
> +            case 3:
> +                m_rects[1].shiftXEdgeTo(rects.left().x());
> +                m_rects[2].shiftXEdgeTo(rects.left().x());
> +                break;
> +            default:
> +                ASSERT_NOT_REACHED();
> +            }

I don't see how this code could be right in RTL or vertical writing mode.
Also, it's not great to communicate three different cases using the number of rects.

> Source/WebCore/editing/SelectionRectGatherer.cpp:119
> +void SelectionRectGatherer::unifyMiddleRects()
> +{
> +    ASSERT(m_rects.size() > 3);
> +
> +    LayoutRect united;
> +    for (unsigned i = 1; i < m_rects.size() - 1; ++i)
> +        united.unite(m_rects[i]);
> +
> +    Vector<LayoutRect> newRects;
> +    newRects.resize(3);
> +    newRects[0] = m_rects[0];
> +    newRects[1] = united;
> +    newRects[2] = m_rects.last();
> +
> +    swap(m_rects, newRects);
> +}

I think it's better we didn't split this logic into a separate function, and instead allocated some enum indicating the type
and then three different variables indicating the semantics of each rect instead of swapping m_rect.

> Source/WebCore/editing/SelectionRectGatherer.cpp:135
> +    if (m_rects[1].x() > m_rects[2].x())
> +        m_rects[2].shiftXEdgeTo(m_rects[1].x());
> +    else if (m_rects[1].x() < m_rects[2].x())
> +        m_rects[1].shiftXEdgeTo(m_rects[2].x());
> +
> +    LayoutUnit topRightEdge = m_rects[0].x() + m_rects[0].width();
> +    LayoutUnit middleRightEdge = m_rects[1].x() + m_rects[1].width();
> +    if (topRightEdge > middleRightEdge)
> +        m_rects[1].shiftMaxXEdgeTo(topRightEdge);
> +    else if (topRightEdge < middleRightEdge)
> +        m_rects[0].shiftMaxXEdgeTo(middleRightEdge);

Again, I'm not certain how this logic is going to work in RTL or vertical writing mode.

> Source/WebCore/rendering/RenderObject.cpp:2521
> +    rects.append(selectionRectForRepaint(repaintContainer, clipToVisibleContent));
> +    return rects.last();

Always appending a vector like this for every selection rect is going to be VERY expensive.
We need to have an inline capacity to mitigate that.

> Source/WebCore/rendering/RenderObject.h:802
> +    // Form of selectionRectForRepaint that also preserves the individual rects.
> +    virtual LayoutRect selectionRectsForRepaint(const RenderLayerModelObject* /*repaintContainer*/, bool /*clipToVisibleContent*/, Vector<LayoutRect>& /*rects*/);

I don't think we should call this selectionRectsForRepaint and have a comment like that.
We should rename the function instead to have a more descriptive name.
e.g. collectSelectionRectsForLineBoxes.

[Brady Eidson]

(In reply to comment #3)
> (From update of attachment 234290 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234290&action=review
> 
> > Source/WebCore/editing/SelectionRectGatherer.cpp:84
> > +            switch (m_rects.size()) {
> > +            case 1:
> > +                m_rects[0].shiftXEdgeTo(rects.left().x());
> > +                break;
> > +            case 2:
> > +                m_rects[1].shiftXEdgeTo(rects.left().x());
> > +                break;
> > +            case 3:
> > +                m_rects[1].shiftXEdgeTo(rects.left().x());
> > +                m_rects[2].shiftXEdgeTo(rects.left().x());
> > +                break;
> > +            default:
> > +                ASSERT_NOT_REACHED();
> > +            }
> 
> I don't see how this code could be right in RTL or vertical writing mode.

Indisputably, the current code is broken in all cases.

I agree it should be fixed for RTL.  After making the enum change you suggest there will be an obvious place to fix in a much smaller followup patch.

I'm not concerned about vertical text because the feature this patches in to doesn't correctly support vertical text.

> Also, it's not great to communicate three different cases using the number of rects.
> 
> > Source/WebCore/editing/SelectionRectGatherer.cpp:119
> > +void SelectionRectGatherer::unifyMiddleRects()
> > +{
> > +    ASSERT(m_rects.size() > 3);
> > +
> > +    LayoutRect united;
> > +    for (unsigned i = 1; i < m_rects.size() - 1; ++i)
> > +        united.unite(m_rects[i]);
> > +
> > +    Vector<LayoutRect> newRects;
> > +    newRects.resize(3);
> > +    newRects[0] = m_rects[0];
> > +    newRects[1] = united;
> > +    newRects[2] = m_rects.last();
> > +
> > +    swap(m_rects, newRects);
> > +}
> 
> I think it's better we didn't split this logic into a separate function, and instead allocated some enum indicating the type
> and then three different variables indicating the semantics of each rect instead of swapping m_rect.

I thought it read better to split into named functions that are explicit about what's being done, but I can also see the case for tighter binding with it all inline.  I will change that.

> 
> > Source/WebCore/editing/SelectionRectGatherer.cpp:135
> > +    if (m_rects[1].x() > m_rects[2].x())
> > +        m_rects[2].shiftXEdgeTo(m_rects[1].x());
> > +    else if (m_rects[1].x() < m_rects[2].x())
> > +        m_rects[1].shiftXEdgeTo(m_rects[2].x());
> > +
> > +    LayoutUnit topRightEdge = m_rects[0].x() + m_rects[0].width();
> > +    LayoutUnit middleRightEdge = m_rects[1].x() + m_rects[1].width();
> > +    if (topRightEdge > middleRightEdge)
> > +        m_rects[1].shiftMaxXEdgeTo(topRightEdge);
> > +    else if (topRightEdge < middleRightEdge)
> > +        m_rects[0].shiftMaxXEdgeTo(middleRightEdge);
> 
> Again, I'm not certain how this logic is going to work in RTL or vertical writing mode.

Same reply as above re: RTL and vertical.

> 
> > Source/WebCore/rendering/RenderObject.h:802
> > +    // Form of selectionRectForRepaint that also preserves the individual rects.
> > +    virtual LayoutRect selectionRectsForRepaint(const RenderLayerModelObject* /*repaintContainer*/, bool /*clipToVisibleContent*/, Vector<LayoutRect>& /*rects*/);
> 
> I don't think we should call this selectionRectsForRepaint and have a comment like that.
> We should rename the function instead to have a more descriptive name.
> e.g. collectSelectionRectsForLineBoxes.

If this has to go on RenderObject, I'm not sure the name should be so tightly bound to RenderText concepts.

Alternately the one place where it's used in RenderSelectionInfo can do an "isText()" check on the object and call collectSelectionRectsForLineBoxes from there.

That way...

> > Source/WebCore/rendering/RenderObject.cpp:2521
> > +    rects.append(selectionRectForRepaint(repaintContainer, clipToVisibleContent));
> > +    return rects.last();
> 
> Always appending a vector like this for every selection rect is going to be VERY expensive.
> We need to have an inline capacity to mitigate that.

... we don't need this at all.

[Ryosuke Niwa]

Comment on attachment 234290 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=234290&action=review

>>> Source/WebCore/rendering/RenderObject.h:802
>>> +    virtual LayoutRect selectionRectsForRepaint(const RenderLayerModelObject* /*repaintContainer*/, bool /*clipToVisibleContent*/, Vector<LayoutRect>& /*rects*/);
>> 
>> I don't think we should call this selectionRectsForRepaint and have a comment like that.
>> We should rename the function instead to have a more descriptive name.
>> e.g. collectSelectionRectsForLineBoxes.
> 
> If this has to go on RenderObject, I'm not sure the name should be so tightly bound to RenderText concepts.
> 
> Alternately the one place where it's used in RenderSelectionInfo can do an "isText()" check on the object and call collectSelectionRectsForLineBoxes from there.
> 
> That way...

I like that approach.

[Tim Horton]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 234290 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=234290&action=review
> > 
> > > Source/WebCore/editing/SelectionRectGatherer.cpp:84
> > > +            switch (m_rects.size()) {
> > > +            case 1:
> > > +                m_rects[0].shiftXEdgeTo(rects.left().x());
> > > +                break;
> > > +            case 2:
> > > +                m_rects[1].shiftXEdgeTo(rects.left().x());
> > > +                break;
> > > +            case 3:
> > > +                m_rects[1].shiftXEdgeTo(rects.left().x());
> > > +                m_rects[2].shiftXEdgeTo(rects.left().x());
> > > +                break;
> > > +            default:
> > > +                ASSERT_NOT_REACHED();
> > > +            }
> > 
> > I don't see how this code could be right in RTL or vertical writing mode.
> 
> Indisputably, the current code is broken in all cases.
> 
> I agree it should be fixed for RTL.  After making the enum change you suggest there will be an obvious place to fix in a much smaller followup patch.
> 
> I'm not concerned about vertical text because the feature this patches in to doesn't correctly support vertical text.

That would be (more) fine if this code weren't super generic WebCore code that someone else might come along and use. But since it is, the fact that it's broken for an non-default writing modes is probably not great.

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 234290 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=234290&action=review
> > > 
> > > > Source/WebCore/editing/SelectionRectGatherer.cpp:84
> > > > +            switch (m_rects.size()) {
> > > > +            case 1:
> > > > +                m_rects[0].shiftXEdgeTo(rects.left().x());
> > > > +                break;
> > > > +            case 2:
> > > > +                m_rects[1].shiftXEdgeTo(rects.left().x());
> > > > +                break;
> > > > +            case 3:
> > > > +                m_rects[1].shiftXEdgeTo(rects.left().x());
> > > > +                m_rects[2].shiftXEdgeTo(rects.left().x());
> > > > +                break;
> > > > +            default:
> > > > +                ASSERT_NOT_REACHED();
> > > > +            }
> > > 
> > > I don't see how this code could be right in RTL or vertical writing mode.
> > 
> > Indisputably, the current code is broken in all cases.
> > 
> > I agree it should be fixed for RTL.  After making the enum change you suggest there will be an obvious place to fix in a much smaller followup patch.
> > 
> > I'm not concerned about vertical text because the feature this patches in to doesn't correctly support vertical text.
> 
> That would be (more) fine if this code weren't super generic WebCore code that someone else might come along and use. But since it is, the fact that it's broken for an non-default writing modes is probably not great.

If you feel strongly about this concern, then the super generic WebCore code could be moved to WK2 close to the feature.

[Brady Eidson]

Created attachment 234317 [details]
Patch v2 - Addressing both Ryosuke's and Tim's feedback

[Darin Adler]

Comment on attachment 234317 [details]
Patch v2 - Addressing both Ryosuke's and Tim's feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=234317&action=review

> Source/WebCore/rendering/RenderTextLineBoxes.cpp:500
> +    for (auto box = m_first; box; box = box->nextTextBox()) {

I like to use auto* in cases like this to emphasize the fact that it’s a pointer. But auto is also OK.

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:139
> +    LayoutUnit leftEdge, rightEdge;
> +    bool hasLeftEdge = false, hasRightEdge = false;

Separate lines please. Also not sure we need these booleans. Could just nest the code a bit and we wouldn’t need the locals.

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:147
> +        rightEdge = gap.right().x() + gap.right().width();

Should be:

    rightEdge = gap.right().maxX();

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:178
> +        rects.resize(3);

Can use shrink here instead of resize; slightly more efficient.

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:204
> +        LayoutRect left, right;

Separate lines please.

[Darin Adler]

Does seem important to fix RTL later; probably not all that hard.

[Brady Eidson]

(In reply to comment #9)
> (From update of attachment 234317 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234317&action=review
> 
> > Source/WebCore/rendering/RenderTextLineBoxes.cpp:500
> > +    for (auto box = m_first; box; box = box->nextTextBox()) {
> 
> I like to use auto* in cases like this to emphasize the fact that it’s a pointer. But auto is also OK.

I'm kinda with you on this, but this file has 15 occurrences of "for (auto box = m_first; box; box = box->nextTextBox())" and the OCD in me won't let me change just this one.  =/

> > Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:139
> > +    LayoutUnit leftEdge, rightEdge;
> > +    bool hasLeftEdge = false, hasRightEdge = false;
> 
> Separate lines please. Also not sure we need these booleans. Could just nest the code a bit and we wouldn’t need the locals.

Okay!

> 
> > Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:147
> > +        rightEdge = gap.right().x() + gap.right().width();
> 
> Should be:
> 
>     rightEdge = gap.right().maxX();

Damn, that's much better!

> 
> > Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:178
> > +        rects.resize(3);
> 
> Can use shrink here instead of resize; slightly more efficient.

Just looked at Vector.h to find out what you mean.  Sounds good!

> 
> > Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:204
> > +        LayoutRect left, right;
> 
> Separate lines please.

Yup.

[Brady Eidson]

http://trac.webkit.org/changeset/170758

[Brady Eidson]

Filed https://bugs.webkit.org/show_bug.cgi?id=134604 to followup on RTL

[Brady Eidson]

http://trac.webkit.org/changeset/170760 followup