Bug 53436

Summary: [Gtk] atk_text_get_caret_offset fails for list items
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apinheiro, jdiggs
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 53388    
Bug Blocks: 25531    
Attachments:
Description Flags
Patch proposal + unit test
mrobinson: review-
Patch proposal + unit tests xan.lopez: review+

Description Mario Sanchez Prada 2011-01-31 10:11:47 PST
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.
Comment 1 Mario Sanchez Prada 2011-01-31 10:44:10 PST
Created attachment 80659 [details]
Patch proposal + unit test

Attaching patch proposal + unit test (actually an update of another one)
Comment 2 Mario Sanchez Prada 2011-01-31 10:45:04 PST
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
Comment 3 Martin Robinson 2011-01-31 15:32:01 PST
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().
Comment 4 Mario Sanchez Prada 2011-02-01 09:15:29 PST
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)
Comment 5 Xan Lopez 2011-02-07 08:12:27 PST
Comment on attachment 80766 [details]
Patch proposal + unit tests

Looks good.
Comment 6 Martin Robinson 2011-02-07 08:14:56 PST
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."
Comment 7 Mario Sanchez Prada 2011-02-07 08:17:59 PST
(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
Comment 8 Mario Sanchez Prada 2011-02-07 08:23:31 PST
Committed r77817: <http://trac.webkit.org/changeset/77817>