Closed
Bug 725644
Opened 13 years ago
Closed 12 years ago
make outputTree of Accessible Events view working for hide events
Categories
(Other Applications :: DOM Inspector, defect)
Other Applications
DOM Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla13
People
(Reporter: surkov, Assigned: surkov)
References
Details
Attachments
(2 files, 2 obsolete files)
5.90 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
5.74 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #595773 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #595775 -
Flags: review?(neil)
Comment 3•13 years ago
|
||
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•13 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
Comment 5•13 years ago
|
||
(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.
Assignee | ||
Comment 6•13 years ago
|
||
inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/3f6d5d9bac61
please do not close the bug
Comment 7•13 years ago
|
||
Target Milestone: --- → mozilla13
Comment 8•13 years ago
|
||
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•13 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•13 years ago
|
||
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•13 years ago
|
Summary: make outputTree(target.document, [target] working for hide events → make outputTree of Accessible Events view working for hide events
Assignee | ||
Updated•13 years ago
|
Attachment #598905 -
Flags: review?(neil)
Comment 11•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 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•13 years ago
|
||
Attachment #598905 -
Attachment is obsolete: true
Attachment #629108 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #629108 -
Flags: review? → review?(neil)
Comment 16•12 years ago
|
||
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•12 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•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Blocks: DOMi2.0.14
You need to log in
before you can comment on or make changes to this bug.
Description
•