157681 2016-05-13 12:33:25 -0700 Text selection is basically impossible on plain text pages 2016-06-08 15:47:00 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified iOS 9.3 RESOLVED FIXED InRadar P2 Normal --- 158155 158284 1 enrica enrica cdumez darin sam thorton oldest_to_newest 1192849 0 enrica 2016-05-13 12:33:25 -0700 Navigate to http://ohhla.com/anonymous/mf_doom/madvill/moneyfld.mfd.txt and try to select text. It is impossible. rdar://problem/26065660 1192855 1 278862 enrica 2016-05-13 12:41:13 -0700 Created attachment 278862 Patch 1193096 2 278862 darin 2016-05-14 10:35:24 -0700 Comment on attachment 278862 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()); 1193097 3 darin 2016-05-14 10:35:47 -0700 I would like to see test coverage for this. It doesn’t just affect plain text, so it can be HTML tests. 1193540 4 enrica 2016-05-16 14:52:00 -0700 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. 1193546 5 enrica 2016-05-16 15:06:57 -0700 (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. 1193549 6 enrica 2016-05-16 15:13:48 -0700 > 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. 1193552 7 enrica 2016-05-16 15:19:32 -0700 Committed revision 200972. 1197098 8 278862 cdumez 2016-05-27 10:08:39 -0700 Comment on attachment 278862 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. 1197121 9 darin 2016-05-27 11:18:05 -0700 Darn. I was suggesting "auto", not "auto*". 278862 2016-05-13 12:41:13 -0700 2016-05-14 10:35:24 -0700 Patch plain-text-page-selection.txt text/plain 4377 enrica SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDIwMDg3NikKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBACisyMDE2LTA1LTEzICBFbnJpY2Eg Q2FzdWNjaSAgPGVucmljYUBhcHBsZS5jb20+CisKKyAgICAgICAgVGV4dCBzZWxlY3Rpb24gaXMg YmFzaWNhbGx5IGltcG9zc2libGUgb24gcGxhaW4gdGV4dCBwYWdlcy4KKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1NzY4MQorICAgICAgICByZGFyOi8v cHJvYmxlbS8yNjA2NTY2MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIFdoZW4gZGVhbGluZyB3aXRoIGEgcGxhaW4gdGV4dCBmaWxlLCB0aGUgcnVsZXMg Zm9yIGRlY2lkaW5nIHdoZXRoZXIKKyAgICAgICAgYSBwb3NpdGlvbiBpcyBzZWxlY3RhYmxlIHNo b3VsZCBiZSBkaWZmZXJlbnQgYW5kIHdlIHNob3VsZCBuZXZlcgorICAgICAgICBzd2l0Y2ggdG8g YmxvY2sgc2VsZWN0aW9uLgorCisgICAgICAgICogV2ViUHJvY2Vzcy9XZWJQYWdlL2lvcy9XZWJQ YWdlSU9TLm1tOgorICAgICAgICAoV2ViS2l0OjpXZWJQYWdlOjpzaG91bGRTd2l0Y2hUb0Jsb2Nr TW9kZUZvckhhbmRsZSk6CisgICAgICAgIChXZWJLaXQ6OnJlY3RJc1Rvb0JpZ0ZvclNlbGVjdGlv bik6IEFkZGVkIGhlbHBlciBmdW5jdGlvbi4KKyAgICAgICAgKFdlYktpdDo6V2ViUGFnZTo6c2Vs ZWN0VGV4dFdpdGhHcmFudWxhcml0eUF0UG9pbnQpOgorICAgICAgICAoV2ViS2l0OjpXZWJQYWdl OjpnZXRQb3NpdGlvbkluZm9ybWF0aW9uKToKKwogMjAxNi0wNS0xMyAgU2FtIFdlaW5pZyAgPHNh bUB3ZWJraXQub3JnPgogCiAgICAgICAgIFJlbmFtZSBXS0RhdGFEZXRlY3RvclR5cGVTcG90bGln aHRTdWdnZXN0aW9uIHRvIFdLRGF0YURldGVjdG9yVHlwZUxvb2t1cFN1Z2dlc3Rpb24KSW5kZXg6 IFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFnZS9pb3MvV2ViUGFnZUlPUy5tbQo9PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvaW9zL1dlYlBhZ2VJ T1MubW0JKHJldmlzaW9uIDIwMDc3NykKKysrIFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2Vi UGFnZS9pb3MvV2ViUGFnZUlPUy5tbQkod29ya2luZyBjb3B5KQpAQCAtMTY1OCw2ICsxNjU4LDgg QEAgUmVmUHRyPFJhbmdlPiBXZWJQYWdlOjpzd2l0Y2hUb0Jsb2NrU2VsZQogCiBib29sIFdlYlBh Z2U6OnNob3VsZFN3aXRjaFRvQmxvY2tNb2RlRm9ySGFuZGxlKGNvbnN0IEludFBvaW50JiBoYW5k bGVQb2ludCwgU2VsZWN0aW9uSGFuZGxlUG9zaXRpb24gaGFuZGxlUG9zaXRpb24pCiB7CisgICAg aWYgKCFtX2Jsb2NrUmVjdEZvclRleHRTZWxlY3Rpb24uaGVpZ2h0KCkpCisgICAgICAgIHJldHVy biBmYWxzZTsKICAgICBzd2l0Y2ggKGhhbmRsZVBvc2l0aW9uKSB7CiAgICAgY2FzZSBTZWxlY3Rp b25IYW5kbGVQb3NpdGlvbjo6VG9wOgogICAgICAgICByZXR1cm4gaGFuZGxlUG9pbnQueSgpIDwg bV9ibG9ja1JlY3RGb3JUZXh0U2VsZWN0aW9uLnkoKTsKQEAgLTE4NTksNiArMTg2MSwxMiBAQCBQ YXNzUmVmUHRyPFJhbmdlPiBXZWJQYWdlOjpyYW5nZUZvckdyYW51CiAgICAgcmV0dXJuIHJhbmdl OwogfQogCitzdGF0aWMgYm9vbCByZWN0SXNUb29CaWdGb3JTZWxlY3Rpb24oY29uc3QgSW50UmVj dCYgYmxvY2tSZWN0LCBjb25zdCBGcmFtZSYgZnJhbWUpCit7CisgICAgY29uc3Qgc3RhdGljIENH RmxvYXQgZmFjdG9yID0gMC45NzsKKyAgICByZXR1cm4gYmxvY2tSZWN0LmhlaWdodCgpID4gKGZy YW1lLnZpZXcoKS0+dW5vYnNjdXJlZENvbnRlbnRSZWN0KCkuaGVpZ2h0KCkgKiBmYWN0b3IpOwor fQorCiB2b2lkIFdlYlBhZ2U6OnNlbGVjdFRleHRXaXRoR3JhbnVsYXJpdHlBdFBvaW50KGNvbnN0 IFdlYkNvcmU6OkludFBvaW50JiBwb2ludCwgdWludDMyX3QgZ3JhbnVsYXJpdHksIGJvb2wgaXNJ bnRlcmFjdGluZ1dpdGhBc3Npc3RlZE5vZGUsIHVpbnQ2NF90IGNhbGxiYWNrSUQpCiB7CiAgICAg Y29uc3QgRnJhbWUmIGZyYW1lID0gbV9wYWdlLT5mb2N1c0NvbnRyb2xsZXIoKS5mb2N1c2VkT3JN YWluRnJhbWUoKTsKQEAgLTE4NjcsOSArMTg3NSwxNyBAQCB2b2lkIFdlYlBhZ2U6OnNlbGVjdFRl eHRXaXRoR3JhbnVsYXJpdHlBCiAgICAgICAgIG1fYmxvY2tTZWxlY3Rpb25EZXNpcmVkU2l6ZS5z ZXRXaWR0aChibG9ja1NlbGVjdGlvblN0YXJ0V2lkdGgpOwogICAgICAgICBtX2Jsb2NrU2VsZWN0 aW9uRGVzaXJlZFNpemUuc2V0SGVpZ2h0KGJsb2NrU2VsZWN0aW9uU3RhcnRIZWlnaHQpOwogICAg ICAgICBtX2N1cnJlbnRCbG9ja1NlbGVjdGlvbiA9IG51bGxwdHI7Ci0gICAgICAgIFJlZlB0cjxS YW5nZT4gcGFyYWdyYXBoUmFuZ2UgPSBlbmNsb3NpbmdUZXh0VW5pdE9mR3JhbnVsYXJpdHkodmlz aWJsZVBvc2l0aW9uSW5Gb2N1c2VkTm9kZUZvclBvaW50KGZyYW1lLCBwb2ludCwgaXNJbnRlcmFj dGluZ1dpdGhBc3Npc3RlZE5vZGUpLCBQYXJhZ3JhcGhHcmFudWxhcml0eSwgRGlyZWN0aW9uRm9y d2FyZCk7Ci0gICAgICAgIGlmIChwYXJhZ3JhcGhSYW5nZSAmJiAhcGFyYWdyYXBoUmFuZ2UtPmNv bGxhcHNlZCgpKQotICAgICAgICAgICAgbV9ibG9ja1JlY3RGb3JUZXh0U2VsZWN0aW9uID0gc2Vs ZWN0aW9uQm94Rm9yUmFuZ2UocGFyYWdyYXBoUmFuZ2UuZ2V0KCkpOworICAgICAgICBhdXRvKiBy ZW5kZXJlciA9IHJhbmdlLT5zdGFydENvbnRhaW5lcigpLnJlbmRlcmVyKCk7CisgICAgICAgIGlm IChyZW5kZXJlci0+c3R5bGUoKS5wcmVzZXJ2ZU5ld2xpbmUoKSkKKyAgICAgICAgICAgIG1fYmxv Y2tSZWN0Rm9yVGV4dFNlbGVjdGlvbiA9IHJlbmRlcmVyLT5hYnNvbHV0ZUJvdW5kaW5nQm94UmVj dCh0cnVlKTsKKyAgICAgICAgZWxzZSB7CisgICAgICAgICAgICBSZWZQdHI8UmFuZ2U+IHBhcmFn cmFwaFJhbmdlID0gZW5jbG9zaW5nVGV4dFVuaXRPZkdyYW51bGFyaXR5KHZpc2libGVQb3NpdGlv bkluRm9jdXNlZE5vZGVGb3JQb2ludChmcmFtZSwgcG9pbnQsIGlzSW50ZXJhY3RpbmdXaXRoQXNz aXN0ZWROb2RlKSwgUGFyYWdyYXBoR3JhbnVsYXJpdHksIERpcmVjdGlvbkZvcndhcmQpOworICAg ICAgICAgICAgaWYgKHBhcmFncmFwaFJhbmdlICYmICFwYXJhZ3JhcGhSYW5nZS0+Y29sbGFwc2Vk KCkpCisgICAgICAgICAgICAgICAgbV9ibG9ja1JlY3RGb3JUZXh0U2VsZWN0aW9uID0gc2VsZWN0 aW9uQm94Rm9yUmFuZ2UocGFyYWdyYXBoUmFuZ2UuZ2V0KCkpOworICAgICAgICB9CisgICAgICAg IAorICAgICAgICBpZiAocmVjdElzVG9vQmlnRm9yU2VsZWN0aW9uKG1fYmxvY2tSZWN0Rm9yVGV4 dFNlbGVjdGlvbiwgZnJhbWUpKQorICAgICAgICAgICAgbV9ibG9ja1JlY3RGb3JUZXh0U2VsZWN0 aW9uLnNldEhlaWdodCgwKTsKICAgICB9CiAKICAgICBpZiAocmFuZ2UpCkBAIC0yMzg4LDggKzI0 MDQsOSBAQCB2b2lkIFdlYlBhZ2U6OmdldFBvc2l0aW9uSW5mb3JtYXRpb24oY29uCiAgICAgICAg ICAgICAgICAgaWYgKGF0dGFjaG1lbnQuZmlsZSgpKQogICAgICAgICAgICAgICAgICAgICBpbmZv LnVybCA9IGRvd25jYXN0PEhUTUxBdHRhY2htZW50RWxlbWVudD4oKmhpdE5vZGUpLmZpbGUoKS0+ cGF0aCgpOwogICAgICAgICAgICAgfSBlbHNlIHsKLSAgICAgICAgICAgICAgICBjb25zdCBzdGF0 aWMgQ0dGbG9hdCBmYWN0b3IgPSAwLjk3OwotICAgICAgICAgICAgICAgIGluZm8uaXNTZWxlY3Rh YmxlID0gcmVuZGVyZXItPnN0eWxlKCkudXNlclNlbGVjdCgpICE9IFNFTEVDVF9OT05FICYmIGlu Zm8uYm91bmRzLmhlaWdodCgpIDwgcmVzdWx0LmlubmVyTm9kZUZyYW1lKCktPnZpZXcoKS0+dW5v YnNjdXJlZENvbnRlbnRSZWN0KCkuaGVpZ2h0KCkgKiBmYWN0b3I7CisgICAgICAgICAgICAgICAg aW5mby5pc1NlbGVjdGFibGUgPSByZW5kZXJlci0+c3R5bGUoKS51c2VyU2VsZWN0KCkgIT0gU0VM RUNUX05PTkU7CisgICAgICAgICAgICAgICAgaWYgKGluZm8uaXNTZWxlY3RhYmxlICYmICFoaXRO b2RlLT5pc1RleHROb2RlKCkpCisgICAgICAgICAgICAgICAgICAgIGluZm8uaXNTZWxlY3RhYmxl ID0gIXJlY3RJc1Rvb0JpZ0ZvclNlbGVjdGlvbihpbmZvLmJvdW5kcywgKnJlc3VsdC5pbm5lck5v ZGVGcmFtZSgpKTsKICAgICAgICAgICAgIH0KICAgICAgICAgfQogICAgIH0K