Last Comment Bug 619002 - When deleting text from edit fields the wrong text is reported through at-spi
: When deleting text from edit fields the wrong text is reported through at-spi
Status: RESOLVED FIXED
[has patch][FF5]
: access, regression
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla7
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
: 669254 (view as bug list)
Depends on:
Blocks: a11ynext 561932
  Show dependency treegraph
 
Reported: 2010-12-13 20:22 PST by Michael Whapples
Modified: 2011-12-21 12:54 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+
wanted


Attachments
Patch using the proposed atk text-deleted and text-inserted signals (1.42 KB, patch)
2011-01-31 20:21 PST, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
updated patch from surkov's comments (1.66 KB, patch)
2011-01-31 21:18 PST, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
Updated patch from Fernando (1.73 KB, patch)
2011-02-08 23:39 PST, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Review
bug 619002 (2.39 KB, patch)
2011-03-24 15:08 PDT, Trevor Saunders (:tbsaunde)
fherrera: review+
Details | Diff | Review
bug 619002 (2.39 KB, patch)
2011-04-07 13:33 PDT, Trevor Saunders (:tbsaunde)
ginn.chen: review+
surkov.alexander: feedback+
Details | Diff | Review
follow up addressing surkovs comments for 619002 (4.13 KB, patch)
2011-04-09 22:35 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
follow up addressing surkovs comments for 619002 (4.09 KB, patch)
2011-04-10 06:16 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
follow up addressing surkovs comments for 619002 (4.09 KB, patch)
2011-04-11 16:55 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
follow up addressing surkovs comments for 619002 (4.22 KB, patch)
2011-04-11 18:40 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
patch to land (3.11 KB, patch)
2011-04-11 19:12 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
test idea (709 bytes, patch)
2011-06-15 22:45 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
patch (6.13 KB, patch)
2011-07-04 06:54 PDT, Trevor Saunders (:tbsaunde)
dbolter: review+
fherrera: review+
Details | Diff | Review
remove the warning with atk < 1.32 (1.19 KB, patch)
2011-12-12 03:17 PST, Ginn Chen
tbsaunde+mozbugs: review+
Details | Diff | Review

Description Michael Whapples 2010-12-13 20:22:07 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.13) Gecko/20101211 Firefox/3.6.13
Build Identifier: 4.0b8pre (2010-12-13)

When using backspace in an edit field object:text-changed:deleted events are sent via at-spi. These events should contain the text which has been deleted but it appears the wrong text is put in these events. This means when a user of assistive technology (eg. orca screen reader) does not get told what they are deleting.

From my experiments with orca it appears that the text being put in these events is the text at the position once the deleted text has been removed. May be an example, if I had the word "hello" in an edit field and I delete the e by using backspace the event through at-spi reports the deleted text as "l". If I were to highlight "el" in "hello" and delete the selection then the at-spi event reports "lo" as being deleted.

This seems to be common to both firefox and thunderbird using gecko 2.0, in all editable fields I have tried (eg. edit fields on webpages, the message writing area of thunderbird, the search box of thunderbird, etc).

Reproducible: Always

Steps to Reproduce:
1. Navigate to an editable field in firefox or thunderbird (eg. the search all messages field in thunderbird).
2. Write something there.
3. Use something which watches at-spi events (eg. orca screen reader or accerciser) and observe what happens when you delete characters (both in the midst and at the end).
Actual Results:  
The at-spi tool will report the text which moves to the position of the deleted text as being the text which was deleted.

Expected Results:  
The at-spi tool should report the actually deleted text as being deleted.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-12-13 21:37:33 PST
This worked in Gecko 1.9.2, right?
Comment 2 Michael Whapples 2010-12-14 06:52:21 PST
Yes the expected shown behaviour is observed with 1.9.2.
Comment 3 Benjamin Smedberg [:bsmedberg] 2010-12-14 09:42:13 PST
Possibly related to bug 546068.
Comment 4 alexander :surkov 2010-12-14 10:23:13 PST
regression from bug 561932
Comment 5 alexander :surkov 2010-12-14 10:24:26 PST
but in general gecko 2.0 architecture doesn't allow sync text events, that makes atk-bridge return wrong text.
Comment 6 alexander :surkov 2010-12-14 10:41:38 PST
some clarification (from chat with fer): the problem is atk-bridge gets removed text from the start offset and text length passed as event arguments when it handles an text event, at this point the text was removed already and therefore the text is wrong.
Comment 7 alexander :surkov 2010-12-14 10:48:33 PST
I think it's kind of impossible to do a fix on Firefox side. We should fix atk-bridge (Fernando gets progress on this). This makes us incompatible with Gnome 2 but perhaps that's best what we can do.
Comment 8 David Bolter [:davidb] 2010-12-14 12:38:28 PST
OK as per our meeting, Fernando can drive this one.

