Bug 112015 – [EFL] accessibility/textarea-line-for-index.html is failing

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Krzysztof Czech]

accessibility/textarea-line-for-index.html is failing on all EFL platforms.

[Rob Płóciennik]

Created attachment 215646 [details]
Proposed patch

[Rob Płóciennik]

Created attachment 215648 [details]
Rebased patch for a change

[Mario Sanchez Prada]

Comment on attachment 215648 [details]
Rebased patch for a change

View in context: https://bugs.webkit.org/attachment.cgi?id=215648&action=review

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:995
> +    if (!ATK_IS_TEXT(m_element))
> +        return -1;

We normally put this check at the beginning of the function. Could you please move it there?

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:998
> +    if (index > atk_text_get_character_count(ATK_TEXT(m_element)))
> +        return -1;

Maybe you could join this check with the index < 0 in a single if condition.

Also, you probably want to check index >= atk_text_get_character_count(ATK_TEXT(m_element)) here (opposite to just >)

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1003
> +    GOwnPtr<gchar> text(atk_text_get_text(ATK_TEXT(m_element), 0, index));
> +
> +    int lineNo = 0;
> +

You don't need these two extra blank lines here

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1006
> +    for (gchar* offset = text.get(); *offset; ++offset)
> +        if (*offset == '\n')
> +            ++lineNo;

Please add braces to this for loop. Even if it only has the if statement inside, it's multiline content and adding braces will make it clearer

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1035
>  int AccessibilityUIElement::lineForIndex(int index)

Same considerations here.

[Rob Płóciennik]

(In reply to comment #3)
> Also, you probably want to check index >= atk_text_get_character_count(ATK_TEXT(m_element)) here (opposite to just >)

If I understand correctly, the index equal to the string's length corresponds to a situation where the caret is placed right after the last character. Wouldn't that cause additional failures in cases where the string in question ended with a newline?

[Mario Sanchez Prada]

(In reply to comment #4)
> (In reply to comment #3)
> > Also, you probably want to check index >= atk_text_get_character_count(ATK_TEXT(m_element)) here (opposite to just >)
> 
> If I understand correctly, the index equal to the string's length corresponds 
> to a situation where the caret is placed right after the last character. 
> Wouldn't that cause additional failures in cases where the string in question 
> ended with a newline?

I'm not sure I understand what you mean? What kind of failures are you considering? Do you have an specific example/failing test case that we can use to discuss about this?

[Rob Płóciennik]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Also, you probably want to check index >= atk_text_get_character_count(ATK_TEXT(m_element)) here (opposite to just >)
> > 
> > If I understand correctly, the index equal to the string's length corresponds 
> > to a situation where the caret is placed right after the last character. 
> > Wouldn't that cause additional failures in cases where the string in question 
> > ended with a newline?
> 
> I'm not sure I understand what you mean? What kind of failures are you considering? Do you have an specific example/failing test case that we can use to discuss about this?

Let's consider a test string of "\n". Wouldn't a valid index range in this case be [0,1], with 0 corresponding to a caret placed just before the newline and 1 right after it, resulting in returned line numbers of 0 and 1 respectively? If so, shouldn't the index validity check accept ranges up to and including the length of the input string?

[Mario Sanchez Prada]

(In reply to comment #6)
> [...]
> Let's consider a test string of "\n". Wouldn't a valid index range in this
> case be [0,1], with 0 corresponding to a caret placed just before the newline
> and 1 right after it, resulting in returned line numbers of 0 and 1
> respectively? If so, shouldn't the index validity check accept ranges up to
> and including the length of the input string?

I see. The thing that confuses me a bit is that in this case the 'index' parameter actually acts more like a 'count'/'len' than as a 'index'/'position' in the text.

From that point of view, then obviously the text length is a valid value for that, and so my previous comment is non-sense. Sorry about that and thanks for the clarification.

[Rob Płóciennik]

Created attachment 215918 [details]
Corrected patch

[Mario Sanchez Prada]

Comment on attachment 215918 [details]
Corrected patch

Thanks

[WebKit Commit Bot]

Comment on attachment 215918 [details]
Corrected patch

Clearing flags on attachment: 215918

Committed r158576: <http://trac.webkit.org/changeset/158576>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.