Closed Bug 347793 Opened 18 years ago Closed 6 years ago

Expose specialized accessible interfaces in DOM Inspector

Categories

(Other Applications :: DOM Inspector, defect)

Product:
Other Applications
Component:
DOM Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: aaronlev, Assigned: vasiliy.potapenko)

References

Details

(Keywords: access)

Attachments

(3 files, 5 obsolete files)

patch4
20.57 KB, patch
timeless
: review+
neil
: superreview-
Details | Diff | Splinter Review
patch sample
7.16 KB, patch
Details | Diff | Splinter Review
patch
17.38 KB, 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
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.

Right. I want to be able to see and expand the support accessible interfaces  right in DOM inspector.
I'd like to work.
Assignee: dom-inspector → surkov.alexander
Attached patch patch (obsolete) — — Splinter Review 
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)
Assignee: surkov.alexander → dom-inspector
Status: NEW → ASSIGNED
Assignee: dom-inspector → surkov.alexander
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Attached patch patch2 (obsolete) — — Splinter Review 
sorry, forgot to add changing into jar.mn
Attachment #232877 - Attachment is obsolete: true
Attachment #232879 - Flags: review?(ajvincent)
Attachment #232877 - Flags: review?(ajvincent)
Attached patch patch3 (obsolete) — — Splinter Review 
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 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.
Attached patch patch4 — — Splinter Review 

        Attachment #232880 -
        Attachment is obsolete: true

        Attachment #232956 -
        Flags: review?(ajvincent)

        Attachment #232880 -
        Flags: review?(ajvincent)

        Attachment #232956 -
        Flags: review?(ajvincent) → review?(timeless)

    
    

    
  
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 on attachment 232956 [details] [diff] [review]
patch4

This didn't work when I tried it (on two machines)

        Attachment #232956 -
        Flags: superreview?(neil) → superreview-

    
    

    
  
(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 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?

    
  
QA Contact: timeless → dom-inspector

    
    

    
  
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

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch5, inprogress (obsolete)
        — 
          — Splinter Review
      

    


  
This inprogress patch.

    
  
Status: NEW → ASSIGNED

    
    

    
  
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?

    
    

    
  
(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.


    
    

    
  
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.

    
    

    
  
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?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch6 (obsolete)
        — 
          — Splinter Review
      

    


  
Accessible interfaces is exposed, but when I open accessibleObject view, I see many errors.

        Attachment #274870 -
        Attachment is obsolete: true

    
    

    
  
(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()).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch sample
        — 
          — Splinter Review
      

    


  
this patch works fine (expose nsIAccessible and nsIAccessibleDocument only).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
Implement nsIClassInfo

        Attachment #276723 -
        Attachment is obsolete: true

        Attachment #289933 -
        Flags: review?(surkov.alexander)

    
    

    
  
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 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-

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: