Navigate to http://ohhla.com/anonymous/mf_doom/madvill/moneyfld.mfd.txt and try to select text. It is impossible. rdar://problem/26065660
Created attachment 278862 [details] Patch
Comment on attachment 278862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278862&action=review > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1662 > + if (!m_blockRectForTextSelection.height()) > + return false; What about very small heights? > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1864 > +static bool rectIsTooBigForSelection(const IntRect& blockRect, const Frame& frame) Consider inline maybe? > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1866 > + const static CGFloat factor = 0.97; I know this code was just moved, but: Why does this need to be a CGFloat? Why not just a float or a double? Doesn’t seem like CG is involved here. Also, const static is not better than const. > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1867 > + return blockRect.height() > (frame.view()->unobscuredContentRect().height() * factor); I know this code was just moved, but: What guarantees view() is non-null? No need for the parentheses in this expression. > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1880 > + if (renderer->style().preserveNewline()) > + m_blockRectForTextSelection = renderer->absoluteBoundingBoxRect(true); This seems not exactly right to me. Why is the only relevant flag “preserveNewline”? What about all the other related properties, like breakOnlyAfterWhiteSpace and auto wrap, etc. > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1882 > + RefPtr<Range> paragraphRange = enclosingTextUnitOfGranularity(visiblePositionInFocusedNodeForPoint(frame, point, isInteractingWithAssistedNode), ParagraphGranularity, DirectionForward); I know this code was just moved, but: I suggest auto rather than writing out the type RefPtr<Range>. > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2409 > + info.isSelectable = renderer->style().userSelect() != SELECT_NONE; > + if (info.isSelectable && !hitNode->isTextNode()) > + info.isSelectable = !rectIsTooBigForSelection(info.bounds, *result.innerNodeFrame()); This whole thing could use just &&: info.isSelectable = renderer->style().userSelect() != SELECT_NONE && !hitNode->isTextNode() && !rectIsTooBigForSelection(info.bounds, *result.innerNodeFrame());
I would like to see test coverage for this. It doesn’t just affect plain text, so it can be HTML tests.
Thanks for the review. I'll address your comments. See my replies below to a your questions. (In reply to comment #2) > Comment on attachment 278862 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=278862&action=review > > > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1662 > > + if (!m_blockRectForTextSelection.height()) > > + return false; > > What about very small heights? I tried to follow the original WK1 code that did not discard very small blocks. A height of 0 is explicitly set when we want to exclude the possibility of block selection. > I know this code was just moved, but: > > What guarantees view() is non-null? I thought that having a WebPage is a guarantee that the view is not null. Nowhere else in this file where a view is referenced we check for null.
(In reply to comment #2) > This seems not exactly right to me. Why is the only relevant flag > “preserveNewline”? What about all the other related properties, like > breakOnlyAfterWhiteSpace and auto wrap, etc. > I forgot to answer one of your questions. I believe this is the only relevant one. What we care about is properties that play a role when computing start and end of paragraph.
> This whole thing could use just &&: > > info.isSelectable = renderer->style().userSelect() != SELECT_NONE > && !hitNode->isTextNode() > && !rectIsTooBigForSelection(info.bounds, *result.innerNodeFrame()); Maybe I'm wrong, but I don't think this is equivalent. Written this way, if hitNode is a text node then isSelectable is false, which is not what we want. We want to allow selection on text nodes if they are selectable. We want to limit the size of selections on non text nodes.
Committed revision 200972.
Comment on attachment 278862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278862&action=review >> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1882 >> + RefPtr<Range> paragraphRange = enclosingTextUnitOfGranularity(visiblePositionInFocusedNodeForPoint(frame, point, isInteractingWithAssistedNode), ParagraphGranularity, DirectionForward); > > I know this code was just moved, but: > > I suggest auto rather than writing out the type RefPtr<Range>. The patch that landed used: auto* paragraphRange = enclosingTextUnitOfGranularity(visiblePositionInFocusedNodeForPoint(frame, point, isInteractingWithAssistedNode), ParagraphGranularity, DirectionForward).get(); This is unsafe because enclosingTextUnitOfGranularity() returns a RefPtr and transfers ownership of the range to the caller. This was causing crashes that Zalan is fixing via Bug 158155.
Darn. I was suggesting "auto", not "auto*".