Bug 72382

Summary: [Gtk] atk_text_get_text_at_offset() sometimes fails to provide the correct line
Product: WebKit Reporter: Joanmarie Diggs <jdiggs>
Component: AccessibilityAssignee: Mario Sanchez Prada <mario>
Status: RESOLVED FIXED    
Severity: Normal CC: mario, mrobinson, pnormand
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 25531, 73431    
Attachments:
Description Flags
test script
none
Patch proposal + Unit test
none
Patch proposal + Unit test
none
Patch proposal + Unit Test
none
Patch proposal + Unit Test mrobinson: review+

Description Joanmarie Diggs 2011-11-15 05:58:47 PST
Created attachment 115151 [details]
test script

Steps to reproduce:

1. View http://www.vogella.de/articles/OSGi/article.html in epiphany
2. Launch the attached test script
3. In epiphany, press F7 to enable caret browsing
4. Position the caret at the heading '3.2 Coding'
5. Begin Down Arrowing

Results: The lines of text are correctly reported outside of the code examples. Inside the examples, characters are reported for lines on which they do not appear, and all lines seem to be incorrect.

(Perhaps a failed/overlooked 'flattening' case??)
Comment 1 Mario Sanchez Prada 2012-01-12 07:49:22 PST
Seems to be related to an additional '\n' that is being manually added in some cases, at the end of a line. See https://bugs.webkit.org/show_bug.cgi?id=25415#c78.

Now working on it...
Comment 2 Mario Sanchez Prada 2012-01-12 09:49:33 PST
Created attachment 122263 [details]
Patch proposal + Unit test
Comment 3 Mario Sanchez Prada 2012-01-17 00:50:42 PST
Comment on attachment 122263 [details]
Patch proposal + Unit test

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

> Source/WebKit/gtk/tests/testatk.c:796
> +static void testWebkitAtkGetTextAtOffserWithPreformattedText()

Obviously, this is a typo. The right name would be testWebkitAtkGetTextAtOffsetWithPreformattedText()
Comment 4 Mario Sanchez Prada 2012-01-17 04:59:48 PST
Created attachment 122750 [details]
Patch proposal + Unit test

New patch, fixing some stupid mistakes in the previous one.
Comment 5 Mario Sanchez Prada 2012-01-24 13:52:30 PST
Created attachment 123799 [details]
Patch proposal + Unit Test

Updating the patch after the refactor done for fixing bug 76783
Comment 6 Martin Robinson 2012-01-24 13:59:28 PST
Comment on attachment 123799 [details]
Patch proposal + Unit Test

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

> Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:-105
> -            box = box->nextTextBox();

Did you mean to remove this line? I don't see anyting about this in the ChangeLog?
Comment 7 Mario Sanchez Prada 2012-01-30 14:31:38 PST
Comment on attachment 123799 [details]
Patch proposal + Unit Test

While smoke testing some stuff with MiniBrowser and Orca, I found out today that this patch seems to cause some hangs when browsing with the caret, so I'm withdrawing the r? flag for now.
Comment 8 Joanmarie Diggs 2012-02-01 07:38:56 PST
(In reply to comment #7)
> (From update of attachment 123799 [details])
> While smoke testing some stuff with MiniBrowser and Orca, I found out today that this patch seems to cause some hangs when browsing with the caret

Found an Orca-free way to reliably reproduce the hang:

1. Launch the tester (below)
2. Launch Epiphany
3. Navigate via the caret

I get reliable and immediate hangs in Epiphany, along with the following pyatspi lookup error:

Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/pyatspi/registry.py", line 193, in eventWrapper
    return callback(event)
  File "./caret-moved.py", line 11, in listener
    print text.getTextAtOffset(event.detail1, pyatspi.TEXT_BOUNDARY_LINE_START)
  File "/usr/lib/python2.7/site-packages/pyatspi/text.py", line 443, in getTextAtOffset
    ret = exwrap(Atspi.Text.get_text_at_offset, self.obj, offset, type)
  File "/usr/lib/python2.7/site-packages/pyatspi/utils.py", line 339, in exwrap
    raise LookupError
LookupError

BTW pyatspi LookupErrors are the spawn of <insert your personal devil here>. I'll poke Mike Gorse.

--------------------
#!/usr/bin/python

import pyatspi

def listener(event):
    try:
        text = event.source.queryText()
    except NotImplementedError:
        return

    print text.getTextAtOffset(event.detail1, pyatspi.TEXT_BOUNDARY_LINE_START)

pyatspi.Registry.registerEventListener(listener, "object:text-caret-moved")
pyatspi.Registry.start()
--------------------
Comment 9 Joanmarie Diggs 2012-02-01 09:30:50 PST
(In reply to comment #6)
> (From update of attachment 123799 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123799&action=review
> 
> > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:-105
> > -            box = box->nextTextBox();
> 
> Did you mean to remove this line? I don't see anyting about this in the ChangeLog?

So... I'm going to test having put that line back. Unfortunately, I'm still building (building building) WebKitGtk due to having built Gtk+ and Glib from master. But in the meantime, it sure looks to me that this may be the source of the hang because I don't see anything else updating the value of box which is being looped over.
Comment 10 Joanmarie Diggs 2012-02-01 10:58:19 PST
So finally the build completed. I did some quick testing of WebKit from master with the patch proposed with the accidentally removed line put back. Bug seems to be fixed, hang is definitely gone.
Comment 11 Mario Sanchez Prada 2012-02-01 12:30:28 PST
Created attachment 124990 [details]
Patch proposal + Unit Test
Comment 12 Mario Sanchez Prada 2012-02-01 12:31:44 PST
(In reply to comment #6)
> (From update of attachment 123799 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123799&action=review
> 
> > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:-105
> > -            box = box->nextTextBox();
> 
> Did you mean to remove this line? I don't see anyting about this in the ChangeLog?

(In reply to comment #10)
> So finally the build completed. I did some quick testing of WebKit from master with the patch proposed with the accidentally removed line put back. Bug seems to be fixed, hang is definitely gone.

You both were right. Sorry for this mistake, and for making you waste your time... now attached the right one
Comment 13 Mario Sanchez Prada 2012-02-02 05:18:26 PST
Committed r106545: <http://trac.webkit.org/changeset/106545>