[Gtk] atk_text_get_caret_offset fails for list items

[Mario Sanchez Prada]

Steps to reproduce:

1. Open the test case attached to bug 53388 (attachment 80582 [details]) in Epiphany.

2. Enable caret mode by pressing F7 and manually (with the mouse) place the caret in the first item, right after the 'First' word.

3. Using Accerciser, attempt to get the caret position within that list item, by:

A. Selecting the corresponding object in Accerciser's tree of accessible objects

B. Typing 'acc.queryText().caretOffset()

Expected results: You should get 8 as the offset value

Actual results: You get 5 as the offset value, basically because it's plainly ignoring the fact that a 3-characted marker ('1. ') is present for that list item.

[Mario Sanchez Prada]

Created attachment 80659 [details]
Patch proposal + unit test

Attaching patch proposal + unit test (actually an update of another one)

[Mario Sanchez Prada]

Depending on bug 53388 as the unit test is an update of the code previously modified with the patch for that bug, so they should be applied in specific order

[Martin Robinson]

Comment on attachment 80659 [details]
Patch proposal + unit test

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

> Source/WebCore/ChangeLog:11
> +        (webkit_accessible_text_get_caret_offset): Ajust the offset with

Should be "Adjust" here.

> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1231
> +        offset += g_utf8_strlen(markerText.utf8().data(), -1);

Unless I'm mistaken, if you want the offset in bytes, you should use strlen(markerText.utf8().data()) if you want the offset in characters you can just use markerText.length().

[Mario Sanchez Prada]

Created attachment 80766 [details]
Patch proposal + unit tests

(In reply to comment #3)
> (From update of attachment 80659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80659&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        (webkit_accessible_text_get_caret_offset): Ajust the offset with
> 
> Should be "Adjust" here.

Fixed
 
> > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1231
> > +        offset += g_utf8_strlen(markerText.utf8().data(), -1);
> 
> Unless I'm mistaken, if you want the offset in bytes, you should use strlen(markerText.utf8().data()) if you want the offset in characters you can just use markerText.length().

You're right. Thanks for noticing.

Fixed either there and in webkit_accessible_text_set_caret_offset (and updated the ChangeLog)

[Xan Lopez]

Comment on attachment 80766 [details]
Patch proposal + unit tests

Looks good.

[Martin Robinson]

Comment on attachment 80766 [details]
Patch proposal + unit tests

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

> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1231
> +        // We need to adjust the offset for list items.

Might want to clarify this a little bit to say "We need to adjust the offset for the list item marker."

[Mario Sanchez Prada]

(In reply to comment #6)
> (From update of attachment 80766 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80766&action=review
> 
> > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1231
> > +        // We need to adjust the offset for list items.
> 
> Might want to clarify this a little bit to say "We need to adjust the offset for the list item marker."

:-) 

Yes, you're right, sorry. Specifying that the thing is related to the item marker adding some extra length to the exposed text will, definitely, be clearer.

I'll change that when committing then.

Thanks

[Mario Sanchez Prada]

Committed r77817: <http://trac.webkit.org/changeset/77817>