Bug 134568 - Selection rects sent to ServicesOverlayController are wrong
Summary: Selection rects sent to ServicesOverlayController are wrong
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-02 15:41 PDT by Brady Eidson
Modified: 2014-07-03 12:17 PDT (History)
3 users (show)

See Also:


Attachments
Patch v1 (21.10 KB, patch)
2014-07-02 15:52 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v2 - Addressing both Ryosuke's and Tim's feedback (24.49 KB, patch)
2014-07-02 22:35 PDT, Brady Eidson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2014-07-02 15:41:50 PDT
Selection rects sent to ServicesOverlayController are wrong

<rdar://problem/16727796>
Comment 1 Brady Eidson 2014-07-02 15:52:27 PDT
Created attachment 234290 [details]
Patch v1
Comment 2 Tim Horton 2014-07-02 16:03:26 PDT
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"
Comment 3 Ryosuke Niwa 2014-07-02 18:39:51 PDT
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.
Comment 4 Brady Eidson 2014-07-02 20:25:29 PDT
(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.
Comment 5 Ryosuke Niwa 2014-07-02 20:29:47 PDT
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.
Comment 6 Tim Horton 2014-07-02 20:30:44 PDT
(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.
Comment 7 Brady Eidson 2014-07-02 21:37:50 PDT
(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.
Comment 8 Brady Eidson 2014-07-02 22:35:36 PDT
Created attachment 234317 [details]
Patch v2 - Addressing both Ryosuke's and Tim's feedback
Comment 9 Darin Adler 2014-07-03 10:33:16 PDT
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.
Comment 10 Darin Adler 2014-07-03 10:34:08 PDT
Does seem important to fix RTL later; probably not all that hard.
Comment 11 Brady Eidson 2014-07-03 10:45:23 PDT
(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.
Comment 12 Brady Eidson 2014-07-03 10:57:22 PDT
http://trac.webkit.org/changeset/170758
Comment 13 Brady Eidson 2014-07-03 11:00:56 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=134604 to followup on RTL
Comment 14 Brady Eidson 2014-07-03 12:17:00 PDT
http://trac.webkit.org/changeset/170760 followup