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

RESOLVED FIXED in mozilla7

Status

()

Core
Disability Access APIs
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Michael Whapples, Assigned: tbsaunde)

Tracking

(Blocks: 1 bug, {access, regression})

Trunk
mozilla7
x86
All
access, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 .x+, status2.0 wanted)

Details

(Whiteboard: [has patch][FF5])

Attachments

(4 attachments, 9 obsolete attachments)

3.11 KB, patch
Details | Diff | Splinter Review
709 bytes, patch
Details | Diff | Splinter Review
6.13 KB, patch
davidb
: review+
Fernando Herrera
: review+
Details | Diff | Splinter Review
1.19 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Keywords: access
Version: unspecified → Trunk

Comment 1

7 years ago
This worked in Gecko 1.9.2, right?
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Component: XUL → Editor
Ever confirmed: true
Keywords: regression
QA Contact: xptoolkit.widgets → editor
(Reporter)

Comment 2

7 years ago
Yes the expected shown behaviour is observed with 1.9.2.
Assignee: nobody → ehsan

Updated

7 years ago
blocking2.0: ? → betaN+
Possibly related to bug 546068.

Updated

7 years ago
blocking2.0: betaN+ → ?

Updated

7 years ago
blocking2.0: ? → betaN+

Comment 4

7 years ago
regression from bug 561932

Updated

7 years ago
Blocks: 561932

Comment 5

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

Updated

7 years ago
Assignee: ehsan → nobody
Component: Editor → Disability Access APIs
QA Contact: editor → accessibility-apis

Comment 6

7 years ago
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

7 years ago
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.
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.
Assignee: nobody → fherrera
(Reporter)

Comment 9

