619002 - When deleting text from edit fields the wrong text is reported through at-spi

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Michael Whapples]

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 [:bzbarsky]]

This worked in Gecko 1.9.2, right?

Comment 2

[Michael Whapples]

Yes the expected shown behaviour is observed with 1.9.2.

Comment 3

[Benjamin Smedberg]

Possibly related to bug 546068.

Comment 4

[alexander :surkov (:asurkov)]

regression from bug 561932

Comment 5

[alexander :surkov (:asurkov)]

but in general gecko 2.0 architecture doesn't allow sync text events, that makes atk-bridge return wrong text.

Comment 6

[alexander :surkov (:asurkov)]

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 (:asurkov)]

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] (NeedInfo me for attention)]

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]

(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 (:asurkov)]

(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]

Here is the atk bug

https://bugzilla.gnome.org/show_bug.cgi?id=638377

Comment 12

[Benjamin Smedberg]

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)]

Attachment: Patch using the proposed atk text-deleted and text-inserted signals

Comment 14

[alexander :surkov (:asurkov)]

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)]

Attachment: updated patch from surkov's comments

Comment 16

[alexander :surkov (:asurkov)]

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 (:asurkov)]

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]

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]

(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]

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)]

Attachment: 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 (:asurkov)]

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 (:asurkov)]

Ginn, ping on this please. We need to land it into FX4, it should be pretty easy patch.

Comment 24

[Ginn Chen]

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]

(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]

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 (:asurkov)]

(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]

(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 (:asurkov)]

(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 [:bzbarsky]]

The next release is in 3 months, no?

Comment 31

[alexander :surkov (:asurkov)]

(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 [:bzbarsky]]

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 (:asurkov)]

> 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] (NeedInfo me for attention)]

(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]

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)]

Attachment: bug 619002

Comment 37

[Fernando Herrera]

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]

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)]

Attachment: bug 619002

Comment 40

[Ginn Chen]

Comment on attachment 524469 [details] [diff] [review]
bug 619002

r=me

surkov, we prefer "char* " to "char *", right?

Comment 41

[alexander :surkov (:asurkov)]

(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 (:asurkov)]

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)]

Attachment: follow up addressing surkovs comments for 619002

Comment 44

[Trevor Saunders (:tbsaunde)]

Attachment: follow up addressing surkovs comments for 619002

Comment 45

[Ginn Chen]

Please use gHaveNewTextSignals.

Comment 46

[Trevor Saunders (:tbsaunde)]

Attachment: follow up addressing surkovs comments for 619002

Comment 47

[alexander :surkov (:asurkov)]

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)]

Attachment: follow up addressing surkovs comments for 619002

Comment 49

[alexander :surkov (:asurkov)]

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)]

Attachment: patch to land

patch to land

Comment 51

[alexander :surkov (:asurkov)]

landed on cedar - http://hg.mozilla.org/projects/cedar/rev/19edf1831109

Comment 52

[Marco Zehe (:MarcoZ)]

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 (:asurkov)]

(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 (:asurkov)]

(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]

(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)]

(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]

(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)]

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)]

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

[(no longer active)]

(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

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/19edf1831109

Comment 62

[Michael Whapples]

As the original bug was observed on thunderbird as well, where does this fit in with inclusion into thunderbird?

Comment 63

[Fernando Herrera]

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]

Trevor, do you want me to prepare a simple glib test program to check for the signal?

Comment 65

[Trevor Saunders (:tbsaunde)]

Attachment: 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]

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)]

(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)]

Attachment: 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]

I have just tested last patch and it works nicely. Great work!

Comment 70

[Fernando Herrera]

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)]

> 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 73

[Ginn Chen]

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]

BTW: I still have the warning after applying attachment 543764 [details] [diff] [review].

Comment 75

[Trevor Saunders (:tbsaunde)]

> 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)]

(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]

(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]

Trevor, friendly poke :)

Comment 79

[Trevor Saunders (:tbsaunde)]

(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]

Attachment: 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)]

Comment on attachment 580869 [details] [diff] [review]
remove the warning with atk < 1.32

r=tbsaunde

Comment 82

[Ginn Chen]

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]]

https://hg.mozilla.org/mozilla-central/rev/f154f18b320a