160786 2016-08-11 16:20:10 -0700 InlineTextBox::isSelected() should only return true for a non-empty selection and remove incorrect FIXME from InlineTextBox::localSelectionRect() 2017-10-11 12:13:58 -0700 1 1 1 Unclassified WebKit Layout and Rendering WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=160735 InRadar P2 Normal --- 1 mmaxfield dbates dbates simon.fraser webkit-bug-importer zalan oldest_to_newest 1219441 0 mmaxfield 2016-08-11 16:20:10 -0700 InlineTextBox::isSelected: // FIXME: We should only be checking if sPos >= ePos here, because those are the // indices used to actually generate the selection rect. Allowing us past this guard // on any other condition creates zero-width selection rects. Same in InlineTextBox::localSelectionRect 1359003 1 dbates 2017-10-10 19:23:05 -0700 Since CSSOM View Module W3C Working Draft 22 February 2008, <https://www.w3.org/TR/2008/WD-cssom-view-20080222>, it has been acceptable for getClientRects() to return a rectangle with zero width or zero height. With respect to the latest draft [1], this is allowed by step 3 in the algorithm for getClientRects(): [[ 3. Return a DOMRectList object containing DOMRect objects in content order, one for each box fragment, describing its border area (including those with a height or width of zero) with the following constraints: ... ]] Notice the phrase "including those with a height or width of zero". [1] <https://drafts.csswg.org/cssom-view/#dom-element-getclientrects> (Editor's Draft, 15 September 2017) 1359004 2 dbates 2017-10-10 19:24:07 -0700 Re-opening to remove comment. 1359030 3 323379 dbates 2017-10-10 20:44:16 -0700 Created attachment 323379 Patch I did not rename the arguments of InlineTextBox::localSelectionRect() as I did for InlineTextBox::isSelected() because I felt it would add too much noise to this patch and I will refactor InlineTextBox::localSelectionRect() sometime this or next week anyway. 1359222 4 dbates 2017-10-11 09:40:37 -0700 For completeness, we have existing layout tests coverage the condition for the conditional in InlineTextBox::localSelectionRect() that allows for returning a rectangle with an empty width or height. One example of such a test is <https://trac.webkit.org/browser/trunk/LayoutTests/fast/text/international/iso-8859-8.html>. With regards to the change to the boolean expression in InlineTextBox::isSelected(), the proposed patch reverts the change to this expression made in <https://trac.webkit.org/changeset/204400> (bug #160735). I will update the ChangeLog entry to reflect this before landing this patch. I am unclear why r204400 amended the expression to allow InlineTextBox::isSelected() to return true for a non-selection and unfortunately r204400 neither included a layout test that demonstrates the need for this change nor explained a reason for it in its ChangeLog entry. Simon Fraser raised questions about this change in review in bug #160735, comment #6. These questions went unanswered :( As far as I can tell it represents a programmer error for InlineTextBox::isSelected() to be called with startPos == endPos (maybe we should assert this?). Lastly, RenderTextLineBoxes::setSelectionState() is the only caller of this function. 1359230 5 dbates 2017-10-11 10:20:24 -0700 I am going to update the patch to revert the change made in <https://trac.webkit.org/changeset/204400> (bug #160735) to localSelectionRect() with regards to the conditional for the early return. Additional remarks: Notice that <https://trac.webkit.org/changeset/204400> (bug #160735) changed the conditional for the early return in InlineTextBox::localSelectionRect() from: sPos > ePos to: sPos >= ePos && !(startPos == endPos && startPos >= start() && startPos <= (start() + len())) This seems unnecessary. I suspect this change was made to make localSelectionRect() work when passed positions that are completely outside the bounds of the text box. Passing such positions is illogical, represents a programmer error and we (the WebKit OpenSource Project) would be better served by catching such bad usage using an assertion(s) so that we can fix them instead of having localSelectionRect() effectively silently fail. 1359254 6 dbates 2017-10-11 11:04:58 -0700 (In reply to Daniel Bates from comment #5) > I am going to update the patch to revert the change made in > <https://trac.webkit.org/changeset/204400> (bug #160735) to > localSelectionRect() with regards to the conditional for the early return. > > Additional remarks: > > Notice that <https://trac.webkit.org/changeset/204400> (bug #160735) changed > the conditional for the early return in InlineTextBox::localSelectionRect() > from: > > sPos > ePos > > to: > > sPos >= ePos && !(startPos == endPos && startPos >= start() && startPos <= > (start() + len())) > > This seems unnecessary. I suspect this change was made to make > localSelectionRect() work when passed positions that are completely outside > the bounds of the text box. Passing such positions is illogical, represents > a programmer error and we (the WebKit OpenSource Project) would be better > served by catching such bad usage using an assertion(s) so that we can fix > them instead of having localSelectionRect() effectively silently fail. Disregard these remarks. It seems like the machinery for Element.getClientRects() has changed since r204400 such that that offsets of the DOM range can effectively be passed through to localSelectionRect() with little to no change (as can be seen in test 5 in layout test LayoutTests/fast/dom/Range/getClientRects.html); => it is the responsibility of localSelectionRect() to handle arbitrary positions and clamped them. 1359300 7 323379 mmaxfield 2017-10-11 11:58:15 -0700 Comment on attachment 323379 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323379&action=review > Source/WebCore/rendering/InlineTextBox.cpp:194 > + // This function is used to implement Element.getClientRects(), which can return a rectangle with zero Comments of the form "this is only used by X" often become out-of-date. 1359302 8 dbates 2017-10-11 12:03:44 -0700 (In reply to Myles C. Maxfield from comment #7) > Comment on attachment 323379 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323379&action=review > > > Source/WebCore/rendering/InlineTextBox.cpp:194 > > + // This function is used to implement Element.getClientRects(), which can return a rectangle with zero > > Comments of the form "this is only used by X" often become out-of-date. Will remove comment before landing. I only added the comment because a FIXME was added that implied there was confusion on how localSelectionRect() should behave for zero-width rectangles. 1359304 9 dbates 2017-10-11 12:04:40 -0700 (In reply to Daniel Bates from comment #8) > (In reply to Myles C. Maxfield from comment #7) > > Comment on attachment 323379 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=323379&action=review > > > > > Source/WebCore/rendering/InlineTextBox.cpp:194 > > > + // This function is used to implement Element.getClientRects(), which can return a rectangle with zero > > > > Comments of the form "this is only used by X" often become out-of-date. > > Will remove comment before landing. I only added the comment because a FIXME > was added that implied there was confusion on how localSelectionRect() > should behave for zero-width rectangles. Forgot to mention, we have test coverage to catch a regression. 1359314 10 dbates 2017-10-11 12:13:16 -0700 Committed r223196: <https://trac.webkit.org/changeset/223196> 1359315 11 webkit-bug-importer 2017-10-11 12:13:58 -0700 <rdar://problem/34938702> 323379 2017-10-10 20:44:16 -0700 2017-10-11 11:28:54 -0700 Patch bug-160786-20171010204415.patch text/plain 4664 dbates U3VidmVyc2lvbiBSZXZpc2lvbjogMjIzMTYyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNzlhZmZlNWU4NzViZWZl MWIzNWQxMjU1MGQwODlkMTE5Yzc1MTEwZS4uNDA0NTA2NGU3MDMyMjNlMWRiZmZlZGNhYTQ4YjY0 MDI2ZDEwYzkyMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDE3LTEwLTEwICBEYW5p ZWwgQmF0ZXMgIDxkYWJhdGVzQGFwcGxlLmNvbT4KKworICAgICAgICBJbmxpbmVUZXh0Qm94Ojpp c1NlbGVjdGVkKCkgc2hvdWxkIG9ubHkgcmV0dXJuIHRydWUgZm9yIGEgbm9uLWVtcHR5IHNlbGVj dGlvbgorICAgICAgICBhbmQgcmVtb3ZlIGluY29ycmVjdCBGSVhNRSBmcm9tIElubGluZVRleHRC b3g6OmxvY2FsU2VsZWN0aW9uUmVjdCgpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xNjA3ODYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBUaGUgZnVuY3Rpb24gSW5saW5lVGV4dEJveDo6aXNTZWxlY3RlZCgp IHNob3VsZCBvbmx5IHJldHVybiB0cnVlIGZvciBhIG5vbi1lbXB0eSBzZWxlY3Rpb24uCisgICAg ICAgIEFsc28gcmVtb3ZlIGFuIGluY29ycmVjdCBGSVhNRSBhZGRlZCB0byBJbmxpbmVUZXh0Qm94 Ojpsb2NhbFNlbGVjdGlvblJlY3QoKSB0aGF0IHF1ZXN0aW9uZWQKKyAgICAgICAgd2hldGhlciBp dCB3YXMgY29ycmVjdCBmb3IgaXQgdG8gcmV0dXJuIGFuIGVtcHR5IHJlY3RhbmdsZS4gSXQgaXMg Y29ycmVjdCBmb3IgaXQgdG8gcmV0dXJuCisgICAgICAgIHN1Y2ggYSByZWN0YW5nbGUgYmVjYXVz ZSB0aGlzIGZ1bmN0aW9uIGlzIHVzZWQgdG8gaW1wbGVtZW50IEVsZW1lbnQuZ2V0Q2xpZW50UmVj dHMoKS4gQW5kCisgICAgICAgIEVsZW1lbnQuZ2V0Q2xpZW50UmVjdHMoKSBjYW4gcmV0dXJuIGEg cmVjdGFuZ2xlIHdpdGggemVybyB3aWR0aCBvciB6ZXJvIGhlaWdodCBieSBzdGVwIDMKKyAgICAg ICAgb2YgYWxnb3JpdGhtIGdldENsaWVudFJlY3RzKCkgb2Ygc2VjdGlvbiBFeHRlbnNpb25zIHRv IHRoZSBFbGVtZW50IGludGVyZmFjZSBvZiB0aGUKKyAgICAgICAgQ1NTT00gVmlldyBNb2R1bGUg c3BlYy4sIDxodHRwczovL2RyYWZ0cy5jc3N3Zy5vcmcvY3Nzb20tdmlldy8+IChFZGl0b3IncyBE cmFmdCwgMTUgU2VwdGVtYmVyIDIwMTcpLgorCisgICAgICAgICogcmVuZGVyaW5nL0lubGluZVRl eHRCb3guY3BwOgorICAgICAgICAoV2ViQ29yZTo6SW5saW5lVGV4dEJveDo6aXNTZWxlY3RlZCBj b25zdCk6IE9ubHkgcmV0dXJuIHRydWUgZm9yIGEgbm9uLWVtcHR5IHNlbGVjdGlvbgorICAgICAg ICBhbmQgcmVtb3ZlIHVubmVjZXNzYXJ5IEZJWE1FLiBBbHNvIHJlbmFtZSB2YXJpYWJsZXMgdG8g aW1wcm92ZSByZWFkYWJpbGl0eS4KKyAgICAgICAgKFdlYkNvcmU6OklubGluZVRleHRCb3g6Omxv Y2FsU2VsZWN0aW9uUmVjdCBjb25zdCk6IFJlbW92ZSBpbmFjY3VyYXRlIEZJWE1FIGNvbW1lbnQu CisgICAgICAgICogcmVuZGVyaW5nL0lubGluZVRleHRCb3guaDoKKwogMjAxNy0xMC0xMCAgQW5k eSBFc3RlcyAgPGFlc3Rlc0BhcHBsZS5jb20+CiAKICAgICAgICAgW1BheW1lbnQgUmVxdWVzdF0g SW1wbGVtZW50IFBheW1lbnRSZXF1ZXN0LmNhbk1ha2VQYXltZW50KCkKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL3JlbmRlcmluZy9JbmxpbmVUZXh0Qm94LmNwcCBiL1NvdXJjZS9XZWJDb3Jl L3JlbmRlcmluZy9JbmxpbmVUZXh0Qm94LmNwcAppbmRleCBkODAwYjBhOGE1ZmRlZGM5YTAzNzNj NjRhNGMwMTE3ZDI0YmU1NWFjLi40M2Q0ZmFhODMwYTExMTQ4OWFhMGY2NGRhZjRiNTY5OTE4YmU3 MWVlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvSW5saW5lVGV4dEJveC5j cHAKKysrIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL0lubGluZVRleHRCb3guY3BwCkBAIC0x MjksMTUgKzEyOSw5IEBAIExheW91dFVuaXQgSW5saW5lVGV4dEJveDo6c2VsZWN0aW9uSGVpZ2h0 KCkgY29uc3QKICAgICByZXR1cm4gcm9vdCgpLnNlbGVjdGlvbkhlaWdodCgpOwogfQogCi1ib29s IElubGluZVRleHRCb3g6OmlzU2VsZWN0ZWQodW5zaWduZWQgc3RhcnRQb3MsIHVuc2lnbmVkIGVu ZFBvcykgY29uc3QKK2Jvb2wgSW5saW5lVGV4dEJveDo6aXNTZWxlY3RlZCh1bnNpZ25lZCBzdGFy dFBvc2l0aW9uLCB1bnNpZ25lZCBlbmRQb3NpdGlvbikgY29uc3QKIHsKLSAgICBpbnQgc1BvcyA9 IGNsYW1wZWRPZmZzZXQoc3RhcnRQb3MpOwotICAgIGludCBlUG9zID0gY2xhbXBlZE9mZnNldChl bmRQb3MpOwotICAgIC8vIEZJWE1FOiBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTYwNzg2Ci0gICAgLy8gV2Ugc2hvdWxkIG9ubHkgYmUgY2hlY2tpbmcgaWYgc1BvcyA+ PSBlUG9zIGhlcmUsIGJlY2F1c2UgdGhvc2UgYXJlIHRoZQotICAgIC8vIGluZGljZXMgdXNlZCB0 byBhY3R1YWxseSBnZW5lcmF0ZSB0aGUgc2VsZWN0aW9uIHJlY3QuIEFsbG93aW5nIHVzIHBhc3Qg dGhpcyBndWFyZAotICAgIC8vIG9uIGFueSBvdGhlciBjb25kaXRpb24gY3JlYXRlcyB6ZXJvLXdp ZHRoIHNlbGVjdGlvbiByZWN0cy4KLSAgICByZXR1cm4gc1BvcyA8IGVQb3MgfHwgKHN0YXJ0UG9z ID09IGVuZFBvcyAmJiBzdGFydFBvcyA+PSBzdGFydCgpICYmIHN0YXJ0UG9zIDw9IChzdGFydCgp ICsgbGVuKCkpKTsKKyAgICByZXR1cm4gY2xhbXBlZE9mZnNldChzdGFydFBvc2l0aW9uKSA8IGNs YW1wZWRPZmZzZXQoZW5kUG9zaXRpb24pOwogfQogCiBSZW5kZXJPYmplY3Q6OlNlbGVjdGlvblN0 YXRlIElubGluZVRleHRCb3g6OnNlbGVjdGlvblN0YXRlKCkKQEAgLTE5NywxMiArMTkxLDEwIEBA IExheW91dFJlY3QgSW5saW5lVGV4dEJveDo6bG9jYWxTZWxlY3Rpb25SZWN0KHVuc2lnbmVkIHN0 YXJ0UG9zLCB1bnNpZ25lZCBlbmRQb3MpCiAgICAgdW5zaWduZWQgc1BvcyA9IGNsYW1wZWRPZmZz ZXQoc3RhcnRQb3MpOwogICAgIHVuc2lnbmVkIGVQb3MgPSBjbGFtcGVkT2Zmc2V0KGVuZFBvcyk7 CiAKLSAgICAvLyBGSVhNRTogaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTE2MDc4NgotICAgIC8vIFdlIHNob3VsZCBvbmx5IGJlIGNoZWNraW5nIGlmIHNQb3MgPj0gZVBv cyBoZXJlLCBiZWNhdXNlIHRob3NlIGFyZSB0aGUKLSAgICAvLyBpbmRpY2VzIHVzZWQgdG8gYWN0 dWFsbHkgZ2VuZXJhdGUgdGhlIHNlbGVjdGlvbiByZWN0LiBBbGxvd2luZyB1cyBwYXN0IHRoaXMg Z3VhcmQKLSAgICAvLyBvbiBhbnkgb3RoZXIgY29uZGl0aW9uIGNyZWF0ZXMgemVyby13aWR0aCBz ZWxlY3Rpb24gcmVjdHMuCisgICAgLy8gVGhpcyBmdW5jdGlvbiBpcyB1c2VkIHRvIGltcGxlbWVu dCBFbGVtZW50LmdldENsaWVudFJlY3RzKCksIHdoaWNoIGNhbiByZXR1cm4gYSByZWN0YW5nbGUg d2l0aCB6ZXJvCisgICAgLy8gd2lkdGggb3IgemVybyBoZWlnaHQgYnkgdGhlIENTU09NIFZpZXcg TW9kdWxlLCA8aHR0cHM6Ly9kcmFmdHMuY3Nzd2cub3JnL2Nzc29tLXZpZXcvPiAoRWRpdG9yJ3Mg RHJhZnQsIDE1IFNlcHRlbWJlciAyMDE3KS4KICAgICBpZiAoc1BvcyA+PSBlUG9zICYmICEoc3Rh cnRQb3MgPT0gZW5kUG9zICYmIHN0YXJ0UG9zID49IHN0YXJ0KCkgJiYgc3RhcnRQb3MgPD0gKHN0 YXJ0KCkgKyBsZW4oKSkpKQotICAgICAgICByZXR1cm4gTGF5b3V0UmVjdCgpOworICAgICAgICBy ZXR1cm4geyB9OwogCiAgICAgTGF5b3V0VW5pdCBzZWxlY3Rpb25Ub3AgPSB0aGlzLT5zZWxlY3Rp b25Ub3AoKTsKICAgICBMYXlvdXRVbml0IHNlbGVjdGlvbkhlaWdodCA9IHRoaXMtPnNlbGVjdGlv bkhlaWdodCgpOwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL0lubGluZVRl eHRCb3guaCBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9JbmxpbmVUZXh0Qm94LmgKaW5kZXgg OTlmNjRkNWI0ZmU4OTQ4NTgyNTIxZjA4MTFkMWZjNTQyNDQ0YTIxMS4uZTdlYjg2N2IzMTY4MGRh MWE2YWZjYjAwMTRlMjAxYThlMTI5ZWZiYSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVu ZGVyaW5nL0lubGluZVRleHRCb3guaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvSW5s aW5lVGV4dEJveC5oCkBAIC0xMTEsNyArMTExLDcgQEAgcHVibGljOgogICAgIEZsb2F0UmVjdCBj YWxjdWxhdGVCb3VuZGFyaWVzKCkgY29uc3Qgb3ZlcnJpZGUgeyByZXR1cm4gRmxvYXRSZWN0KHgo KSwgeSgpLCB3aWR0aCgpLCBoZWlnaHQoKSk7IH0KIAogICAgIHZpcnR1YWwgTGF5b3V0UmVjdCBs b2NhbFNlbGVjdGlvblJlY3QodW5zaWduZWQgc3RhcnRQb3MsIHVuc2lnbmVkIGVuZFBvcykgY29u c3Q7Ci0gICAgYm9vbCBpc1NlbGVjdGVkKHVuc2lnbmVkIHN0YXJ0UG9zLCB1bnNpZ25lZCBlbmRQ b3MpIGNvbnN0OworICAgIGJvb2wgaXNTZWxlY3RlZCh1bnNpZ25lZCBzdGFydFBvc2l0aW9uLCB1 bnNpZ25lZCBlbmRQb3NpdGlvbikgY29uc3Q7CiAgICAgc3RkOjpwYWlyPHVuc2lnbmVkLCB1bnNp Z25lZD4gc2VsZWN0aW9uU3RhcnRFbmQoKSBjb25zdDsKIAogcHJvdGVjdGVkOgo=