Bug 83435 - [Gtk] atk_text_get_text_at_offset() fails to provide the correct line for paragraphs in list items whose text wraps
Summary: [Gtk] atk_text_get_text_at_offset() fails to provide the correct line for par...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 25531
  Show dependency treegraph
 
Reported: 2012-04-08 11:45 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2012-08-15 11:14 PDT (History)
3 users (show)

See Also:


Attachments
test case (227 bytes, text/html)
2012-04-08 11:45 PDT, Joanmarie Diggs (irc: joanie)
no flags Details
test script (439 bytes, text/plain)
2012-04-08 11:46 PDT, Joanmarie Diggs (irc: joanie)
no flags Details
Another test case (1.71 KB, text/html)
2012-04-09 10:47 PDT, Joanmarie Diggs (irc: joanie)
no flags Details
proposed patch with updated unit test (5.55 KB, patch)
2012-08-14 17:09 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Updated patch to address review comments (5.60 KB, patch)
2012-08-15 09:46 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2012-04-08 11:45:38 PDT
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.
Comment 1 Joanmarie Diggs (irc: joanie) 2012-04-08 11:46:09 PDT
Created attachment 136152 [details]
test script
Comment 2 Joanmarie Diggs (irc: joanie) 2012-04-09 10:47:38 PDT
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.)
Comment 3 Joanmarie Diggs (irc: joanie) 2012-08-14 17:09:42 PDT
Created attachment 158452 [details]
proposed patch with updated unit test
Comment 4 Mario Sanchez Prada 2012-08-15 01:39:02 PDT
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
Comment 5 Joanmarie Diggs (irc: joanie) 2012-08-15 02:11:55 PDT
(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().
Comment 6 Joanmarie Diggs (irc: joanie) 2012-08-15 03:07:26 PDT
Adding Chris to CC for review. (It's a one-line bug fix.)
Comment 7 Mario Sanchez Prada 2012-08-15 03:19:46 PDT
(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 :).
Comment 8 chris fleizach 2012-08-15 09:13:31 PDT
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 ..."
Comment 9 Joanmarie Diggs (irc: joanie) 2012-08-15 09:46:43 PDT
Created attachment 158583 [details]
Updated patch to address review comments
Comment 10 WebKit Review Bot 2012-08-15 11:14:02 PDT
Comment on attachment 158583 [details]
Updated patch to address review comments

Clearing flags on attachment: 158583

Committed r125685: <http://trac.webkit.org/changeset/125685>
Comment 11 WebKit Review Bot 2012-08-15 11:14:05 PDT
All reviewed patches have been landed.  Closing bug.