Bug 83435

Summary: [Gtk] atk_text_get_text_at_offset() fails to provide the correct line for paragraphs in list items whose text wraps
Product: WebKit Reporter: Joanmarie Diggs <jdiggs>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cfleizach, mario, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 25531    
Attachments:
Description Flags
test case
none
test script
none
Another test case
none
proposed patch with updated unit test
none
Updated patch to address review comments none

Joanmarie Diggs
Reported 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.
Attachments
test case (227 bytes, text/html)
2012-04-08 11:45 PDT, Joanmarie Diggs 		no flags
Details
test script (439 bytes, text/plain)
2012-04-08 11:46 PDT, Joanmarie Diggs 		no flags
Details
Another test case (1.71 KB, text/html)
2012-04-09 10:47 PDT, Joanmarie Diggs 		no flags
Details
proposed patch with updated unit test (5.55 KB, patch)
2012-08-14 17:09 PDT, Joanmarie Diggs 		no flags
DetailsFormatted DiffDiff
Updated patch to address review comments (5.60 KB, patch)
2012-08-15 09:46 PDT, Joanmarie Diggs 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Joanmarie Diggs
Comment 1 2012-04-08 11:46:09 PDT
Created attachment 136152 [details] test script
Joanmarie Diggs
Comment 2 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.)
Joanmarie Diggs
Comment 3 2012-08-14 17:09:42 PDT
Created attachment 158452 [details] proposed patch with updated unit test
Mario Sanchez Prada
Comment 4 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
Joanmarie Diggs
Comment 5 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().
Joanmarie Diggs
Comment 6 2012-08-15 03:07:26 PDT
Adding Chris to CC for review. (It's a one-line bug fix.)
Mario Sanchez Prada
Comment 7 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 :).
chris fleizach
Comment 8 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 ..."
Joanmarie Diggs
Comment 9 2012-08-15 09:46:43 PDT
Created attachment 158583 [details] Updated patch to address review comments
WebKit Review Bot
Comment 10 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>
WebKit Review Bot
Comment 11 2012-08-15 11:14:05 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.