631026 - text_changed signal emitted for wrong object

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Fernando Herrera]

When running ff4 with an at-spi2 bridge I get a lot of this warnings:
GLib-GObject-WARNING **: gsignal.c:3081: signal name `text_changed::delete:system' is invalid for instance `0xa9ea2c70'

Those happens for text deletes and insertions.

To reproduce, just go to the location bar and type something. The typed text will appear with characters in wrong positions.

setting a breakpoint on when those warning happens and getting a trace:

#5  0x00a00521 in g_signal_emit_by_name (instance=0xa9ea2c70, detailed_signal=0xa371d400 "text_changed::delete:system") at gsignal.c:3081
#6  0x015ec923 in nsAccessibleWrap::FireAtkTextChangedEvent (this=0xa8b05f00, aEvent=0xa374cb80, aObject=0xa9ea2c70 [MaiAtkType201]) at /home/fer/code/mozilla/accessible/src/atk/nsAccessibleWrap.cpp:1366
#7  0x015eb465 in nsAccessibleWrap::FirePlatformEvent (this=0xa8b05f00, aEvent=0xa374cb80) at /home/fer/code/mozilla/accessible/src/atk/nsAccessibleWrap.cpp:1082
#8  0x015eb305 in nsAccessibleWrap::HandleAccEvent (this=0xa8b05f00, aEvent=0xa374cb80) at /home/fer/code/mozilla/accessible/src/atk/nsAccessibleWrap.cpp:1049
#9  0x015b97c9 in nsEventShell::FireEvent (aEvent=0xa374cb80) at /home/fer/code/mozilla/accessible/src/base/nsEventShell.cpp:63
#10 0x0158c780 in nsDocAccessible::ProcessPendingEvent (this=0xa8b05f00, aEvent=0xa374cb80) at /home/fer/code/mozilla/accessible/src/base/nsDocAccessible.cpp:1714


and the most relevant thing that at nsAccessibleWrap::FirePlatformEvent, atkObj is:

$24 = {
  parent = {
    g_type_instance = {
      g_class = 0xa79993a0
    }, 
    ref_count = 2, 
    qdata = 0x0
  }, 
  description = 0x0, 
  name = 0xa9ea2d00 "Minefield Start Page - Minefield", 
  accessible_parent = 0xa9ddfb80 [MaiAtkObject], 
  role = ATK_ROLE_FRAME, 
  relation_set = 0xa9e90e60 [AtkRelationSet], 
  layer = ATK_LAYER_INVALID
}

so it looks like we are tying to fire a text change event on the top frame.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

What is the regression range?

Comment 2

[Fernando Herrera]

This is the first nightly build showing this behavoiur:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2011-01-28-03-mozilla-central/

Comment 3

[alexander :surkov (:asurkov)]

Fernando, can you take this bug and investigate it?

Comment 4

[Fernando Herrera]

Taking it.

Comment 5

[Fernando Herrera]

what happens here is that we are sending text changed events for the XUL nsDocAccessible.  XUL documents don't support hypertext accessible interface, so we need to fix AsHyperText/IsHyperText.

Also we should make QueryInterface of doc accessible and atk nsAccessibleWrap::CreateMaiInterfaces to use IsHyperText().

Comment 6

[Fernando Herrera]

Attachment: patch clearing mFlags for XUL nsDocAccessible and using IsHyperText in atk

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 510269 [details] [diff] [review]
patch clearing mFlags for XUL nsDocAccessible and using IsHyperText in atk

I wonder if the fix can go higher up. Like check in CreateHyperTextAccessible and call nsAccessibilityService::CreateAccessibleByType if XUL? That seems hacky still though. Checking for xul in the calls from layout might be another option -- Alexander would know best.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> Also we should make QueryInterface of doc accessible and atk
> nsAccessibleWrap::CreateMaiInterfaces to use IsHyperText().

You forgot to fix QueryInterface of document accessible.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Comment on attachment 510269 [details] [diff] [review]
> patch clearing mFlags for XUL nsDocAccessible and using IsHyperText in atk
> 
> I wonder if the fix can go higher up. Like check in CreateHyperTextAccessible
> and call nsAccessibilityService::CreateAccessibleByType if XUL?

David, I don't understand your suggestion. We keep nsDocAccessible for XUL and HTML DOM documents, the document accessible for XUL DOM document shouldn't be expose nshypertext interface. We could do architecture changes (to keep XUL and HTML document accessible classes separately). But I miss how this can be related to hypertext accessible creation.

> That seems
> hacky still though. Checking for xul in the calls from layout might be another
> option -- Alexander would know best.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 510269 [details] [diff] [review]
patch clearing mFlags for XUL nsDocAccessible and using IsHyperText in atk

canceling review per comment #8

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #9)
> (In reply to comment #7)
> > Comment on attachment 510269 [details] [diff] [review]
> > patch clearing mFlags for XUL nsDocAccessible and using IsHyperText in atk
> > 
> > I wonder if the fix can go higher up. Like check in CreateHyperTextAccessible
> > and call nsAccessibilityService::CreateAccessibleByType if XUL?
> 
> David, I don't understand your suggestion.

Sorry about that. Please disregard; my mind saw nsAccessible instead of nsDocAccessible (sigh).

Comment 13

[Fernando Herrera]

(In reply to comment #8)
> (In reply to comment #5)
> > Also we should make QueryInterface of doc accessible and atk
> > nsAccessibleWrap::CreateMaiInterfaces to use IsHyperText().
> 
> You forgot to fix QueryInterface of document accessible.

by fixing QueryInterface you mean something like this?

--- a/accessible/src/base/nsDocAccessible.cpp
+++ b/accessible/src/base/nsDocAccessible.cpp
@@ -169,17 +169,17 @@ NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_
 
   nsresult status;
   if (!foundInterface) {
     // HTML document accessible must inherit from nsHyperTextAccessible to get
     // support text interfaces. XUL document accessible doesn't need this.
     // However at some point we may push <body> to implement the interfaces and
     // return nsDocAccessible to inherit from nsAccessibleWrap.
 
-    if (mDocument && mDocument->IsXUL())
+    if (!IsHyperText())
       status = nsAccessible::QueryInterface(aIID, (void**)&foundInterface);
     else
       status = nsHyperTextAccessible::QueryInterface(aIID,

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> (In reply to comment #8)
> > (In reply to comment #5)
> > > Also we should make QueryInterface of doc accessible and atk
> > > nsAccessibleWrap::CreateMaiInterfaces to use IsHyperText().
> > 
> > You forgot to fix QueryInterface of document accessible.
> 
> by fixing QueryInterface you mean something like this?

yes

> -    if (mDocument && mDocument->IsXUL())
> +    if (!IsHyperText())
>        status = nsAccessible::QueryInterface(aIID, (void**)&foundInterface);
>      else
>        status = nsHyperTextAccessible::QueryInterface(aIID,

though I would invert this like
if (IsHyperText())
  status = nsHyperText...
else
  status = 

or 

status = IsHyperText() ?
  nsHyperTextAccessible::QueryInterface(aIID, :
  nsAccessible::QueryInterface(aIID, (void**)&foundInterface);

Comment 15

[Fernando Herrera]

Attachment: Updated patch

Comment 16

[Fernando Herrera]

try build request:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=ffcf421a0dad

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 511789 [details] [diff] [review]
Updated patch

r=me

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 511789 [details] [diff] [review]
Updated patch

a=me

Comment 19

[alexander :surkov (:asurkov)]

landed on 2.0 (fx4b12) - http://hg.mozilla.org/mozilla-central/rev/9ea616f391af