Bug 174856 - AX: Incorrect range from index and length in contenteditable with <p> tags
Summary: AX: Incorrect range from index and length in contenteditable with <p> tags
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 168957 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-07-26 01:52 PDT by Nan Wang
Modified: 2017-07-30 17:19 PDT (History)
11 users (show)

See Also:


Attachments
patch (5.49 KB, patch)
2017-07-26 02:03 PDT, Nan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2017-07-26 01:52:40 PDT
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>
Comment 1 Nan Wang 2017-07-26 02:03:40 PDT
Created attachment 316437 [details]
patch
Comment 2 chris fleizach 2017-07-26 08:57:35 PDT
Comment on attachment 316437 [details]
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?
Comment 3 Nan Wang 2017-07-26 09:03:41 PDT
Comment on attachment 316437 [details]
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.
Comment 4 chris fleizach 2017-07-26 09:08:03 PDT
Comment on attachment 316437 [details]
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
Comment 5 Nan Wang 2017-07-26 09:15:32 PDT
Comment on attachment 316437 [details]
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>
Comment 6 chris fleizach 2017-07-26 09:19:45 PDT
Comment on attachment 316437 [details]
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?
Comment 7 Nan Wang 2017-07-26 09:21:44 PDT
Comment on attachment 316437 [details]
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.
Comment 8 WebKit Commit Bot 2017-07-26 09:53:13 PDT
Comment on attachment 316437 [details]
patch

Clearing flags on attachment: 316437

Committed r219949: <http://trac.webkit.org/changeset/219949>
Comment 9 WebKit Commit Bot 2017-07-26 09:53:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 James Craig 2017-07-30 17:19:17 PDT
*** Bug 168957 has been marked as a duplicate of this bug. ***