Bug 83435 – [Gtk] atk_text_get_text_at_offset() fails to provide the correct line for paragraphs in list items whose text wraps

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

Created attachment 136151 [details]
test case

+++ This bug was initially created as a clone of Bug #73431 +++

Steps to reproduce:

1. View the attached test case in Epiphany
2. Resize the window so that the text wraps
3. Enable caret browsing
4. Launch the attached test script in a terminal
5. Arrow Up and Down amongst the lines of text

Expected results: The test script would always print the correct text for the current line.

Actual results: The test script fails to present the correct text for the second list item.

[Joanmarie Diggs]

Created attachment 136152 [details]
test script

[Joanmarie Diggs]

Created attachment 136264 [details]
Another test case

An additional get_text_at_offset() failure/test case. (Not sure if it's a variant of the same bug or a different bug.)

[Joanmarie Diggs]

Created attachment 158452 [details]
proposed patch with updated unit test

[Mario Sanchez Prada]

Comment on attachment 158452 [details]
proposed patch with updated unit test

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

Thanks for the patch. It looks good to me with the exception of the missing comments in the ChangeLog.

You should still ask for an official review, though

> Source/WebCore/ChangeLog:11
> +        (textForRenderer):

A brief description of the change for textForRenderer would be nice here.

> Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:82
> +            if (object->isReplaced() && !object->isListMarker())

Oh! It indeed looks like a mistake in the code (and not coherent withthe comment).

Good catch (/me runs and hides)

> Source/WebKit/gtk/ChangeLog:11
> +        (testWebkitAtkGetTextAtOffsetWithSpecialCharacters):

Something like "Updated unit test" would be nice here

[Joanmarie Diggs]

(In reply to comment #4)
> (From update of attachment 158452 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158452&action=review
> 
> Thanks for the patch. It looks good to me with the exception of the missing comments in the ChangeLog.

Thanks for the review! :) Perhaps I should have coffee first and then respond, but.... What missing comments?

Looking at: https://bugs.webkit.org/attachment.cgi?id=158452&action=prettypatch, I see:

    Source/WebCore/ChangeLog
    8        Be sure the list item marker to be ignored is actually a list item marker prior to ignoring it.

    Source/WebKit/gtk/ChangeLog
    8        Include a paragraph in a list item when testing atk_text_get_text_at_offset().

[Joanmarie Diggs]

Adding Chris to CC for review. (It's a one-line bug fix.)

[Mario Sanchez Prada]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 158452 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=158452&action=review
> > 
> > Thanks for the patch. It looks good to me with the exception of the missing comments in the ChangeLog.
> 
> Thanks for the review! :) Perhaps I should have coffee first and then respond, but.... What missing comments?

Looks like I should be the one drinking coffee first :-). I meant the missing  comments _per-function_, but it's true that in this case it maybe makes not much sense since it would be duplicating the stuff in the general description.

Anyway, in my defense I must say I should not be here today as it's a bank day and so I'm officially not working :).

[chris fleizach]

Comment on attachment 158452 [details]
proposed patch with updated unit test

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

> Source/WebCore/ChangeLog:8
> +        Be sure the list item marker to be ignored is actually a list item marker prior to ignoring it.

i think this comment is a little hard to follow

Maybe something like "Fix a logic error when checking if an object is a list marker"

>> Source/WebKit/gtk/ChangeLog:11
>> +        (testWebkitAtkGetTextAtOffsetWithSpecialCharacters):
> 
> Something like "Updated unit test" would be nice here

Agreed "Updated unit test to include a paragraph in a list ..."

[Joanmarie Diggs]

Created attachment 158583 [details]
Updated patch to address review comments

[WebKit Review Bot]

Comment on attachment 158583 [details]
Updated patch to address review comments

Clearing flags on attachment: 158583

Committed r125685: <http://trac.webkit.org/changeset/125685>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.