128806 2014-02-14 02:28:53 -0800 computeSelectionStart and computeSelectionEnd shouldn't trigger synchronous layout 2020-02-13 15:02:10 -0800 1 1 1 Unclassified WebKit Forms 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=207724 P2 Normal --- 127832 1 rniwa rniwa barraclough benjamin darin enrica kling koivisto oldest_to_newest 980708 0 rniwa 2014-02-14 02:28:53 -0800 This is similar to the bug 128478. computeSelectionStart() and computeSelectionStart() in HTMLTextFormControlElement use indexForVisiblePosition to compute start and end indexes but we shouldn't need to since we only have br elements and text nodes inside the shadow DOM of inputs and text area elements. We should be able to traverse through the shadow DOM with regular Position instead of VisiblePosition. 981154 1 224295 rniwa 2014-02-15 03:31:04 -0800 Created attachment 224295 Fixes the bug 981172 2 224295 darin 2014-02-15 09:37:50 -0800 Comment on attachment 224295 Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=224295&action=review > Source/WebCore/html/HTMLTextFormControlElement.cpp:333 > + int index = indexForPosition(position.deepEquivalent()); Why int rather than unsigned? > Source/WebCore/html/HTMLTextFormControlElement.cpp:601 > + index += std::min(length, static_cast<unsigned>(passedPosition.offsetInContainerNode())); Instead of static_cast<unsigned>, I suggest writing std::min<unsigned>. > Source/WebCore/html/HTMLTextFormControlElement.cpp:616 > +#ifndef NDEBUG > + VisiblePosition visiblePosition = passedPosition; > + unsigned indexComputedByVisiblePosition = 0; > + if (visiblePosition.isNotNull()) > + indexComputedByVisiblePosition = WebCore::indexForVisiblePosition(innerText, visiblePosition, false /* forSelectionPreservation */); > + ASSERT(index == indexComputedByVisiblePosition); > +#endif I think an assertion like this should be written with a helper function so it doesn’t need to be inside an #if. Also can consider ASSERT_DISABLED instead of NDEBUG. 981195 3 rniwa 2014-02-15 11:56:28 -0800 (In reply to comment #2) > (From update of attachment 224295 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224295&action=review > > > Source/WebCore/html/HTMLTextFormControlElement.cpp:333 > > + int index = indexForPosition(position.deepEquivalent()); > > Why int rather than unsigned? Will fix. > > Source/WebCore/html/HTMLTextFormControlElement.cpp:601 > > + index += std::min(length, static_cast<unsigned>(passedPosition.offsetInContainerNode())); > > Instead of static_cast<unsigned>, I suggest writing std::min<unsigned>. Will fix. > > Source/WebCore/html/HTMLTextFormControlElement.cpp:616 > > +#ifndef NDEBUG > > + VisiblePosition visiblePosition = passedPosition; > > + unsigned indexComputedByVisiblePosition = 0; > > + if (visiblePosition.isNotNull()) > > + indexComputedByVisiblePosition = WebCore::indexForVisiblePosition(innerText, visiblePosition, false /* forSelectionPreservation */); > > + ASSERT(index == indexComputedByVisiblePosition); > > +#endif > > I think an assertion like this should be written with a helper function so it doesn’t need to be inside an #if. Also can consider ASSERT_DISABLED instead of NDEBUG. I'd like to be able to see indexComputedByVisiblePosition and visiblePosition for debugging purposes when the assertion is hit. If I put the computation inside a helper function, it would be tricky to get those objects/values in the debugger. But I'd use ASSERT_DISABLED. 981210 4 rniwa 2014-02-15 15:19:57 -0800 Committed r164180: <http://trac.webkit.org/changeset/164180> 224295 2014-02-15 03:31:04 -0800 2014-02-15 09:37:50 -0800 Fixes the bug bug-128806-20140215033107.patch text/plain 6252 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2NDE3MSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI5IEBACisyMDE0LTAyLTE1ICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIGNvbXB1dGVTZWxlY3Rpb25TdGFy dCBhbmQgY29tcHV0ZVNlbGVjdGlvbkVuZCBzaG91bGRuJ3QgdHJpZ2dlciBzeW5jaHJvbm91cyBs YXlvdXQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEy ODgwNgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEFk ZGVkIGluZGV4Rm9yUG9zaXRpb24gdG8gSFRNTFRleHRGb3JtQ29udHJvbEVsZW1lbnQuIExpa2Ug cjE2MzgyNSwgdGhpcyBwYXRjaCB0cmF2ZXJzZXMgdGhlIERPTSB0cmVlCisgICAgICAgIGluc3Rl YWQgb2YgdGhlIHJlbmRlciB0cmVlIHRvIGNvbXB1dGUgdGhlIGluZGV4IGZvciBhIGdpdmVuIHBv c2l0aW9uLgorCisgICAgICAgIFdlIHRyYXZlcnNlIHRoZSBET00gVHJlZSBiYWNrd2FyZHMgc3Rh cnRpbmcgYXQgdGhlIHNwZWNpZmllZCBQb3NpdGlvbiBhbGwgdGhlIHdheSBiYWNrIHRvIHRoZSBi ZWdpbm5pbmcKKyAgICAgICAgb2YgdGhlIGlubmVyIHRleHQgZWxlbWVudC4gVGhlIGluZGV4IGlz IGNvbXB1dGVkIGFzIHRoZSBudW1iZXIgb2YgY2hhcmFjdGVycyB3ZSBlbmNvdW50ZXJlZCBkdXJp bmcKKyAgICAgICAgdGhpcyBiYWNrd2FyZHMgRE9NIHRyYXZlcnNhbC4KKworICAgICAgICBJdCdz IHdvcnRoIG5vdGluZyB0aGF0IHBhc3NlZFBvc2l0aW9uLmNvbXB1dGVOb2RlQmVmb3JlUG9zaXRp b24oKSByZXR1cm5zIGFuZCBvbmx5IHJldHVybnMgMCB3aGVuIHRoZQorICAgICAgICBwb3NpdGlv biBpcyBiZWZvcmUgdGhlIGZpcnN0IG5vZGUgb2YgaXRzIHBhcmVudCBvciBpbnNpZGUgYSB0ZXh0 IG5vZGUuIEluIHN1Y2ggY2FzZXMsIHdlIGNhbGwKKyAgICAgICAgcGFzc2VkUG9zaXRpb24uY29u dGFpbmVyTm9kZSgpIHRvIGZpbmQgdGhlIHBhcmVudCBvciB0aGUgdGV4dCBub2RlLgorCisgICAg ICAgICogaHRtbC9IVE1MVGV4dEZvcm1Db250cm9sRWxlbWVudC5jcHA6CisgICAgICAgIChXZWJD b3JlOjpIVE1MVGV4dEZvcm1Db250cm9sRWxlbWVudDo6aW5kZXhGb3JWaXNpYmxlUG9zaXRpb24p OiBVc2UgaW5kZXhGb3JQb3NpdGlvbi4KKyAgICAgICAgKFdlYkNvcmU6OkhUTUxUZXh0Rm9ybUNv bnRyb2xFbGVtZW50Ojpjb21wdXRlU2VsZWN0aW9uU3RhcnQpOiBEaXR0by4KKyAgICAgICAgKFdl YkNvcmU6OkhUTUxUZXh0Rm9ybUNvbnRyb2xFbGVtZW50Ojpjb21wdXRlU2VsZWN0aW9uRW5kKTog RG90dG8uCisgICAgICAgIChXZWJDb3JlOjpmaW5pc2hUZXh0KTogQ2xlYW51cC4gVXNlIG5ld2xp bmVDaGFyYWN0ZXIgaW5zdGVhZCBvZiBoYXJkLWNvZGluZyAnXG4nLgorICAgICAgICAoV2ViQ29y ZTo6SFRNTFRleHRGb3JtQ29udHJvbEVsZW1lbnQ6OmluZGV4Rm9yUG9zaXRpb24pOiBBZGRlZC4g U2VlIGFib3ZlIGZvciB0aGUgZGVzY3JpcHRpb24uCisgICAgICAgICogaHRtbC9IVE1MVGV4dEZv cm1Db250cm9sRWxlbWVudC5oOgorCiAyMDE0LTAyLTE1ICBSZW5hdGEgSG9kb3ZhbiAgPHJob2Rv dmFuLnUtc3plZ2VkQHBhcnRuZXIuc2Ftc3VuZy5jb20+CiAKICAgICAgICAgQVNTRVJUX1dJVEhf U0VDVVJJVFlfSU1QTElDQVRJT04gaW4gV2ViQ29yZTo6dG9FbGVtZW50CkluZGV4OiBTb3VyY2Uv V2ViQ29yZS9odG1sL0hUTUxUZXh0Rm9ybUNvbnRyb2xFbGVtZW50LmNwcAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxUZXh0Rm9ybUNvbnRyb2xFbGVtZW50LmNwcAkocmV2 aXNpb24gMTY0MTU3KQorKysgU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MVGV4dEZvcm1Db250cm9s RWxlbWVudC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTMzMCw4ICszMzAsNyBAQCBpbnQgSFRNTFRl eHRGb3JtQ29udHJvbEVsZW1lbnQ6OmluZGV4Rm9yCiAgICAgVGV4dENvbnRyb2xJbm5lclRleHRF bGVtZW50KiBpbm5lclRleHQgPSBpbm5lclRleHRFbGVtZW50KCk7CiAgICAgaWYgKCFpbm5lclRl eHQgfHwgIWlubmVyVGV4dC0+Y29udGFpbnMocG9zaXRpb24uZGVlcEVxdWl2YWxlbnQoKS5hbmNo b3JOb2RlKCkpKQogICAgICAgICByZXR1cm4gMDsKLSAgICBib29sIGZvclNlbGVjdGlvblByZXNl cnZhdGlvbiA9IGZhbHNlOwotICAgIHVuc2lnbmVkIGluZGV4ID0gV2ViQ29yZTo6aW5kZXhGb3JW aXNpYmxlUG9zaXRpb24oaW5uZXJUZXh0RWxlbWVudCgpLCBwb3NpdGlvbiwgZm9yU2VsZWN0aW9u UHJlc2VydmF0aW9uKTsKKyAgICBpbnQgaW5kZXggPSBpbmRleEZvclBvc2l0aW9uKHBvc2l0aW9u LmRlZXBFcXVpdmFsZW50KCkpOwogICAgIEFTU0VSVChWaXNpYmxlUG9zaXRpb24ocG9zaXRpb25G b3JJbmRleChpbm5lclRleHRFbGVtZW50KCksIGluZGV4KSkgPT0gcG9zaXRpb24pOwogICAgIHJl dHVybiBpbmRleDsKIH0KQEAgLTM2MCw3ICszNTksNyBAQCBpbnQgSFRNTFRleHRGb3JtQ29udHJv bEVsZW1lbnQ6OmNvbXB1dGVTCiAgICAgaWYgKCFmcmFtZSkKICAgICAgICAgcmV0dXJuIDA7CiAK LSAgICByZXR1cm4gaW5kZXhGb3JWaXNpYmxlUG9zaXRpb24oZnJhbWUtPnNlbGVjdGlvbigpLnNl bGVjdGlvbigpLnN0YXJ0KCkpOworICAgIHJldHVybiBpbmRleEZvclBvc2l0aW9uKGZyYW1lLT5z ZWxlY3Rpb24oKS5zZWxlY3Rpb24oKS5zdGFydCgpKTsKIH0KIAogaW50IEhUTUxUZXh0Rm9ybUNv bnRyb2xFbGVtZW50OjpzZWxlY3Rpb25FbmQoKSBjb25zdApAQCAtMzc5LDcgKzM3OCw3IEBAIGlu dCBIVE1MVGV4dEZvcm1Db250cm9sRWxlbWVudDo6Y29tcHV0ZVMKICAgICBpZiAoIWZyYW1lKQog ICAgICAgICByZXR1cm4gMDsKIAotICAgIHJldHVybiBpbmRleEZvclZpc2libGVQb3NpdGlvbihm cmFtZS0+c2VsZWN0aW9uKCkuc2VsZWN0aW9uKCkuZW5kKCkpOworICAgIHJldHVybiBpbmRleEZv clBvc2l0aW9uKGZyYW1lLT5zZWxlY3Rpb24oKS5zZWxlY3Rpb24oKS5lbmQoKSk7CiB9CiAKIHN0 YXRpYyBjb25zdCBBdG9taWNTdHJpbmcmIGRpcmVjdGlvblN0cmluZyhUZXh0RmllbGRTZWxlY3Rp b25EaXJlY3Rpb24gZGlyZWN0aW9uKQpAQCAtNTM3LDcgKzUzNiw3IEBAIHN0YXRpYyBTdHJpbmcg ZmluaXNoVGV4dChTdHJpbmdCdWlsZGVyJiAKIHsKICAgICAvLyBSZW1vdmUgb25lIHRyYWlsaW5n IG5ld2xpbmU7IHRoZXJlJ3MgYWx3YXlzIG9uZSB0aGF0J3MgY29sbGFwc2VkIG91dCBieSByZW5k ZXJpbmcuCiAgICAgc2l6ZV90IHNpemUgPSByZXN1bHQubGVuZ3RoKCk7Ci0gICAgaWYgKHNpemUg JiYgcmVzdWx0W3NpemUgLSAxXSA9PSAnXG4nKQorICAgIGlmIChzaXplICYmIHJlc3VsdFtzaXpl IC0gMV0gPT0gbmV3bGluZUNoYXJhY3RlcikKICAgICAgICAgcmVzdWx0LnJlc2l6ZSgtLXNpemUp OwogICAgIHJldHVybiByZXN1bHQudG9TdHJpbmcoKTsKIH0KQEAgLTU3OSw2ICs1NzgsNDUgQEAg c3RhdGljIFBvc2l0aW9uIHBvc2l0aW9uRm9ySW5kZXgoVGV4dENvbgogICAgIHJldHVybiBsYXN0 UG9zaXRpb25Jbk5vZGUoaW5uZXJUZXh0KTsKIH0KIAordW5zaWduZWQgSFRNTFRleHRGb3JtQ29u dHJvbEVsZW1lbnQ6OmluZGV4Rm9yUG9zaXRpb24oY29uc3QgUG9zaXRpb24mIHBhc3NlZFBvc2l0 aW9uKSBjb25zdAoreworICAgIFRleHRDb250cm9sSW5uZXJUZXh0RWxlbWVudCogaW5uZXJUZXh0 ID0gaW5uZXJUZXh0RWxlbWVudCgpOworICAgIGlmICghaW5uZXJUZXh0IHx8ICFpbm5lclRleHQt PmNvbnRhaW5zKHBhc3NlZFBvc2l0aW9uLmFuY2hvck5vZGUoKSkgfHwgcGFzc2VkUG9zaXRpb24u aXNOdWxsKCkpCisgICAgICAgIHJldHVybiAwOworCisgICAgaWYgKHBvc2l0aW9uQmVmb3JlTm9k ZShpbm5lclRleHQpID09IHBhc3NlZFBvc2l0aW9uKQorICAgICAgICByZXR1cm4gMDsKKworICAg IHVuc2lnbmVkIGluZGV4ID0gMDsKKyAgICBOb2RlKiBzdGFydE5vZGUgPSBwYXNzZWRQb3NpdGlv bi5jb21wdXRlTm9kZUJlZm9yZVBvc2l0aW9uKCk7CisgICAgaWYgKCFzdGFydE5vZGUpCisgICAg ICAgIHN0YXJ0Tm9kZSA9IHBhc3NlZFBvc2l0aW9uLmNvbnRhaW5lck5vZGUoKTsKKyAgICBBU1NF UlQoc3RhcnROb2RlKTsKKyAgICBBU1NFUlQoaW5uZXJUZXh0LT5jb250YWlucyhzdGFydE5vZGUp KTsKKworICAgIGZvciAoTm9kZSogbm9kZSA9IHN0YXJ0Tm9kZTsgbm9kZTsgbm9kZSA9IE5vZGVU cmF2ZXJzYWw6OnByZXZpb3VzKG5vZGUsIGlubmVyVGV4dCkpIHsKKyAgICAgICAgaWYgKG5vZGUt PmlzVGV4dE5vZGUoKSkgeworICAgICAgICAgICAgdW5zaWduZWQgbGVuZ3RoID0gdG9UZXh0KCpu b2RlKS5sZW5ndGgoKTsKKyAgICAgICAgICAgIGlmIChub2RlID09IHBhc3NlZFBvc2l0aW9uLmNv bnRhaW5lck5vZGUoKSkKKyAgICAgICAgICAgICAgICBpbmRleCArPSBzdGQ6Om1pbihsZW5ndGgs IHN0YXRpY19jYXN0PHVuc2lnbmVkPihwYXNzZWRQb3NpdGlvbi5vZmZzZXRJbkNvbnRhaW5lck5v ZGUoKSkpOworICAgICAgICAgICAgZWxzZQorICAgICAgICAgICAgICAgIGluZGV4ICs9IGxlbmd0 aDsKKyAgICAgICAgfSBlbHNlIGlmIChub2RlLT5oYXNUYWdOYW1lKGJyVGFnKSkKKyAgICAgICAg ICAgIGluZGV4Kys7CisgICAgfQorCisgICAgdW5zaWduZWQgbGVuZ3RoID0gaW5uZXJUZXh0VmFs dWUoKS5sZW5ndGgoKTsKKyAgICBpbmRleCA9IHN0ZDo6bWluKGluZGV4LCBsZW5ndGgpOyAvLyBG SVhNRTogV2Ugc2hvdWxkbid0IGhhdmUgdG8gY2FsbCBpbm5lclRleHRWYWx1ZSgpIGp1c3QgdG8g aWdub3JlIHRoZSBsYXN0IExGLiBTZWUgZmluaXNoVGV4dC4KKyNpZm5kZWYgTkRFQlVHCisgICAg VmlzaWJsZVBvc2l0aW9uIHZpc2libGVQb3NpdGlvbiA9IHBhc3NlZFBvc2l0aW9uOworICAgIHVu c2lnbmVkIGluZGV4Q29tcHV0ZWRCeVZpc2libGVQb3NpdGlvbiA9IDA7CisgICAgaWYgKHZpc2li bGVQb3NpdGlvbi5pc05vdE51bGwoKSkKKyAgICAgICAgaW5kZXhDb21wdXRlZEJ5VmlzaWJsZVBv c2l0aW9uID0gV2ViQ29yZTo6aW5kZXhGb3JWaXNpYmxlUG9zaXRpb24oaW5uZXJUZXh0LCB2aXNp YmxlUG9zaXRpb24sIGZhbHNlIC8qIGZvclNlbGVjdGlvblByZXNlcnZhdGlvbiAqLyk7CisgICAg QVNTRVJUKGluZGV4ID09IGluZGV4Q29tcHV0ZWRCeVZpc2libGVQb3NpdGlvbik7CisjZW5kaWYK KyAgICByZXR1cm4gaW5kZXg7Cit9CisKICNpZiBQTEFURk9STShJT1MpCiB2b2lkIEhUTUxUZXh0 Rm9ybUNvbnRyb2xFbGVtZW50OjpoaWRlUGxhY2Vob2xkZXIoKQogewpJbmRleDogU291cmNlL1dl YkNvcmUvaHRtbC9IVE1MVGV4dEZvcm1Db250cm9sRWxlbWVudC5oCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNv dXJjZS9XZWJDb3JlL2h0bWwvSFRNTFRleHRGb3JtQ29udHJvbEVsZW1lbnQuaAkocmV2aXNpb24g MTY0MTU3KQorKysgU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MVGV4dEZvcm1Db250cm9sRWxlbWVu dC5oCSh3b3JraW5nIGNvcHkpCkBAIC0xMjQsNiArMTI0LDggQEAgcHJpdmF0ZToKICAgICB2aXJ0 dWFsIHZvaWQgZGlzcGF0Y2hCbHVyRXZlbnQoUGFzc1JlZlB0cjxFbGVtZW50PiBuZXdGb2N1c2Vk RWxlbWVudCkgb3ZlcnJpZGUgZmluYWw7CiAgICAgdmlydHVhbCBib29sIGNoaWxkU2hvdWxkQ3Jl YXRlUmVuZGVyZXIoY29uc3QgTm9kZSYpIGNvbnN0IG92ZXJyaWRlOwogCisgICAgdW5zaWduZWQg aW5kZXhGb3JQb3NpdGlvbihjb25zdCBQb3NpdGlvbiYpIGNvbnN0OworCiAgICAgLy8gUmV0dXJu cyB0cnVlIGlmIHVzZXItZWRpdGFibGUgdmFsdWUgaXMgZW1wdHkuIFVzZWQgdG8gY2hlY2sgcGxh Y2Vob2xkZXIgdmlzaWJpbGl0eS4KICAgICB2aXJ0dWFsIGJvb2wgaXNFbXB0eVZhbHVlKCkgY29u c3QgPSAwOwogICAgIC8vIFJldHVybnMgdHJ1ZSBpZiBzdWdnZXN0ZWQgdmFsdWUgaXMgZW1wdHku IFVzZWQgdG8gY2hlY2sgcGxhY2Vob2xkZXIgdmlzaWJpbGl0eS4K