7 years ago
(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

7 years ago
(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

7 years ago
Here is the atk bug

https://bugzilla.gnome.org/show_bug.cgi?id=638377
Given the scope of this, it doesn't look like it's going to hit the Firefox 4 schedule. Moving to .x per davidb.
blocking2.0: betaN+ → .x

Updated

7 years ago
status2.0: --- → wanted
(Assignee)

Comment 13

7 years ago
Created attachment 508658 [details] [diff] [review]
Patch using the proposed atk text-deleted and text-inserted signals
Attachment #508658 - Flags: review?(fherrera)
Attachment #508658 - Flags: feedback?

Comment 14

7 years ago
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
(Assignee)

Comment 15

7 years ago
Created attachment 508667 [details] [diff] [review]
updated patch from surkov's comments
Attachment #508658 - Attachment is obsolete: true
Attachment #508667 - Flags: review?
Attachment #508658 - Flags: review?(fherrera)
Attachment #508658 - Flags: feedback?

Updated

7 years ago
Attachment #508667 - Flags: review? → review?(ginn.chen)

Comment 16

7 years ago
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
Attachment #508667 - Flags: review?(ginn.chen)

Comment 17

7 years ago
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

7 years ago
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

7 years ago
(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.
OS: Linux → Windows CE

Comment 20

7 years ago
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());
(Assignee)

Comment 21

7 years ago
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
Attachment #508667 - Attachment is obsolete: true
Attachment #510960 - Flags: review?(surkov.alexander)
Attachment #510960 - Flags: feedback?

Updated

7 years ago
Assignee: fherrera → trev.saunders

Comment 22

7 years ago
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?
Attachment #510960 - Flags: review?(surkov.alexander)
Attachment #510960 - Flags: review?(ginn.chen)
Attachment #510960 - Flags: review+
Attachment #510960 - Flags: feedback?(fherrera)
Attachment #510960 - Flags: feedback?

Comment 23

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

Comment 24

7 years ago
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.
OS: Windows CE → Linux

Comment 25

7 years ago
(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.
OS: Linux → All

Comment 26

7 years ago
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

7 years ago
(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

7 years ago
(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

7 years ago
(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

7 years ago
The next release is in 3 months, no?

Comment 31

7 years ago
(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

7 years ago
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

7 years ago
> 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.
(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?

Updated

7 years ago
Whiteboard: [FF5]

Comment 35

7 years ago
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"'.
Attachment #510960 - Flags: review?(ginn.chen)

Updated

7 years ago
Whiteboard: [FF5] → [has patch][FF5]

Updated

7 years ago
Blocks: 632301
(Assignee)

Comment 36

6 years ago
Created attachment 521648 [details] [diff] [review]
bug 619002
Attachment #510960 - Attachment is obsolete: true
Attachment #521648 - Flags: review?(fherrera)
Attachment #510960 - Flags: feedback?(fherrera)

Comment 37

6 years ago
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
Attachment #521648 - Flags: superreview?(ginn.chen)
Attachment #521648 - Flags: review?(fherrera)
Attachment #521648 - Flags: review+

Comment 38

6 years ago
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.
(Assignee)

Comment 39

6 years ago
Created attachment 524469 [details] [diff] [review]
bug 619002

Updated

6 years ago
Attachment #521648 - Flags: superreview?(ginn.chen)

Comment 40

6 years ago
Comment on attachment 524469 [details] [diff] [review]
bug 619002

r=me

surkov, we prefer "char* " to "char *", right?
Attachment #524469 - Flags: review+
Attachment #524469 - Flags: feedback?(surkov.alexander)

Comment 41

6 years ago
(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

6 years ago
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
Attachment #524469 - Flags: feedback?(surkov.alexander) → feedback+
(Assignee)

Comment 43

6 years ago
Created attachment 524909 [details] [diff] [review]
follow up addressing surkovs comments for 619002
(Assignee)

Comment 44

6 years ago
Created attachment 524947 [details] [diff] [review]
follow up addressing surkovs comments for 619002
Attachment #524909 - Attachment is obsolete: true

Comment 45

6 years ago
Please use gHaveNewTextSignals.
(Assignee)

Comment 46

6 years ago
Created attachment 525218 [details] [diff] [review]
follow up addressing surkovs comments for 619002
Attachment #524947 - Attachment is obsolete: true

Comment 47

6 years ago
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
(Assignee)

Comment 48

6 years ago
Created attachment 525265 [details] [diff] [review]
follow up addressing surkovs comments for 619002
Attachment #525218 - Attachment is obsolete: true

Comment 49

6 years ago
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"
(Assignee)

Comment 50

6 years ago
Created attachment 525272 [details] [diff] [review]
patch to land

patch to land
Attachment #521648 - Attachment is obsolete: true
Attachment #524469 - Attachment is obsolete: true
Attachment #525265 - Attachment is obsolete: true

Comment 51

6 years ago
landed on cedar - http://hg.mozilla.org/projects/cedar/rev/19edf1831109
Whiteboard: [has patch][FF5] → [has patch][FF5][fixed-in-cedar]
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

6 years ago
(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

6 years ago
(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

6 years ago
(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.
(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

6 years ago
(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
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.
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?

Updated

6 years ago
tracking-firefox5: --- → ?

Updated

6 years ago
tracking-firefox5: ? → ---
(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.  :-)
http://hg.mozilla.org/mozilla-central/rev/19edf1831109
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][FF5][fixed-in-cedar] → [has patch][FF5]
Target Milestone: --- → mozilla6
(Reporter)

Comment 62

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

Comment 63

6 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 64

6 years ago
Trevor, do you want me to prepare a simple glib test program to check for the signal?
(Assignee)

Comment 65

6 years ago
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

6 years ago
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'
(Assignee)

Comment 67

6 years ago
(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
(Assignee)

Comment 68

6 years ago
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

6 years ago
I have just tested last patch and it works nicely. Great work!
(Assignee)

Updated

6 years ago
Attachment #543764 - Flags: review?(bolterbugz)

Updated

6 years ago
Attachment #543764 - Flags: review?(fherrera)
Attachment #543764 - Flags: review?(bolterbugz)
Attachment #543764 - Flags: review+

Comment 70

6 years ago
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
Attachment #543764 - Flags: review?(fherrera) → review+
(Assignee)

Comment 71

6 years ago
> 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

Updated

6 years ago
Depends on: 669254

Updated

6 years ago
No longer depends on: 669254
Duplicate of this bug: 669254

Comment 73

6 years ago
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

6 years ago
BTW: I still have the warning after applying attachment 543764 [details] [diff] [review].
(Assignee)

Comment 75

6 years ago
> 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
(Assignee)

Comment 76

6 years ago
(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

6 years ago
(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

6 years ago
Trevor, friendly poke :)
(Assignee)

Comment 79

6 years ago
(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.
(Assignee)

Updated

6 years ago
Target Milestone: mozilla6 → mozilla7

Comment 80

6 years ago
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
Attachment #580869 - Flags: review?(trev.saunders)
(Assignee)

Comment 81

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

r=tbsaunde
Attachment #580869 - Flags: review?(trev.saunders) → review+

Comment 82

6 years ago
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
Attachment #580869 - Attachment description: remove the warning with trev.saunders@gmail.com → remove the warning with atk < 1.32
https://hg.mozilla.org/mozilla-central/rev/f154f18b320a
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.