174856 2017-07-26 01:52:40 -0700 AX: Incorrect range from index and length in contenteditable with <p> tags 2017-07-30 17:19:17 -0700 1 1 1 Unclassified WebKit Accessibility WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 n_wang webkit-unassigned aboxhall apinheiro buildbot cfleizach commit-queue dmazzoni jcraig jdiggs n_wang samuel_white webkit-bug-importer oldest_to_newest 1332291 0 n_wang 2017-07-26 01:52:40 -0700 For something like this: <div contenteditable="true"> <p>ab</p> <p>cd</p> </div> On Mac when we get AXValueAttribute value it would be: "ab\n\ncd" So getting the string from range {2, 4} should be: "\n\ncd" But instead we get: "\ncd" <rdar://problem/30187854> 1332298 1 316437 n_wang 2017-07-26 02:03:40 -0700 Created attachment 316437 patch 1332351 2 316437 cfleizach 2017-07-26 08:57:35 -0700 Comment on attachment 316437 patch View in context: https://bugs.webkit.org/attachment.cgi?id=316437&action=review > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3956 > + return m_object->doAXStringForRange(plainTextRange); is this not possible to fix in the characterOffsetForIndex? do you know the reason? 1332352 3 316437 n_wang 2017-07-26 09:03:41 -0700 Comment on attachment 316437 patch View in context: https://bugs.webkit.org/attachment.cgi?id=316437&action=review >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3956 >> + return m_object->doAXStringForRange(plainTextRange); > > is this not possible to fix in the characterOffsetForIndex? do you know the reason? I think the problem here is that for text controls, the AXValue comes from the AccessibilityNodeObject::text() (we call downcast<HTMLTextFormControlElement>(*node).value() or downcast<Element>(node)->innerText()). This value can be different from what textIterator gives us. In this case it will contain two newline characters instead of one visually. So when we try to get the range of text we should match that with the entire element value. 1332354 4 316437 cfleizach 2017-07-26 09:08:03 -0700 Comment on attachment 316437 patch View in context: https://bugs.webkit.org/attachment.cgi?id=316437&action=review >>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3956 >>> + return m_object->doAXStringForRange(plainTextRange); >> >> is this not possible to fix in the characterOffsetForIndex? do you know the reason? > > I think the problem here is that for text controls, the AXValue comes from the AccessibilityNodeObject::text() (we call downcast<HTMLTextFormControlElement>(*node).value() or downcast<Element>(node)->innerText()). > This value can be different from what textIterator gives us. In this case it will contain two newline characters instead of one visually. > So when we try to get the range of text we should match that with the entire element value. does this need to be nativeTextControl() then? I think textControl also pulls in role=textbox 1332359 5 316437 n_wang 2017-07-26 09:15:32 -0700 Comment on attachment 316437 patch View in context: https://bugs.webkit.org/attachment.cgi?id=316437&action=review >>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3956 >>>> + return m_object->doAXStringForRange(plainTextRange); >>> >>> is this not possible to fix in the characterOffsetForIndex? do you know the reason? >> >> I think the problem here is that for text controls, the AXValue comes from the AccessibilityNodeObject::text() (we call downcast<HTMLTextFormControlElement>(*node).value() or downcast<Element>(node)->innerText()). >> This value can be different from what textIterator gives us. In this case it will contain two newline characters instead of one visually. >> So when we try to get the range of text we should match that with the entire element value. > > does this need to be nativeTextControl() then? I think textControl also pulls in role=textbox I think isTextControl() is what we want, otherwise the problematic contenteditable wouldn't be covered. nativeTextControl() only returns true for form controls and <textarea> 1332363 6 316437 cfleizach 2017-07-26 09:19:45 -0700 Comment on attachment 316437 patch View in context: https://bugs.webkit.org/attachment.cgi?id=316437&action=review >>>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3956 >>>>> + return m_object->doAXStringForRange(plainTextRange); >>>> >>>> is this not possible to fix in the characterOffsetForIndex? do you know the reason? >>> >>> I think the problem here is that for text controls, the AXValue comes from the AccessibilityNodeObject::text() (we call downcast<HTMLTextFormControlElement>(*node).value() or downcast<Element>(node)->innerText()). >>> This value can be different from what textIterator gives us. In this case it will contain two newline characters instead of one visually. >>> So when we try to get the range of text we should match that with the entire element value. >> >> does this need to be nativeTextControl() then? I think textControl also pulls in role=textbox > > I think isTextControl() is what we want, otherwise the problematic contenteditable wouldn't be covered. nativeTextControl() only returns true for form controls and <textarea> ok so contenteitable also ends up using innerText() instead of a text iterator? 1332366 7 316437 n_wang 2017-07-26 09:21:44 -0700 Comment on attachment 316437 patch View in context: https://bugs.webkit.org/attachment.cgi?id=316437&action=review >>>>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3956 >>>>>> + return m_object->doAXStringForRange(plainTextRange); >>>>> >>>>> is this not possible to fix in the characterOffsetForIndex? do you know the reason? >>>> >>>> I think the problem here is that for text controls, the AXValue comes from the AccessibilityNodeObject::text() (we call downcast<HTMLTextFormControlElement>(*node).value() or downcast<Element>(node)->innerText()). >>>> This value can be different from what textIterator gives us. In this case it will contain two newline characters instead of one visually. >>>> So when we try to get the range of text we should match that with the entire element value. >>> >>> does this need to be nativeTextControl() then? I think textControl also pulls in role=textbox >> >> I think isTextControl() is what we want, otherwise the problematic contenteditable wouldn't be covered. nativeTextControl() only returns true for form controls and <textarea> > > ok so contenteitable also ends up using innerText() instead of a text iterator? Yes, for NSRange calls. The text marker stuff will still be using textIterator. 1332371 8 316437 commit-queue 2017-07-26 09:53:13 -0700 Comment on attachment 316437 patch Clearing flags on attachment: 316437 Committed r219949: <http://trac.webkit.org/changeset/219949> 1332372 9 commit-queue 2017-07-26 09:53:14 -0700 All reviewed patches have been landed. Closing bug. 1333351 10 jcraig 2017-07-30 17:19:17 -0700 *** Bug 168957 has been marked as a duplicate of this bug. *** 316437 2017-07-26 02:03:40 -0700 2017-07-26 09:53:13 -0700 patch patch.txt text/plain 5622 n_wang SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIxOTkxMSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDE3LTA3LTI2ICBOYW4gV2Fu ZyAgPG5fd2FuZ0BhcHBsZS5jb20+CisKKyAgICAgICAgQVg6IEluY29ycmVjdCByYW5nZSBmcm9t IGluZGV4IGFuZCBsZW5ndGggaW4gY29udGVudGVkaXRhYmxlIHdpdGggPHA+IHRhZ3MKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE3NDg1NgorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdoZW4gYXNraW5nIGZv ciB0aGUgc3RyaW5nIGluc2lkZSBhIHRleHQgY29udHJvbCB3aXRoIGEgZ2l2ZW4gcmFuZ2UsIHdl IHNvbWV0aW1lcyBnZXQKKyAgICAgICAgYSB3cm9uZyBzdHJpbmcgYXQgdGhlIGxpbmUgYm91bmRh cnkgZHVlIHRvIGEgYmFkIHBsYWluIHJhbmdlIHRvIHRleHQgbWFya2VyIGNvbnZlcnNpb24uCisg ICAgICAgIFRvIGZpeCB0aGlzLCB3ZSBzaG91bGQgdXNlIHRoZSBleHNpc3RpbmcgbWV0aG9kIG9u IHRleHQgY29udHJvbHMgdG8gYXZvaWQgdGhpcyBpc3N1ZS4gCisKKyAgICAgICAgVXBkYXRlZCB0 aGUgdGVzdCB0byB0ZXN0IHRoZSBwcm9ibGVtYXRpYyBjYXNlLgorCisgICAgICAgICogYWNjZXNz aWJpbGl0eS9tYWMvV2ViQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXJNYWMubW06CisgICAgICAg ICgtW1dlYkFjY2Vzc2liaWxpdHlPYmplY3RXcmFwcGVyIGFjY2Vzc2liaWxpdHlBdHRyaWJ1dGVW YWx1ZTpmb3JQYXJhbWV0ZXI6XSk6CisKIDIwMTctMDctMjYgIFphbiBEb2JlcnNlayAgPHpkb2Jl cnNla0BpZ2FsaWEuY29tPgogCiAgICAgICAgIFtXUEVdIFdlYkdMMiBzdXBwb3J0CkluZGV4OiBT b3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L21hYy9XZWJBY2Nlc3NpYmlsaXR5T2JqZWN0V3Jh cHBlck1hYy5tbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L21h Yy9XZWJBY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlck1hYy5tbQkocmV2aXNpb24gMjE5OTAxKQor KysgU291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9tYWMvV2ViQWNjZXNzaWJpbGl0eU9iamVj dFdyYXBwZXJNYWMubW0JKHdvcmtpbmcgY29weSkKQEAgLTM5NTEsNiArMzk1MSwxMSBAQCAtIChp ZClhY2Nlc3NpYmlsaXR5QXR0cmlidXRlVmFsdWU6KE5TU3RyCiAgICAgfQogICAgIAogICAgIGlm IChbYXR0cmlidXRlIGlzRXF1YWxUb1N0cmluZzpOU0FjY2Vzc2liaWxpdHlTdHJpbmdGb3JSYW5n ZVBhcmFtZXRlcml6ZWRBdHRyaWJ1dGVdKSB7CisgICAgICAgIGlmIChtX29iamVjdC0+aXNUZXh0 Q29udHJvbCgpKSB7CisgICAgICAgICAgICBQbGFpblRleHRSYW5nZSBwbGFpblRleHRSYW5nZSA9 IFBsYWluVGV4dFJhbmdlKHJhbmdlLmxvY2F0aW9uLCByYW5nZS5sZW5ndGgpOworICAgICAgICAg ICAgcmV0dXJuIG1fb2JqZWN0LT5kb0FYU3RyaW5nRm9yUmFuZ2UocGxhaW5UZXh0UmFuZ2UpOwor ICAgICAgICB9CisgICAgICAgIAogICAgICAgICBDaGFyYWN0ZXJPZmZzZXQgc3RhcnQgPSBjYWNo ZS0+Y2hhcmFjdGVyT2Zmc2V0Rm9ySW5kZXgocmFuZ2UubG9jYXRpb24sIG1fb2JqZWN0KTsKICAg ICAgICAgQ2hhcmFjdGVyT2Zmc2V0IGVuZCA9IGNhY2hlLT5jaGFyYWN0ZXJPZmZzZXRGb3JJbmRl eChyYW5nZS5sb2NhdGlvbiArIHJhbmdlLmxlbmd0aCwgbV9vYmplY3QpOwogICAgICAgICBpZiAo c3RhcnQuaXNOdWxsKCkgfHwgZW5kLmlzTnVsbCgpKQpJbmRleDogTGF5b3V0VGVzdHMvQ2hhbmdl TG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL0NoYW5nZUxvZwkocmV2aXNpb24gMjE5OTAx KQorKysgTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTMg QEAKKzIwMTctMDctMjYgIE5hbiBXYW5nICA8bl93YW5nQGFwcGxlLmNvbT4KKworICAgICAgICBB WDogSW5jb3JyZWN0IHJhbmdlIGZyb20gaW5kZXggYW5kIGxlbmd0aCBpbiBjb250ZW50ZWRpdGFi bGUgd2l0aCA8cD4gdGFncworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MTc0ODU2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgKiBhY2Nlc3NpYmlsaXR5L21hYy9yYW5nZS1mb3ItY29udGVudGVkaXRhYmxlLW5l d2xpbmUtZXhwZWN0ZWQudHh0OgorICAgICAgICAqIGFjY2Vzc2liaWxpdHkvbWFjL3JhbmdlLWZv ci1jb250ZW50ZWRpdGFibGUtbmV3bGluZS5odG1sOgorCiAyMDE3LTA3LTI1ICBBbmR5IEVzdGVz ICA8YWVzdGVzQGFwcGxlLmNvbT4KIAogICAgICAgICBbQXBwbGUgUGF5XSBBZGQgImNhcnRlQmFu Y2FpcmUiIGFzIGEgc3VwcG9ydGVkIHBheW1lbnQgbmV0d29yawpJbmRleDogTGF5b3V0VGVzdHMv YWNjZXNzaWJpbGl0eS9tYWMvcmFuZ2UtZm9yLWNvbnRlbnRlZGl0YWJsZS1uZXdsaW5lLWV4cGVj dGVkLnR4dAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9hY2Nlc3NpYmlsaXR5L21hYy9yYW5n ZS1mb3ItY29udGVudGVkaXRhYmxlLW5ld2xpbmUtZXhwZWN0ZWQudHh0CShyZXZpc2lvbiAyMTk5 MDEpCisrKyBMYXlvdXRUZXN0cy9hY2Nlc3NpYmlsaXR5L21hYy9yYW5nZS1mb3ItY29udGVudGVk aXRhYmxlLW5ld2xpbmUtZXhwZWN0ZWQudHh0CSh3b3JraW5nIGNvcHkpCkBAIC03LDkgKzcsOSBA QCBhYmMKIAogCiBkZWYKLVRoZSBxdWljayBicm93biBmb3gKK2FiCiAKLWp1bXBlZCBvdmVyIHRo ZSBsYXp5IGRvZworY2QgZWYKIAogVGV4dCBpbiBhIHByZSBlbGVtZW50CiBpcyBkaXNwbGF5ZWQg aW4gYSBmaXhlZC13aWR0aApAQCAtMjYsMTQgKzI2LDExIEBAIFBBU1MgdGV4dFRlc3QgaXMgJ3Rl c3QnCiBQQVNTIHRleHRhcmVhVGV4dEhlbGxvIGlzICdoZWxsbycKIFBBU1MgdGV4dGFyZWFUZXh0 V29ybGQgaXMgJ3dvcmxkJwogUEFTUyB0ZXh0REVGIGlzICdkZWYnCi1QQVNTIHJhbmdlRm9yTGlu ZSBpcyAnezIxLCAyNX0nCi1QQVNTIHRleHRMaW5lIGlzICdqdW1wZWQgb3ZlciB0aGUgbGF6eSBk b2cnCi1QQVNTIGZpcnN0TGluZSBpcyAwCi1QQVNTIHNlY29uZExpbmUgaXMgMQorUEFTUyByYW5n ZVZhbHVlIGlzICdbbmV3bGluZV1bbmV3bGluZV1jZCcKIFBBU1MgcmFuZ2VGb3JMaW5lIGlzICd7 MjIsIDMwfScKIFBBU1MgdGV4dExpbmUgaXMgJ2lzIGRpc3BsYXllZCBpbiBhIGZpeGVkLXdpZHRo XG4nCiBQQVNTIGZpcnN0TGluZSBpcyAwCi1GQUlMIHNlY29uZExpbmUgc2hvdWxkIGJlIDEuIFdh cyAyLgorUEFTUyBzZWNvbmRMaW5lIGlzIDEKIFBBU1Mgc3VjY2Vzc2Z1bGx5UGFyc2VkIGlzIHRy dWUKIAogVEVTVCBDT01QTEVURQpJbmRleDogTGF5b3V0VGVzdHMvYWNjZXNzaWJpbGl0eS9tYWMv cmFuZ2UtZm9yLWNvbnRlbnRlZGl0YWJsZS1uZXdsaW5lLmh0bWwKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gTGF5 b3V0VGVzdHMvYWNjZXNzaWJpbGl0eS9tYWMvcmFuZ2UtZm9yLWNvbnRlbnRlZGl0YWJsZS1uZXds aW5lLmh0bWwJKHJldmlzaW9uIDIxOTkwMSkKKysrIExheW91dFRlc3RzL2FjY2Vzc2liaWxpdHkv bWFjL3JhbmdlLWZvci1jb250ZW50ZWRpdGFibGUtbmV3bGluZS5odG1sCSh3b3JraW5nIGNvcHkp CkBAIC0xMyw4ICsxMyw4IEBAIHdvcmxkPC90ZXh0YXJlYT4KIDxkaXYgaWQ9InRleHRjb250cm9s MiIgY29udGVudGVkaXRhYmxlPSJ0cnVlIj5hYmM8ZGl2Pjxicj48L2RpdjxkaXY+PGJyPjwvZGl2 PjxkaXY+PGJyPjwvZGl2PjxkaXY+ZGVmPC9kaXY+PC9kaXY+CiAKIDxkaXYgaWQ9InRleHRjb250 cm9sMyIgY29udGVudGVkaXRhYmxlPSJ0cnVlIj4KLTxwPlRoZSBxdWljayBicm93biBmb3g8L3A+ Ci08cD5qdW1wZWQgb3ZlciB0aGUgbGF6eSBkb2c8L3A+Cis8cD5hYjwvcD4KKzxwPmNkIGVmPC9w PgogPC9kaXY+CiAKIDxkaXYgaWQ9InRleHRjb250cm9sNCIgY29udGVudGVkaXRhYmxlPSJ0cnVl Ij4KQEAgLTU5LDE0ICs1OSwxMCBAQCBmb250CiAgICAgCiAgICAgLy8gVGVzdCBsaW5lIHJhbmdl cyBpbiBjb250ZW50ZWRpdGFibGUKICAgICB2YXIgdGV4dENvbnRyb2wzID0gYWNjZXNzaWJpbGl0 eUNvbnRyb2xsZXIuYWNjZXNzaWJsZUVsZW1lbnRCeUlkKCJ0ZXh0Y29udHJvbDMiKTsKLSAgICB2 YXIgcmFuZ2VGb3JMaW5lID0gdGV4dENvbnRyb2wzLnJhbmdlRm9yTGluZSgxKTsKLSAgICBzaG91 bGRCZSgicmFuZ2VGb3JMaW5lIiwgIid7MjEsIDI1fSciKTsKLSAgICB2YXIgdGV4dExpbmUgPSB0 ZXh0Q29udHJvbDMuc3RyaW5nRm9yUmFuZ2UoMjEsIDI1KTsKLSAgICBzaG91bGRCZSgidGV4dExp bmUiLCAiJ2p1bXBlZCBvdmVyIHRoZSBsYXp5IGRvZyciKTsKLSAgICB2YXIgZmlyc3RMaW5lID0g dGV4dENvbnRyb2wzLmxpbmVGb3JJbmRleCgyMCk7Ci0gICAgdmFyIHNlY29uZExpbmUgPSB0ZXh0 Q29udHJvbDMubGluZUZvckluZGV4KDQ2KTsKLSAgICBzaG91bGRCZSgiZmlyc3RMaW5lIiwgIjAi KTsKLSAgICBzaG91bGRCZSgic2Vjb25kTGluZSIsICIxIik7CisgICAgdmFyIHJhbmdlVmFsdWUg PSB0ZXh0Q29udHJvbDMuc3RyaW5nRm9yUmFuZ2UoMiwgNCk7CisgICAgLy8gUmVwbGFjZSB0aGUg bmV3bGluZSBjaGFyYWN0ZXJzLCBhbmQgbWFrZSBzdXJlIGl0IGhhcyB0d28gb2YgdGhlbQorICAg IHJhbmdlVmFsdWUgPSByYW5nZVZhbHVlLnJlcGxhY2UoLyg/OlxyXG58XHJ8XG4pL2csICdbbmV3 bGluZV0nKTsgCisgICAgc2hvdWxkQmUoInJhbmdlVmFsdWUiLCAiJ1tuZXdsaW5lXVtuZXdsaW5l XWNkJyIpOwogICAgIAogICAgIC8vIHByZSB0YWcgaW4gY29udGVudGVkaXRhYmxlCiAgICAgdmFy IHRleHRDb250cm9sNCA9IGFjY2Vzc2liaWxpdHlDb250cm9sbGVyLmFjY2Vzc2libGVFbGVtZW50 QnlJZCgidGV4dGNvbnRyb2w0Iik7CkBAIC03NSw3ICs3MSw3IEBAIGZvbnQKICAgICB0ZXh0TGlu ZSA9IHRleHRDb250cm9sNC5zdHJpbmdGb3JSYW5nZSgyMiwgMzApOwogICAgIHNob3VsZEJlKCJ0 ZXh0TGluZSIsICInaXMgZGlzcGxheWVkIGluIGEgZml4ZWQtd2lkdGhcXG4nIik7CiAgICAgZmly c3RMaW5lID0gdGV4dENvbnRyb2w0LmxpbmVGb3JJbmRleCgyMSk7Ci0gICAgc2Vjb25kTGluZSA9 IHRleHRDb250cm9sNC5saW5lRm9ySW5kZXgoNTIpOworICAgIHNlY29uZExpbmUgPSB0ZXh0Q29u dHJvbDQubGluZUZvckluZGV4KDUxKTsKICAgICBzaG91bGRCZSgiZmlyc3RMaW5lIiwgIjAiKTsK ICAgICBzaG91bGRCZSgic2Vjb25kTGluZSIsICIxIik7CiAK