Bug 72830 - [Gtk] Regression: text-inserted events lack text inserted and current line
: [Gtk] Regression: text-inserted events lack text inserted and current line
Status: RESOLVED FIXED
: WebKit
Accessibility
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: Gtk
:
: 25531
  Show dependency treegraph
 
Reported: 2011-11-20 11:59 PST by
Modified: 2012-01-09 02:16 PST (History)


Attachments
test script (550 bytes, text/plain)
2011-11-20 11:59 PST, Joanmarie Diggs (irc: joanie)
no flags Details
Patch proposal + unit test (24.42 KB, patch)
2011-11-25 08:18 PST, Mario Sanchez Prada
cfleizach: review+
Review Patch | Details | Formatted Diff | Diff
Patch proposal for the remaining issues + updated unit tests (17.70 KB, patch)
2012-01-05 07:38 PST, Mario Sanchez Prada
mrobinson: review+
mrobinson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-20 11:59:11 PST
Created an attachment (id=115991) [details]
test script

Steps to reproduce:

1. Launch the attached test script in an inaccessible terminal (e.g. xterm)
2. Launch epiphany
3. Navigate to http://live.gnome.org/Orca
4. Type something in the search entry of the resulting page

Expected results: After both steps 3 and 4, the test script would output the character inserted, the current line, and all of the text.

Actual results: The test script output is correct after step 3 (Gtk+ widget). However, after step 4, only the full text is correctly output. The event.any_data (which should contain the inserted text) and current line are empty.
------- Comment #1 From 2011-11-25 08:18:11 PST -------
Created an attachment (id=116624) [details]
Patch proposal + unit test

Attaching now a patch proposal that basically updates the code to using the new 'text-insert' and 'text-remove' signals (instead of the old, and now deprecated, 'text-changed' one), which allows us to emit the text being actually modified (inserted or removed) along with the signal, instead of just emitting the 'offset' and 'count' values and then relaying on the ATK bridge to get the right information at the right time, which is something we know does not always work fine (and one of the main reasons behind the deprecation of the 'text-changed' signal, AFAIK).

This patch also changes the signature of the nodeTextChangeNotification() and  nodeTextChangePlatformNotification() in AXObjectCache, to allow specifying the text  being modified from the text editing commands (as we're gonna send the modified text, there's no point in not providing this information to the AXObjectCache if we already know it, right?), and so I needed to update also the signatures in the platform-specific versions of that class for the mac, win and chromium ports too, so they will keep building. Fortunately, this change shouldn't impact those ports at all since they are basically ignoring this functionality and providing an empty implementation for them.

