make outputTree of Accessible Events view working for hide events

RESOLVED FIXED in mozilla13

Status

RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

unspecified
mozilla13
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

7 years ago
Created attachment 595773 [details] [diff] [review]
a11y part
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #595773 - Flags: review?(trev.saunders)
(Assignee)

Comment 2

7 years ago
Created attachment 595775 [details] [diff] [review]
DOMi part
Attachment #595775 - Flags: review?(neil)
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.
Attachment #595773 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 4

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

Comment 9

7 years ago
(In reply to neil@parkwaycc.co.uk 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.
(Assignee)

Comment 10

7 years ago
Created attachment 598905 [details] [diff] [review]
DOMi part (patch2)

updated, still based on bug 658776 patch which waits for Colby review.
Attachment #595775 - Attachment is obsolete: true
Attachment #595775 - Flags: review?(neil)
(Assignee)

Updated

7 years ago
Summary: make outputTree(target.document, [target] working for hide events → make outputTree of Accessible Events view working for hide events
(Assignee)

Updated

7 years ago
Attachment #598905 - Flags: review?(neil)
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...
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 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?
Attachment #598905 - Flags: review?(neil) → review-
(Assignee)

Comment 14

7 years ago
(In reply to neil@parkwaycc.co.uk 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.
(Assignee)

Comment 15

7 years ago
Created attachment 629108 [details] [diff] [review]
DOMi part (patch3)
Attachment #598905 - Attachment is obsolete: true
Attachment #629108 - Flags: review?
(Assignee)

Updated

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

Comment 17

6 years ago
(In reply to neil@parkwaycc.co.uk 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
(Assignee)

Comment 18

6 years ago
http://hg.mozilla.org/dom-inspector/rev/a449c941fd30
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.