Next steps:
1. define new signals (atk3, and atk2)
2. drive atk-bridge support (mgorse)
3. add FF4 support (fer)
4. doc what version of FF + atk is broken.
Comment 9 Michael Whapples 2010-12-14 13:42:35 PST
(In reply to comment #8)
> OK as per our meeting, Fernando can drive this one.
> 
> Next steps:
> 1. define new signals (atk3, and atk2)
> 2. drive atk-bridge support (mgorse)
> 3. add FF4 support (fer)
> 4. doc what version of FF + atk is broken.

NOTE: Thunderbird is affected as well, certainly thunderbird 3.3a1 is affected.
Comment 10 alexander :surkov 2010-12-14 15:09:12 PST
(In reply to comment #9)

> > Next steps:
> > 1. define new signals (atk3, and atk2)
> > 2. drive atk-bridge support (mgorse)
> > 3. add FF4 support (fer)
> > 4. doc what version of FF + atk is broken.
> 
> NOTE: Thunderbird is affected as well, certainly thunderbird 3.3a1 is affected.

it should be worded as
> > 3. add Gecko2.0 support (fer)
> > 4. doc what version of FF/Th + atk is broken.
Comment 11 Fernando Herrera 2011-01-05 06:30:58 PST
Here is the atk bug

https://bugzilla.gnome.org/show_bug.cgi?id=638377
Comment 12 Benjamin Smedberg [:bsmedberg] 2011-01-06 11:32:12 PST
Given the scope of this, it doesn't look like it's going to hit the Firefox 4 schedule. Moving to .x per davidb.
Comment 13 Trevor Saunders (:tbsaunde) 2011-01-31 20:21:23 PST
Created attachment 508658 [details] [diff] [review]
Patch using the proposed atk text-deleted and text-inserted signals
Comment 14 alexander :surkov 2011-01-31 20:33:20 PST
Comment on attachment 508658 [details] [diff] [review]
Patch using the proposed atk text-deleted and text-inserted signals

just style issues.

>+    char *newSignalName = g_strconcat(isInserted ? "text_inserted" : "text_delete",
>+                                    isFromUserInput ? "" : kNonUserInputEvent, NULL);

nit: char*
nit: line up isFromUserInput

>+    PRInt32 newSignal = g_signal_lookup(newSignalName, MAI_TYPE_ATK_OBJECT);
>     char *signal_name = g_strconcat(isInserted ? "text_changed::insert" : "text_changed::delete",

nit: char*

>                                     isFromUserInput ? "" : kNonUserInputEvent, NULL);
>+    if (0 != newSignal) {
>+	    nsAutoString text;
>+	    event->GetModifiedText(text);
>+	    NS_ConvertUTF16toUTF8 utf8text(text);
>+	    g_signal_emit(newSignalName, start, length, utf8text.get());

no tabs

>+    } else {
>     g_signal_emit_by_name(aObject, signal_name, start, length);

4 spaces indent
Comment 15 Trevor Saunders (:tbsaunde) 2011-01-31 21:18:56 PST
Created attachment 508667 [details] [diff] [review]
updated patch from surkov's comments
Comment 16 alexander :surkov 2011-01-31 21:40:57 PST
Comment on attachment 508667 [details] [diff] [review]
updated patch from surkov's comments

code and style nits:

>-    char *signal_name = g_strconcat(isInserted ? "text_changed::insert" : "text_changed::delete",
>+    char* newSignalName = g_strconcat(isInserted ? "text_inserted" :
>+	         	              "text_delete",

tabs!

>+                                      isFromUserInput ? "" : kNonUserInputEvent,
>+				      NULL);

tabs!

>+    PRInt32 newSignal = g_signal_lookup(newSignalName, MAI_TYPE_ATK_OBJECT);
>+    char* signal_name = g_strconcat(isInserted ? "text_changed::insert" :
>+	       	                    "text_changed::delete",
>                                     isFromUserInput ? "" : kNonUserInputEvent, NULL);

don't create old signal if new signal will be fired.

btw, I would prefer signal vs obsoleteSignal or something

>-    g_signal_emit_by_name(aObject, signal_name, start, length);
>+    if (0 != newSignal) {

if (newSignal)

let's fix nits before to bother ginn
Comment 17 alexander :surkov 2011-01-31 21:42:59 PST
Fernando, can you confirm that ATK bridge is going to store the passed string or process it synchronously?

NS_ConvertUTF16toUTF8 utf8text(text);
g_signal_emit(newSignalName, start, length, utf8text.get());

otherwise it's crash.
Comment 18 Ginn Chen 2011-01-31 21:59:08 PST
Comment on attachment 508667 [details] [diff] [review]
updated patch from surkov's comments

besides surkov's comments,

g_signal_emit(newSignalName, start, length, utf8text.get());

I think it should be

g_signal_emit(aObject, newSignal, start, length, utf8text.get());

     g_free (signal_name);
+    g_free(newSignalName);

Please change |g_free (signal_name)| to |g_free(signal_name)|.
Comment 19 Fernando Herrera 2011-02-01 04:38:43 PST
(In reply to comment #17)
> Fernando, can you confirm that ATK bridge is going to store the passed string
> or process it synchronously?

g_signal_emit is processed synchronously.
Comment 20 Fernando Herrera 2011-02-02 04:29:38 PST
Comment on attachment 508667 [details] [diff] [review]
updated patch from surkov's comments

>+    PRInt32 newSignal = g_signal_lookup(newSignalName, MAI_TYPE_ATK_OBJECT);

I would use here ATK_TYPE_TEXT, as that is the type with that signal.

>+        g_signal_emit(newSignalName, start, length, utf8text.get());

You are missing the "detail" parameter. It should be:

g_signal_emit(aObject, newSignal, 0, start, length, utf8text.get());
Comment 21 Trevor Saunders (:tbsaunde) 2011-02-08 23:39:52 PST
Created attachment 510960 [details] [diff] [review]
Updated patch from Fernando

Fernando, you said this patches works for you right?  I don't currently have at-spi2 working well enough to test, and I'm not sure when I'll have to make it work so I'll go with you here
Comment 22 alexander :surkov 2011-02-09 00:31:43 PST
Comment on attachment 510960 [details] [diff] [review]
Updated patch from Fernando


>+    PRInt32 newSignal = g_signal_lookup(isInserted ? "text-insert" : "text-remove", ATK_TYPE_TEXT);
>+    if (newSignal) {
>+        nsAutoString text;
>+        event->GetModifiedText(text);
>+        NS_ConvertUTF16toUTF8 utf8text(text);

there's no need to keep utf8text as a local variable.

>+        g_signal_emit(aObject, newSignal,
>+                      isFromUserInput ? 0 : g_quark_from_string("system"), // FIXME: Unify ":system" and "system"

why is it different and who should do unification?

>+                      start, length, g_strdup(utf8text.get()));

why is it necessary to copy the string?
Comment 23 alexander :surkov 2011-02-15 01:20:00 PST
Ginn, ping on this please. We need to land it into FX4, it should be pretty easy patch.
Comment 24 Ginn Chen 2011-02-15 20:43:39 PST
Fernando,
1)
I'd prefer to use g_quark_from_string(":system") in Firefox code.
It looks more natural to me, and you can save a branch in at-spi code, just do
minor = g_strconcat ("delete", minor, NULL);

2)
I didn't see anywhere to free g_strdup(utf8text.get()).
I think probably you don't need to copy the string and let Firefox free the string.
We need to make it clear in AT-SPI document.
Comment 25 Fernando Herrera 2011-02-16 03:57:18 PST
(In reply to comment #24)
> Fernando,
> 1)
> I'd prefer to use g_quark_from_string(":system") in Firefox code.
> It looks more natural to me, and you can save a branch in at-spi code, just do
> minor = g_strconcat ("delete", minor, NULL);

If we use g_signal_emit, we cannot use ":system", as it internally builds the detailed name (name + detail) doing: signalstring + ":" + detailstring, and it would end up as "text-delete::system".

We could use g_signal_lookup just to ensure that the signal exists, discard the signal id, and use g_signal_emit_by_name.


> 2)
> I didn't see anywhere to free g_strdup(utf8text.get()).
> I think probably you don't need to copy the string and let Firefox free the
> string.
> We need to make it clear in AT-SPI document.

Yeah, we don't need it. It was on the original patch somewhere.
Comment 26 Ginn Chen 2011-02-16 17:55:04 PST
OK, so detail should not have "colon".
I see the code in signal_pasrse_name().

I think we can use a global bool for g_signal_lookup("text-insert", ATK_TYPE_TEXT);
Assuming "text-removed" is also there.
And use g_signal_emit_by_name here.

I'm OK with your current implementation, if you don't want to change.


Surkov, I don't think we need this for 4.0.
The GNOME part is not committed yet.
Comment 27 alexander :surkov 2011-02-16 18:39:24 PST
(In reply to comment #26)

> Surkov, I don't think we need this for 4.0.
> The GNOME part is not committed yet.

Do you think the next GNOME (where it's fixed) will definitely use Firefox 5?
Comment 28 Ginn Chen 2011-02-16 21:41:38 PST
(In reply to comment #27)
 
> Do you think the next GNOME (where it's fixed) will definitely use Firefox 5?

Maybe we can fix it in Firefox 4.1 or 4.0.x.
Comment 29 alexander :surkov 2011-02-16 21:43:31 PST
(In reply to comment #28)
> (In reply to comment #27)
> 
> > Do you think the next GNOME (where it's fixed) will definitely use Firefox 5?
> 
> Maybe we can fix it in Firefox 4.1 or 4.0.x.

unusually it's hard to get something into release when it was shipped. I really would like to see this in Fx4 to be on safe side.
Comment 30 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-02-16 22:47:40 PST
The next release is in 3 months, no?
Comment 31 alexander :surkov 2011-02-16 23:21:38 PST
(In reply to comment #30)
> The next release is in 3 months, no?

It's planned, right, we could be faced to delays though. I don't know when next GNOME with the fix is released. But if we take this patch into fx4 then we're about 100% guaranteed the next GNOME with the fix works with Firefox again.
Comment 32 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-02-17 08:24:57 PST
Yes, but then we run the risk of delaying Fx4.

How safe is the patch?  Would you be willing to bet money and/or lives on it?
Comment 33 alexander :surkov 2011-02-17 18:51:39 PST
> How safe is the patch?  Would you be willing to bet money and/or lives on it?

I'm not a player, money bet isn't in my rules and I wouldn't bet lives for anything I do :)

It looks safe with me because all we do is a check of signal existence. But I trust Fernando and Ginn to judge. If risk is not about zero then I'm fine to land it after Fx4.
Comment 34 David Bolter [:davidb] 2011-03-08 08:26:59 PST
(In reply to comment #26)
> OK, so detail should not have "colon".
> I see the code in signal_pasrse_name().
> 
> I think we can use a global bool for g_signal_lookup("text-insert",
> ATK_TYPE_TEXT);
> Assuming "text-removed" is also there.
> And use g_signal_emit_by_name here.
> 
> I'm OK with your current implementation, if you don't want to change.
> 
> 
> Surkov, I don't think we need this for 4.0.
> The GNOME part is not committed yet.

Ginn, can you r+ for FF5 landing?
Comment 35 Ginn Chen 2011-03-08 18:55:17 PST
Comment on attachment 510960 [details] [diff] [review]
Updated patch from Fernando

Canceling review request.

2) in comment #24 should be addressed.

And since we targets FF5, we should also address comment #26.
It would fix 'Unify ":system" and "system"'.
Comment 36 Trevor Saunders (:tbsaunde) 2011-03-24 15:08:05 PDT
Created attachment 521648 [details] [diff] [review]
bug 619002
Comment 37 Fernando Herrera 2011-03-30 04:06:31 PDT
Comment on attachment 521648 [details] [diff] [review]
bug 619002

I cannot find any nice solution for the kNonUserInputEvent vs. "::system" construction, but I think it is not a big deal.

So r=me
Comment 38 Ginn Chen 2011-03-30 18:56:56 PDT
please align the new code with the old code.

haveSignals, please use a more meaningful name.

use NS_ConvertUTF16toUTF8(text).get().

// xxx remove this code when we can stop supporting old atk since it
I don't think we're going to remove it in a near future.
Add this bug number in the comment, use FIXME, TODO, or XXX, not xxx.
Comment 39 Trevor Saunders (:tbsaunde) 2011-04-07 13:33:32 PDT
Created attachment 524469 [details] [diff] [review]
bug 619002
Comment 40 Ginn Chen 2011-04-07 20:59:30 PDT
Comment on attachment 524469 [details] [diff] [review]
bug 619002

r=me

surkov, we prefer "char* " to "char *", right?
Comment 41 alexander :surkov 2011-04-09 16:56:11 PDT
(In reply to comment #40)
> Comment on attachment 524469 [details] [diff] [review]
> bug 619002
> 
> r=me
> 
> surkov, we prefer "char* " to "char *", right?

yes, char*
Comment 42 alexander :surkov 2011-04-09 17:05:09 PDT
Comment on attachment 524469 [details] [diff] [review]
bug 619002


> nsresult
> nsAccessibleWrap::FireAtkTextChangedEvent(AccEvent* aEvent,
>                                           AtkObject *aObject)
> {
>+    static PRBool checkedSignals = PR_FALSE;
>+    static PRBool haveNewTextSignals = PR_FALSE;
>+    if (!checkedSignals) {
>+        haveNewTextSignals = g_signal_lookup("text-insert", ATK_TYPE_TEXT);
>+        checkedSignals = PR_TRUE;
>+    }
>+

I'd prefer to do this check when a11y is initialized (nsAccessNodeWrap::InitAccessibility), having one global allows us to avoid two local statics. Though if Ginn feels that's what we should do then it's fine with me.

>+    char* signal_name;

I prefer to initialize local variables always (especially pointers).

>+
>+    if (haveNewTextSignals) {
>+        nsAutoString text;
>+        event->GetModifiedText(text);
>+        signal_name = g_strconcat(isInserted ? "text-insert" : "text-remove",
>+                                  isFromUserInput ? "" : "::system", NULL);
>+        g_signal_emit_by_name(aObject, signal_name, start, length,
>+                              NS_ConvertUTF16toUTF8(text).get());
>+    } else {
>+        // XXX remove this code when we can stop supporting old atk since it
>+        // doesn't really work anyway see bug 619002

I think the comment should say we should remove also the code that checks "text-insert" signal
Comment 43 Trevor Saunders (:tbsaunde) 2011-04-09 22:35:56 PDT
Created attachment 524909 [details] [diff] [review]
follow up addressing surkovs comments for 619002
Comment 44 Trevor Saunders (:tbsaunde) 2011-04-10 06:16:15 PDT
Created attachment 524947 [details] [diff] [review]
follow up addressing surkovs comments for 619002
Comment 45 Ginn Chen 2011-04-10 19:12:52 PDT
Please use gHaveNewTextSignals.
Comment 46 Trevor Saunders (:tbsaunde) 2011-04-11 16:55:47 PDT
Created attachment 525218 [details] [diff] [review]
follow up addressing surkovs comments for 619002
Comment 47 alexander :surkov 2011-04-11 17:53:33 PDT
Comment on attachment 525218 [details] [diff] [review]
follow up addressing surkovs comments for 619002


> void nsAccessNodeWrap::InitAccessibility()
> {
>   nsAccessNode::InitXPAccessibility();
>+  nsAccessNodeWrap::gHaveNewTextSignals = g_signal_lookup("text-insert", ATK_TYPE_TEXT);

you don't need name specifier here.

> class nsAccessNodeWrap :  public nsAccessNode
> {
> public: // construction, destruction
>     nsAccessNodeWrap(nsIContent *aContent, nsIWeakReference *aShell);
>     virtual ~nsAccessNodeWrap();
> 
>     static void InitAccessibility();
>     static void ShutdownAccessibility();
>+
>+    static PRBool gHaveNewTextSignals;

nit: I would love to see a comment

> 
>-    if (haveNewTextSignals) {
>+    if (nsAccessNodeWrap::gHaveNewTextSignals) {

you don't need name specifier here as well.

>         nsAutoString text;
>         event->GetModifiedText(text);
>         signal_name = g_strconcat(isInserted ? "text-insert" : "text-remove",
>                                   isFromUserInput ? "" : "::system", NULL);
>         g_signal_emit_by_name(aObject, signal_name, start, length,
>                               NS_ConvertUTF16toUTF8(text).get());
>     } else {
>-        // XXX remove this code when we can stop supporting old atk since it
>-        // doesn't really work anyway see bug 619002
>+        // XXX remove this code and the check which signal to use when we can

perhaps add "(gHaveNewTextSignals)" to make clear what check you talk about

>+        // stop supporting old atk since it doesn't really work anyway
>+        // see bug 619002
Comment 48 Trevor Saunders (:tbsaunde) 2011-04-11 18:40:54 PDT
Created attachment 525265 [details] [diff] [review]
follow up addressing surkovs comments for 619002
Comment 49 alexander :surkov 2011-04-11 18:44:19 PDT
Comment on attachment 525265 [details] [diff] [review]
follow up addressing surkovs comments for 619002


>+
>+    // do we have text-remove and text-insert signals if not we need to use
>+    // text-changed see nsAccessibleWrap::FireAtkTextChangedEvent() and
>+    // bug 619002
>+    static PRBool gHaveNewTextSignals;

for comments in header the /* comment style */ is used

please upload "patch to land"
Comment 50 Trevor Saunders (:tbsaunde) 2011-04-11 19:12:12 PDT
Created attachment 525272 [details] [diff] [review]
patch to land

patch to land
Comment 51 alexander :surkov 2011-04-12 01:04:20 PDT
landed on cedar - http://hg.mozilla.org/projects/cedar/rev/19edf1831109
Comment 52 Marco Zehe (:MarcoZ) 2011-04-12 03:46:51 PDT
Please do not check this into Firefox 5 since I saw a crash with the try-server build that contained this patch, when entering certain textareas. It does not happen if I run a build that does not contain this patch.
Comment 53 alexander :surkov 2011-04-12 04:10:04 PDT
(In reply to comment #52)
> Please do not check this into Firefox 5 since I saw a crash with the try-server
> build that contained this patch, when entering certain textareas. It does not
> happen if I run a build that does not contain this patch.

Obviously you talk about Linux/Orca crash, right?
Comment 54 alexander :surkov 2011-04-12 04:14:41 PDT
(In reply to comment #52)
> Please do not check this into Firefox 5

Is it ok to keep Firefox 5 incompatible with Gnome 3? We should be mindful by taking decisions like this.
Comment 55 Fernando Herrera 2011-04-12 05:42:32 PDT
(In reply to comment #54)
> Is it ok to keep Firefox 5 incompatible with Gnome 3? We should be mindful by
> taking decisions like this.

Incompatible how? I don't see how this patch affect compatibility with GNOME 3. Situation without (ff4) and with (ff5) this patch is:

firefox 4 + GNOME 2 = wrong deleted text reported
firefox 4 + GNOME 3 = wrong deleted text reported
firefox 5 + GNOME 2 = wrong deleted text reported
firefox 5 + GNOME 3 = proper deleted text reported

there has been some discussion about backporting the new signal also to GNOME 2 stable release so ff5 + latest gnome 2 would work fine too.
Comment 56 Marco Zehe (:MarcoZ) 2011-04-12 06:01:36 PDT
(In reply to comment #53)
> Obviously you talk about Linux/Orca crash, right?

No, this crash happens on Windows, and only in that try-server build, not with a build I made that only contains the fix for bug 648988. So this patch clearly affects Windows, too, probably because of platform-independent code changes.
Comment 57 Fernando Herrera 2011-04-12 06:06:48 PDT
(In reply to comment #56)
> No, this crash happens on Windows, and only in that try-server build, not with
> a build I made that only contains the fix for bug 648988. So this patch clearly
> affects Windows, too, probably because of platform-independent code changes.

This patch does not touch anything outside accessible/src/atk
Comment 58 Marco Zehe (:MarcoZ) 2011-04-12 08:34:37 PDT
Trevor, thanks for the clarification. I'm convinced that this can't be responsible for the crash i saw. OK, this is OK to land on m-c, and possibly FX5 to ensure Gnome 3 compatibility.
Comment 59 Marco Zehe (:MarcoZ) 2011-04-12 08:50:17 PDT
Christian, this patch made it onto Cedar before the Aurora merge, but not onto m-c yet, but would be important to have for Gnome 3 compatibility in accessibility. Is there a chance we can still port this into Aurora, or has that train left?
Comment 60 :Ehsan Akhgari (out sick) 2011-04-12 17:47:17 PDT
(In reply to comment #59)
> Christian, this patch made it onto Cedar before the Aurora merge, but not onto
> m-c yet, but would be important to have for Gnome 3 compatibility in
> accessibility. Is there a chance we can still port this into Aurora, or has
> that train left?

Sorry, the train has left the station.  But you'll be on the next one.  :-)
Comment 61 :Ehsan Akhgari (out sick) 2011-04-13 11:16:51 PDT
http://hg.mozilla.org/mozilla-central/rev/19edf1831109
Comment 62 Michael Whapples 2011-04-13 12:44:48 PDT
As the original bug was observed on thunderbird as well, where does this fit in with inclusion into thunderbird?
Comment 63 Fernando Herrera 2011-06-07 11:16:13 PDT
It looks like this is not the proper check for the signal. When using at-spi2 and a libatk new enough to contain that signal I see this:

(firefox-bin:22234): GLib-GObject-WARNING **: gsignal.c:1149: unable to lookup signal "text-insert" for non instantiatable type `AtkText'

One explanation of that:
https://mail.gnome.org/archives/gtk-devel-list/2001-October/msg00289.html
Comment 64 Fernando Herrera 2011-06-15 07:21:21 PDT
Trevor, do you want me to prepare a simple glib test program to check for the signal?
Comment 65 Trevor Saunders (:tbsaunde) 2011-06-15 22:45:26 PDT
Created attachment 539726 [details] [diff] [review]
test idea

I'm not sure if refing an interface like a class works for glib or not.  THe problem is that if we keep the feature check where it is I'm not sure what instantiatable type we can use here that we know inherits from atk text.
Comment 66 Fernando Herrera 2011-07-04 06:30:11 PDT
We cannot g_type_class_ref with an interface. I get this error:

(<unknown>:28639): GLib-GObject-WARNING **: cannot retrieve class for invalid (unclassed) type `AtkText'
Comment 67 Trevor Saunders (:tbsaunde) 2011-07-04 06:34:28 PDT
(In reply to comment #66)
> We cannot g_type_class_ref with an interface. I get this error:
> 
> (<unknown>:28639): GLib-GObject-WARNING **: cannot retrieve class for
> invalid (unclassed) type `AtkText'

YEAH, i SPENT SOME TIME ON THIS WEEKEND, i THINK OUR BEST OPTION IS GOING BACK TO CHECKING ON THE FIRST TIME THE SIGNAL GETS FIRED THAT WAY THE TYPE HAS BEEN USED.  i HAVE A PATCH THAT DOES THAT, i'LL TAKE A LAST LOOK MYSELF THEN ATTACH IT
Comment 68 Trevor Saunders (:tbsaunde) 2011-07-04 06:54:53 PDT
Created attachment 543764 [details] [diff] [review]
patch

go back to checking on the first text changed event, also use an enum to store the state instead of a bool.  This works for me.
Comment 69 Fernando Herrera 2011-07-04 08:29:07 PDT
I have just tested last patch and it works nicely. Great work!
Comment 70 Fernando Herrera 2011-07-04 09:49:05 PDT
Comment on attachment 543764 [details] [diff] [review]
patch

I know that Alex preferred this check to be done when a11y is being initialized (comment 42), but at that point we don't have any instance of any AtkText based object, so it fails and this looks like the only way to do it.

So the patch looks ok to me. Just make it to use 4 spaces instead of 2 to match the current style. r=me
Comment 71 Trevor Saunders (:tbsaunde) 2011-07-04 20:19:07 PDT
> So the patch looks ok to me. Just make it to use 4 spaces instead of 2 to
> match the current style. r=me

actually, changing the indentation style is intentional, on the way to changing all the style to 2 spaces.  see bug 657719 comment 36
Comment 72 Ginn Chen 2011-07-04 22:06:14 PDT
*** Bug 669254 has been marked as a duplicate of this bug. ***
Comment 73 Ginn Chen 2011-07-04 22:21:55 PDT
Fernando,

I think we should use
g_signal_lookup("text-insert", G_OBJECT_TYPE(aObject))
instead of
gAvailableAtkSignals = g_signal_lookup("text-insert", ATK_TYPE_TEXT)

The aObject should have ATK_TYPE_TEXT interface.

Does it work for you?
Comment 74 Ginn Chen 2011-07-04 22:26:38 PDT
BTW: I still have the warning after applying attachment 543764 [details] [diff] [review].
Comment 75 Trevor Saunders (:tbsaunde) 2011-07-05 07:16:32 PDT
> I think we should use
> g_signal_lookup("text-insert", G_OBJECT_TYPE(aObject))
> instead of
> gAvailableAtkSignals = g_signal_lookup("text-insert", ATK_TYPE_TEXT)

why? what do you think that gets us?  so far as I can see that's longer and may require more run time computation.

> The aObject should have ATK_TYPE_TEXT interface.

agree
Comment 76 Trevor Saunders (:tbsaunde) 2011-07-05 07:23:18 PDT
(In reply to comment #74)
> BTW: I still have the warning after applying attachment 543764 [details] [diff] [review]
> [review].

I assume this is with atk < 1.32? I don't see it here with atk 2.0 and todays nightly.   If so I suspect its just because the signal actually isn't available and gobject assumes you   are looking up a signal to get its id not see  if its available.  In that case I don't think there's much we can do about the warning which is a little suboptimal.
Comment 77 Ginn Chen 2011-07-05 21:06:06 PDT
(In reply to comment #75)
> > I think we should use
> > g_signal_lookup("text-insert", G_OBJECT_TYPE(aObject))
> > instead of
> > gAvailableAtkSignals = g_signal_lookup("text-insert", ATK_TYPE_TEXT)
> 
> why? what do you think that gets us?  so far as I can see that's longer and
> may require more run time computation.
> 

It will not print the warning with atk < 1.32.
Since we only do it for one single time, we don't care about time computation.
Comment 78 Ginn Chen 2011-07-20 06:37:55 PDT
Trevor, friendly poke :)
Comment 79 Trevor Saunders (:tbsaunde) 2011-07-22 19:42:50 PDT
(In reply to comment #73)
> Fernando,
> 
> I think we should use
> g_signal_lookup("text-insert", G_OBJECT_TYPE(aObject))
> instead of
> gAvailableAtkSignals = g_signal_lookup("text-insert", ATK_TYPE_TEXT)
> 
> The aObject should have ATK_TYPE_TEXT interface.
> 
> Does it work for you?

if you've tested, and 1 we still fire text-insert / remove with atk > 1.32 and you don't have the warning yes.  I would expect that we still  fire the right events with atk > 1.32, but I'd like to be sure that in practice we do actually fire on something implemnting atk text the first time.
Comment 80 Ginn Chen 2011-12-12 03:17:06 PST
Created attachment 580869 [details] [diff] [review]
remove the warning with atk < 1.32

tested on Ubuntu 11.10, works as expected
tested on Solaris 11, warning is gone
Comment 81 Trevor Saunders (:tbsaunde) 2011-12-12 03:30:21 PST
Comment on attachment 580869 [details] [diff] [review]
remove the warning with atk < 1.32

r=tbsaunde
Comment 82 Ginn Chen 2011-12-21 03:31:08 PST
Comment on attachment 580869 [details] [diff] [review]
remove the warning with atk < 1.32

Remove the warning message with atk < 1.32

http://hg.mozilla.org/integration/mozilla-inbound/rev/f154f18b320a
Comment 83 Ed Morley [:emorley] 2011-12-21 12:54:47 PST
https://hg.mozilla.org/mozilla-central/rev/f154f18b320a

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