725644 - make outputTree of Accessible Events view working for hide events

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[alexander :surkov (:asurkov)]

the problem when we receive hide event the target accessible is not in document tree so outputTree fails to show it. Internally hide event contains information about old parent, so 
1) make this information exposed through XPCOM (add nsIAccessibleHideEvent)
2) use new interface to show hidden accessible in the tree (gray color can be used)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: a11y part

Comment 2

[alexander :surkov (:asurkov)]

Attachment: DOMi part

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 595773 [details] [diff] [review]
a11y part

>+{
>+  nsAccEvent* event = new nsAccHideEvent(this);
>+  NS_ADDREF(event);
>+  return event;

I think nsRefPtr<nsAccHideEvent> = new blah
return event.forget();

works too, but this is fine.

>+nsAccHideEvent::GetTargetParent(nsIAccessible** aAccessible)
>+{
>+  NS_ENSURE_ARG_POINTER(aAccessible);
>+  *aAccessible = nsnull;
>+
>+  NS_IF_ADDREF(*aAccessible =
>+    static_cast<AccHideEvent*>(mEvent.get())->TargetParent());

the first assignment is useless, but any optimizer should get rid of it so I don't really care

>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsAccHideEvent::GetTargetNextSibling(nsIAccessible** aAccessible)
>+{
>+  NS_ENSURE_ARG_POINTER(aAccessible);
>+  *aAccessible = nsnull;

same for the rest of these

I think we should test stuff now that js can see it, but making that a good first bug doesn't seem unreasonable to me.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Comment on attachment 595773 [details] [diff] [review]
> a11y part
> 
> >+{
> >+  nsAccEvent* event = new nsAccHideEvent(this);
> >+  NS_ADDREF(event);
> >+  return event;
> 
> I think nsRefPtr<nsAccHideEvent> = new blah
> return event.forget();
> 
> works too, but this is fine.

no sure I like to create extra object when it's not necessary.

> >+nsAccHideEvent::GetTargetParent(nsIAccessible** aAccessible)
> >+{
> >+  NS_ENSURE_ARG_POINTER(aAccessible);
> >+  *aAccessible = nsnull;
> >+
> >+  NS_IF_ADDREF(*aAccessible =
> >+    static_cast<AccHideEvent*>(mEvent.get())->TargetParent());
> 
> the first assignment is useless, but any optimizer should get rid of it so I
> don't really care

> >+  return NS_OK;
> >+}
> >+
> >+NS_IMETHODIMP
> >+nsAccHideEvent::GetTargetNextSibling(nsIAccessible** aAccessible)
> >+{
> >+  NS_ENSURE_ARG_POINTER(aAccessible);
> >+  *aAccessible = nsnull;
> 
> same for the rest of these

I'll get rid these

> I think we should test stuff now that js can see it, but making that a good
> first bug doesn't seem unreasonable to me.

I agree nice to have

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > Comment on attachment 595773 [details] [diff] [review]
> > a11y part
> > 
> > >+{
> > >+  nsAccEvent* event = new nsAccHideEvent(this);
> > >+  NS_ADDREF(event);
> > >+  return event;
> > 
> > I think nsRefPtr<nsAccHideEvent> = new blah
> > return event.forget();
> > 
> > works too, but this is fine.
> 
> no sure I like to create extra object when it's not necessary.

ok, though I think inlining will end up making them the same code, but if you like the way it looks that's fine by me.

Comment 6

[alexander :surkov (:asurkov)]

inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/3f6d5d9bac61

please do not close the bug

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/3f6d5d9bac61

Comment 8

[[e-mail]]

Sorry for taking so long to get around to trying this out, but what exactly am I looking for here? (I'm not even sure which panel I should look at.)

Comment 9

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #8)
> Sorry for taking so long to get around to trying this out, but what exactly
> am I looking for here? (I'm not even sure which panel I should look at.)

1) switch to Accessible Events view of left panel
2) switch to Watched Events tab
3) locate hide event (under mutation event section) and add custom handler:
outputTree(target.document, [target]);
4) then you should trigger hide events somehow (you could inspect Firefox UI and do something like enter/remove text from addressbar, usually Firefox UI produces a lot of mutation changes)
5) select handled hide event (Event list tab) and switch to Accessible Event view of right panel, you should see the accessible tree snapshot and hide event target should be presented in the tree and marked by gray color.

but if you are going to try then it can be a problem because this patch is based on bug 727380, bug 725573 and bug 725575 (first two bugs waits for your review). I'd say it makes sense to review these bugs first, then I land them and you can try this bug out.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: DOMi part (patch2)

updated, still based on bug 658776 patch which waits for Colby review.

Comment 11

[[e-mail]]

Well, there's definitely a bug in the code somewhere. I used this handler:

if (event instanceof Components.interfaces.nsIAccessibleHideEvent) outputTree(target.document, [target, event.targetParent, event.targetPrevSibling, event.targetNextSibling]);

When you load a page, SeaMonkey temporarily turns its progress bar on and off. This generates appropriate show and hide events. When you show the hide event, the accessible event appears after its next sibling instead of between...

Comment 12

[[e-mail]]

Oh, and the simple expedient of maximising the window hides the resizer, which triggers a hide event on the last accessible of the status bar, but the output actually shows the resizer as being the second last accessible. It looks as if you're setting insertBeforeAttachedSibling in the wrong place.

Comment 13

[[e-mail]]

Comment on attachment 598905 [details] [diff] [review]
DOMi part (patch2)

>+  var attachedSibling = null, insertBeforeAttachedSibling = false;
Nit: separate lines please.

>+  if ("nsIAccessibleHideEvent" in Ci) {
>+    if (gEvent instanceof Ci.nsIAccessibleHideEvent &&
>+        gEvent.targetParent == aAccessible) {
Nit: can do this all in one if.

>+      var sibling = gEvent.targetNextSibling;
>+      var siblingParent = null;
>+      try { // if sibling is unattached then 'parent' call fails
>+        siblingParent = sibling.parent;
>+      } catch(e) {
>+      }
>+      if (sibling && siblingParent == aAccessible) {
>+        attachedSibling = sibling;
>+      } else {
>+        sibling = gEvent.targetPrevSibling;
>+        try {
>+          siblingParent = sibling.parent;
>+        } catch(e) {
>+        }
>+        if (sibling && siblingParent == aAccessible) {
>+          attachedSibling = sibling;
>+          insertBeforeAttachedSibling = true;
>+        }
>+      }
Alternative way of writing this:
try {
  if (gEvent.targetNextSibling.parent == aAccessible) {
    insertBefore = gEvent.targetNextSibling;
  }
} catch (e) {
}
try {
  if (!insertBefore && gEvent.targetPrevSibling.parent == aAccessible) {
    insertBefore = gEvent.targetPrevSibling.nextSibling;
  }
} catch (e) {
}
If gEvent.targetPrevSibling.nextSibling is null then your fallback code will append the hidden accessible, which is exactly where it belongs anyway.

Is it not possible to compute these variables once in inAccTreeView() and pass them as parameters or globals instead of computing them on every call?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #13)

> Is it not possible to compute these variables once in inAccTreeView() and
> pass them as parameters or globals instead of computing them on every call?

it seems there's no much to calculate globally because the logic depends on currently traversed accessible.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: DOMi part (patch3)

Comment 16

[[e-mail]]

Comment on attachment 629108 [details] [diff] [review]
DOMi part (patch3)

[Bah, I got hung up on what I thought was a bug, but it was just because the accessible being hidden had no adjacent visible accessible (it was the first child and its next sibling was also hidden), which means that the code gives up and appends the accessible to the end of the parent's children...]

While testing this, I couldn't seem to find the Accessible Event view in the main menu, is that a bug?

>+  var containsUnattached = false;
Now, previously this used to be called containsUnattachedAcc...

>+    containsUnattachedAcc = true;
Oops! r=me with that fixed.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #16)

> While testing this, I couldn't seem to find the Accessible Event view in the
> main menu, is that a bug?

I filed bug 787948.

> >+  var containsUnattached = false;
> Now, previously this used to be called containsUnattachedAcc...
> 
> >+    containsUnattachedAcc = true;
> Oops! r=me with that fixed.

thank you

Comment 18

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/dom-inspector/rev/a449c941fd30