Last, I haven't included a new test with this patch, but just updated the a11y API test we already had in place to use the new signals instead of the old ones. Also, I made the most of this change to implement more careful and detailed checks in that unit test, since it's obvious that the old version of it was not good enough, as it didn't detected that things broke heavily recently (and thus the reason for this bug to be reported).
------- Comment #2 From 2011-11-25 08:20:10 PST -------
Adding reviewers to CC
------- Comment #3 From 2011-11-28 12:01:29 PST -------
(From update of attachment 116624 [details])
r=me
------- Comment #4 From 2011-11-29 03:04:58 PST -------
Committed r101349: <http://trac.webkit.org/changeset/101349>
------- Comment #5 From 2011-12-05 14:02:42 PST -------
Mario, using WebKitGtk built from this morning I am seeing this bug only partially fixed: The text inserted is now correct, but the current line seems to only be a single letter which may or may not correspond with the text displayed. Could you please double-check this? Thanks!
------- Comment #6 From 2011-12-05 14:22:27 PST -------
(In reply to comment #5)
> Mario, using WebKitGtk built from this morning I am seeing this bug only partially fixed: The text inserted is
> now correct, but the current line seems to only be a single letter which may or may not correspond with the
> text displayed. Could you please double-check this? Thanks!

Hmmm... I am not sure I understand. I've quickly checked this in the text entries in http://www.igalia.com/contact and all the events I see in accerciser seem to be correct, both in terms of the text displayed and the offset and count values.

Perhaps I am not reproducing properly the issue. Could you please point out specific steps for it? In any case, no need to hurry much since I won't be able to work much (if at all) on this month, since I am on holidays since today and so I can't promise much before January.
------- Comment #7 From 2011-12-06 04:25:11 PST -------
(In reply to comment #6)

> Hmmm... I am not sure I understand. I've quickly checked this in the text entries in http://www.igalia.com/contact and all the events I see in accerciser seem to be correct, both in terms of the text displayed and the offset and count values.

Yeah, but what about the current line? -- which you would have to use the ipython console for. Or alternatively, you could use my handy (and attached with the opening report) test script. ;)

When I type 'Acme Inc.' in the Company field at http://www.igalia.com/contact, here is what my test script prints out in Epiphany:

    Current line: A  
    All text: A  
    Text inserted: <c>  
    Current line: A          <-- WRONG  
    All text: Ac  
    Text inserted: <m>  
    Current line: A          <-- WRONG  
    All text: Acm  
    Text inserted: <e>  
    Current line: A          <-- WRONG  
    All text: Acme  
    Text inserted: < >  
    Current line: A          <-- WRONG  
    All text: Acme   
    Text inserted: <I>  
    Current line: A          <-- WRONG  
    All text: Acme I  
    Text inserted: <n>  
    Current line: A          <-- WRONG  
    All text: Acme In  
    Text inserted: <c>  
    Current line: A          <-- WRONG  
    All text: Acme Inc  
    Text inserted: <.>  
    Current line: A          <-- WRONG  
    All text: Acme Inc.  

Contrast that with what is seen in Epiphany's Gtk+ widget in which you can type the URL to navigate to.

    Text inserted: <A>  
    Current line: A  
    All text: A  
    Text inserted: <c>  
    Current line: Ac         <-- EXPECTED  
    All text: Ac  
    Text inserted: <m>  
    Current line: Acm        <-- EXPECTED  
    All text: Acm  
    Text inserted: <e>  
    Current line: Acme       <-- EXPECTED  
    All text: Acme  
    Text inserted: <,>  
    Current line: Acme,      <-- EXPECTED  
    All text: Acme,  
    Text inserted: < >  
    Current line: Acme,      <-- EXPECTED  
    All text: Acme,   
    Text inserted: <I>  
    Current line: Acme, I    <-- EXPECTED  
    All text: Acme, I  
    Text inserted: <n>  
    Current line: Acme, In   <-- EXPECTED  
    All text: Acme, In  
    Text inserted: <c>  
    Current line: Acme, Inc  <-- EXPECTED  
    All text: Acme, Inc  
    Text inserted: <.>  
    Current line: Acme, Inc. <-- EXPECTED  
    All text: Acme, Inc.  

Given that your response was based on what you were seeing in Accerciser (which does not spit out the current line of text automatically in the event monitor), I am going to assume that this bug is only partially fixed and REOPEN it so that it does not get forgotten.
------- Comment #8 From 2011-12-06 04:26:10 PST -------
Argh, seems I do not have the super powers required to REOPEN bugs. :-/ So UNCONFIRMED.
------- Comment #9 From 2011-12-06 13:58:49 PST -------
(In reply to comment #7)
>[...]
> Given that your response was based on what you were seeing
> in Accerciser (which does not spit out the current line of text
> automatically in the event monitor), I am going to assume that
> this bug is only partially fixed and REOPEN it so that it does
> not get forgotten.

Ok, I agree with reopening. Will try to tackle it asap then

Thanks for reporting the problem, and sorry not to have fixed it completely before (I thought I had)
------- Comment #10 From 2011-12-21 14:25:15 PST -------
Attachment 116624 [details] was posted by a committer and has review+, assigning to Mario Sanchez Prada for commit.
------- Comment #11 From 2012-01-04 03:08:42 PST -------
(In reply to comment #9)
> [...]
> Ok, I agree with reopening. Will try to tackle it asap then
> 
> Thanks for reporting the problem, and sorry not to have fixed it completely before (I thought I had)

JFTR, now working on this...
------- Comment #12 From 2012-01-05 07:38:25 PST -------
Created an attachment (id=121274) [details]
Patch proposal for the remaining issues + updated unit tests

This patch fixed the problem getting the current line, both for single line and multi-line text controls. It also fixes a problem getting the caret offset for text controls, which was causing trouble when retrieving the current line in multi-line text controls (so that's why it's included in this patch too, instead of a separate bug).

It also updates some caret-related unit tests in testatk.c, so we make sure we don't break this again in the future.
------- Comment #13 From 2012-01-05 10:24:47 PST -------
(From update of attachment 121274 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=121274&action=review

Looks good, but consider the following changes.

> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2717
> +AccessibilityObject* focusedObjectAndCaretOffsetUnignored(AccessibilityObject* objectOfReference, int& offset)

You can just call objectOfReference referenceObject, if you like.

> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2739
> +    AccessibilityObject* realObject = focusedObject;

I think I would prefer just continuing to use focusedObject here. After this point it seems unused and realObject is a little ambiguous.

> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.h:63
> +WebCore::AccessibilityObject* focusedObjectAndCaretOffsetUnignored(WebCore::AccessibilityObject* objectOfReference, int& offset);

objectFocusedAndCaretOffsetUnignored might be clearer. You can just omit the parameter name here instead of calling it objectOfReference.
------- Comment #14 From 2012-01-05 15:32:04 PST -------
(From update of attachment 121274 [details])
Attachment 121274 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11147137

New failing tests:
http/tests/inspector/network/download.html
------- Comment #15 From 2012-01-05 16:03:59 PST -------
(From update of attachment 121274 [details])
Remove cq bit since I think there are a few style changes I'd like Mario to consider before landing this patch.
------- Comment #16 From 2012-01-09 02:13:45 PST -------
(From update of attachment 121274 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=121274&action=review

Thanks for the review, Martin. I'm applying your suggestions to the patch and committing after that.

See my comments below.

>> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2717
>> +AccessibilityObject* focusedObjectAndCaretOffsetUnignored(AccessibilityObject* objectOfReference, int& offset)
> 
> You can just call objectOfReference referenceObject, if you like.

Ok. Changed.

>> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2739
>> +    AccessibilityObject* realObject = focusedObject;
> 
> I think I would prefer just continuing to use focusedObject here. After this point it seems unused and realObject is a little ambiguous.

Sounds fine to me. Just used realObject since it was what we had before this patch, but always using focusedObject sounds clearer to me too.

>> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.h:63
>> +WebCore::AccessibilityObject* focusedObjectAndCaretOffsetUnignored(WebCore::AccessibilityObject* objectOfReference, int& offset);
> 
> objectFocusedAndCaretOffsetUnignored might be clearer. You can just omit the parameter name here instead of calling it objectOfReference.

Ok. Changed
------- Comment #17 From 2012-01-09 02:16:34 PST -------
Committed r104446: <http://trac.webkit.org/changeset/104446>