RESOLVED FIXED 134568
Selection rects sent to ServicesOverlayController are wrong
https://bugs.webkit.org/show_bug.cgi?id=134568
Summary Selection rects sent to ServicesOverlayController are wrong
Brady Eidson
Reported 2014-07-02 15:41:50 PDT
Selection rects sent to ServicesOverlayController are wrong <rdar://problem/16727796>
Attachments
Patch v1 (21.10 KB, patch)
2014-07-02 15:52 PDT, Brady Eidson
no flags
Patch v2 - Addressing both Ryosuke's and Tim's feedback (24.49 KB, patch)
2014-07-02 22:35 PDT, Brady Eidson
darin: review+
Brady Eidson
Comment 1 2014-07-02 15:52:27 PDT
Created attachment 234290 [details] Patch v1
Tim Horton
Comment 2 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"
Ryosuke Niwa
Comment 3 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.
Brady Eidson
Comment 4 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.
Ryosuke Niwa
Comment 5 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.
Tim Horton
Comment 6 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.
Brady Eidson
Comment 7 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.
Brady Eidson
Comment 8 2014-07-02 22:35:36 PDT
Created attachment 234317 [details] Patch v2 - Addressing both Ryosuke's and Tim's feedback
Darin Adler
Comment 9 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.
Darin Adler
Comment 10 2014-07-03 10:34:08 PDT
Does seem important to fix RTL later; probably not all that hard.
Brady Eidson
Comment 11 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.
Brady Eidson
Comment 12 2014-07-03 10:57:22 PDT
Brady Eidson
Comment 13 2014-07-03 11:00:56 PDT
Brady Eidson
Comment 14 2014-07-03 12:17:00 PDT
Note You need to log in before you can comment on or make changes to this bug.