Closed
Bug 347793
Opened 18 years ago
Closed 6 years ago
Expose specialized accessible interfaces in DOM Inspector
Categories
(Other Applications :: DOM Inspector, defect)
Other Applications
DOM Inspector
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: aaronlev, Assigned: vasiliy.potapenko)
References
Details
(Keywords: access)
Attachments
(3 files, 5 obsolete files)
|
(deleted),
patch
|
timeless
:
review+
neil
:
superreview-
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
surkov
:
review-
|
Details | Diff | Splinter Review |
Right now in DOM Inspector we can only see the results of nsIAccessible.
What about these?
nsIAccessibleDocument
nsIAccessibleEditableText
nsIAccessibleHyperLink
nsIAccessibleHyperText
nsIAccessibleSelectable
nsIAccessibleTable
nsIAccessibleText
nsIAccessibleValue
nsIAccessNode
|
||
Comment 1•18 years ago
|
||
Well, generally speaking once you have an object you think might implement an interface, all you need to do is QueryInterface it. In JavaScript, this is best done with the instanceof operator:
void(object instanceof Components.interfaces.nsIAccessibleFoo);
The beauty of this is that exceptions aren't generated when you don't have an interface.
|
Reporter | |
Comment 2•18 years ago
|
||
Right. I want to be able to see and expand the support accessible interfaces right in DOM inspector.
|
||
Comment 4•18 years ago
|
||
Alex, please do unofficial review :)
There are two issues I don't like.
1) I'd prefer to hide unsupported interfaces instead disabling. But menuitem inherits 'disabled' attribute from a command only.
2) When I'm switching between interfaces then every accessible object has nsIAccessible interface (bug 279090). I can't see workaround without nsIAccessibilityService changing.
Attachment #232877 -
Flags: review?(ajvincent)
|
|
||
Updated•18 years ago
|
Assignee: surkov.alexander → dom-inspector
|
|
||
Updated•18 years ago
|
Status: NEW → ASSIGNED
|
|
||
Updated•18 years ago
|
Assignee: dom-inspector → surkov.alexander
Status: ASSIGNED → NEW
|
|
||
Updated•18 years ago
|
Status: NEW → ASSIGNED
|
|
||
Comment 5•18 years ago
|
||
sorry, forgot to add changing into jar.mn
Attachment #232877 -
Attachment is obsolete: true
Attachment #232879 -
Flags: review?(ajvincent)
Attachment #232877 -
Flags: review?(ajvincent)
|
|
||
Comment 6•18 years ago
|
||
Sorry, for spam. It seems today is not my day :(.
Attachment #232879 -
Attachment is obsolete: true
Attachment #232880 -
Flags: review?(ajvincent)
Attachment #232879 -
Flags: review?(ajvincent)
|
|
||
Comment 7•18 years ago
|
||
Comment on attachment 232880 [details] [diff] [review]
patch3
First pass:
>Index: extensions/inspector/jar.mn
>+ chrome/inspector/viewers/accessibleObject/commandOverlay.xul (resources/content/viewers/accessibleObject/commandOverlay.xul)
>+ chrome/inspector/viewers/accessibleObject/popupOverlay.xul (resources/content/viewers/accessibleObject/popupOverlay.xul)
I think there's something missing here - toolkit has a different way of declaring overlays than xpfe (which uses contents.rdf). Reference http://developer.mozilla.org/en/docs/jar.mn .
>Index: extensions/inspector/resources/content/inspector.xml
>+ <!-- Return command element -->
>+ <method name="getCommand">
>+ <parameter name="aCommandId"/>
>+ <body>
>+ return document.getElementById(aCommandId);
>+ </body>
>+ </method>
Which document? Let's be clear (though you probably don't need to).
>Index: extensions/inspector/resources/content/viewers/accessibleObject/accessibleObject.js
>+ subject.QueryInterface(eval(interfaceId));
Um, no. Eval() in chrome is evil()! I don't care if the source is trusted chrome itself; this is dangerous in chrome, and actually unnecessary, as I will shortly show you.
Just to give you an idea, what if interfaceId equals "doSomething(); Components.interfaces.foo"?
Besides, you shouldn't use QI directly in JS land. This can throw an exception, whereas the instanceof operator doesn't. I know, you've got it in try...catch, but even so, it's still a generated exception that Venkman (when you're debugging) will trip on.
>+ this.mSubject = this.mSubject.QueryInterface(eval(interfaceId));
Again, this cannot be accepted.
>Index: extensions/inspector/resources/content/viewers/accessibleObject/commandOverlay.xul
>+ <command id="cmd:nsIAccessible" viewer="accessibleObject"
Because this is a new file, I'll request (but won't require) you put each attribute (save for the first) on a new line. I know that's unusual, but in the future if we have to change an attribute, the diff will be slightly easier to read.
It also makes it easier to add new attributes - or remove bad ones.
>+ interfaceid="Components.interfaces.nsIAccessible"
Here's what you do:
<command interfaceid="nsIAccessible"/>
if ((typeof Components.interfaces[interfaceid] == "object") &&
(subject instanceof Components.interfaces[interfaceid])) {
// do your magic here
}
else {
// do something else here
}
I'll look more closely later.
|
|
||
Comment 8•18 years ago
|
||
Attachment #232880 -
Attachment is obsolete: true
Attachment #232956 -
Flags: review?(ajvincent)
Attachment #232880 -
Flags: review?(ajvincent)
|
|
||
Updated•18 years ago
|
Attachment #232956 -
Flags: review?(ajvincent) → review?(timeless)
|
|
||
Comment 9•18 years ago
|
||
Aaron, can we add one more method to nsIAccessibilityService like nsISupports getAccessibleFor2(in nsIDOMNode node)? It allows to show correct objects (only those attributes/methods of object that are in choosen interface) in domi.
Attachment #232956 -
Flags: superreview?(neil)
Attachment #232956 -
Flags: review?(timeless)
Attachment #232956 -
Flags: review+
|
||
Comment 10•18 years ago
|
||
Comment on attachment 232956 [details] [diff] [review]
patch4
This didn't work when I tried it (on two machines)
Attachment #232956 -
Flags: superreview?(neil) → superreview-
|
|
Reporter | |
Comment 11•18 years ago
|
||
(In reply to comment #9)
> Aaron, can we add one more method to nsIAccessibilityService like nsISupports
> getAccessibleFor2(in nsIDOMNode node)? It allows to show correct objects (only
> those attributes/methods of object that are in choosen interface) in domi.
I don't understand the suggestion. Can you explain?
|
|
||
Comment 12•18 years ago
|
||
(In reply to comment #11)
> (In reply to comment #9)
> > Aaron, can we add one more method to nsIAccessibilityService like nsISupports
> > getAccessibleFor2(in nsIDOMNode node)? It allows to show correct objects (only
> > those attributes/methods of object that are in choosen interface) in domi.
>
> I don't understand the suggestion. Can you explain?
>
In any way this suggesions doesn't work. The problem is accessible objects showed in js viewer has all methods/attributes of all interfaces that are implemented by it. Therefore idea to see method of specific interface does not work. I have not any thoughts how to get only those methods/attributes that are exactly implemented by specific interface. Who can help?
|
||
Updated•17 years ago
|
QA Contact: timeless → dom-inspector
|
|
||
Comment 13•17 years ago
|
||
Vasiliy, I guess here we should just query interfaces from accessible to show all methods/properties in accessible object view.
Assignee: surkov.alexander → vasiliy.potapenko
Status: ASSIGNED → NEW
|
|
||
Comment 14•17 years ago
|
||
I talked with Alex on irc. It's worth to implement nsIClassInfo on accessible object then we do not need to change something in DOMi to fix the bug.
|
Assignee | |
Comment 15•17 years ago
|
||
This inprogress patch.
|
|
Assignee | |
Updated•17 years ago
|
Status: NEW → ASSIGNED
|
|
||
Comment 16•17 years ago
|
||
Comment on attachment 274870 [details] [diff] [review]
patch5, inprogress
I'm not a peer on this code, but I'll lend an eyeball anyway.
>Index: accessible/src/base/nsAccessible.cpp
>+NS_IMETHODIMP
>+nsAccessible::GetInterfaces(PRUint32 *aCount,
>+ nsIID * **aArray)
>+{
>+ nsCOMPtr<nsIAccessibleDocument> document = do_QueryInterface((nsIAccessible*)this);
>+ nsCOMPtr<nsIAccessibleEditableText> editableText = do_QueryInterface((nsIAccessible*)this);
>+ nsCOMPtr<nsIAccessibleHyperLink> hyperLink = do_QueryInterface((nsIAccessible*)this);
>+ nsCOMPtr<nsIAccessibleHyperText> hyperText = do_QueryInterface((nsIAccessible*)this);
>+ nsCOMPtr<nsIAccessibleSelectable> selectable = do_QueryInterface((nsIAccessible*)this);
>+ nsCOMPtr<nsIAccessibleTable> table = do_QueryInterface((nsIAccessible*)this);
>+ nsCOMPtr<nsIAccessibleText> text = do_QueryInterface((nsIAccessible*)this);
>+ nsCOMPtr<nsIAccessibleValue> value = do_QueryInterface((nsIAccessible*)this);
>+ nsCOMPtr<nsIAccessNode> node = do_QueryInterface((nsIAccessible*)this);
Something seems fundamentally wrong about this whole block. QI to this??? Why?
Looking at QueryInterface, you know this is a nsIAccessible, nsIAccessiblePI. Also, nsAccessNode::QueryInterface, which nsAccessible's QI calls, states support for nsIAccessNode, nsPIAccessNode. Should these be listed too?
>+NS_IMETHODIMP
>+nsAccessible::GetContractID(char * *aContractID)
>+{
>+ *aContractID = nsnull;
How are these objects created? (I'm not saying this is wrong; but if there's an appropriate contract ID, you should supply it.)
>+NS_IMETHODIMP
>+nsAccessible::GetClassDescription(char * *aClassDescription)
>+{
>+ *aClassDescription = nsnull;
>+ return NS_OK;
>+}
>+
Worth putting something here?
|
|
||
Comment 17•17 years ago
|
||
(In reply to comment #16)
> (From update of attachment 274870 [details] [diff] [review])
> I'm not a peer on this code, but I'll lend an eyeball anyway.
Thank you. It's great to have your notes.
> >Index: accessible/src/base/nsAccessible.cpp
> >+NS_IMETHODIMP
> >+nsAccessible::GetInterfaces(PRUint32 *aCount,
> >+ nsIID * **aArray)
> >+{
> >+ nsCOMPtr<nsIAccessibleDocument> document = do_QueryInterface((nsIAccessible*)this);
> >+ nsCOMPtr<nsIAccessibleEditableText> editableText = do_QueryInterface((nsIAccessible*)this);
> >+ nsCOMPtr<nsIAccessibleHyperLink> hyperLink = do_QueryInterface((nsIAccessible*)this);
> >+ nsCOMPtr<nsIAccessibleHyperText> hyperText = do_QueryInterface((nsIAccessible*)this);
> >+ nsCOMPtr<nsIAccessibleSelectable> selectable = do_QueryInterface((nsIAccessible*)this);
> >+ nsCOMPtr<nsIAccessibleTable> table = do_QueryInterface((nsIAccessible*)this);
> >+ nsCOMPtr<nsIAccessibleText> text = do_QueryInterface((nsIAccessible*)this);
> >+ nsCOMPtr<nsIAccessibleValue> value = do_QueryInterface((nsIAccessible*)this);
> >+ nsCOMPtr<nsIAccessNode> node = do_QueryInterface((nsIAccessible*)this);
>
> Something seems fundamentally wrong about this whole block. QI to this???
> Why?
nsAccessNode is base class for all accessible classes therefore he tries to query possible interfaces what are supported, also, note, duiring life time of accessible the list of supported interfaces may be changed (though we tend to get rid this).
Also I would suggest to use QueryInterface() instead of do_QueryInterface. I suppose code should be cleaner.
|
|
||
Comment 18•17 years ago
|
||
I think you missed the point. Let me try again:
do_QueryInterface calls NS_ADDREF. nsCOMPtr stores the reference, and when it goes away, NS_RELEASE happens. These extra calls are unnecessary.
The QI implementation is currently designed to forward a lot of these calls elsewhere.
Since you implement your own QI, I recommend you abstract out the part which checks whether you support an interface into a private function which returns true or false. QI would call this and if the private function returns true, AddRef and do all the other QI magic. GetInterfaces would call this private function and not need the QI.
(In reply to comment #17)
> nsAccessNode is base class for all accessible classes therefore he tries to
> query possible interfaces what are supported, also, note, duiring life time of
> accessible the list of supported interfaces may be changed (though we tend to
> get rid this).
I don't think that's applicable to GetInterfaces(). I think it gets called once by the native code, to determine which interfaces to map to automatically. Modifying the values here later is likely to produce results other than what you expect.
Also, I worry as a general principal about this statement implying you can *drop* support for an interface on the fly - that would probably break things everywhere.
|
|
||
Comment 19•17 years ago
|
||
Ok. Good point.
Also we tend to get rid dynamic changing of interfaces. But it's interesting what about XBL allowing to change interfaces on fly for the given DOM element?
|
|
Assignee | |
Comment 20•17 years ago
|
||
Accessible interfaces is exposed, but when I open accessibleObject view, I see many errors.
Attachment #274870 -
Attachment is obsolete: true
|
|
||
Comment 21•17 years ago
|
||
(In reply to comment #20)
> Created an attachment (id=276723) [details]
> patch6
>
> Accessible interfaces is exposed, but when I open accessibleObject view, I see
> many errors.
>
Your patch has two problems:
1) you do not expose nsIAccessible (but nsIAccessibleRetrival returns it)
2) you may say we support certain interface but actually we don't (note, we redifine QueryInterface()).
|
|
||
Comment 22•17 years ago
|
||
this patch works fine (expose nsIAccessible and nsIAccessibleDocument only).
|
|
Assignee | |
Comment 23•17 years ago
|
||
Implement nsIClassInfo
Attachment #276723 -
Attachment is obsolete: true
Attachment #289933 -
Flags: review?(surkov.alexander)
|
|
||
Comment 24•17 years ago
|
||
Are your new methods are related with QueryInterface? I mean is it possibly QueryInterface returns that interface is supported but your method says no?
|
|
||
Comment 25•17 years ago
|
||
Comment on attachment 289933 [details] [diff] [review]
patch
Right, GetInterfaces() and QueryInterface() aren't related. It's wrong. Btw I mentioned this in comment #21.
Possibly you can use helper macros where we use ISUPPORTS_IMPL macros and use QueryInterface directly where we redefine QueryInterface.
Another way is to call your virtual methods from QueryInterface additionally. Though not sure how much it is natural.
Attachment #289933 -
Flags: review?(surkov.alexander) → review-
|
||
Comment 26•6 years ago
|
||
DomI is no more, closing as WONTFIX.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•