Closed Bug 357430 Opened 18 years ago Closed 18 years ago

New ATK: expose AtkImage interface via nsIAccessibleImage

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: nian.liu)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(2 files, 3 obsolete files)

patch
17.88 KB, patch
aaronlev
: review+
Details | Diff | Splinter Review
patchv2
18.48 KB, patch
ginnchen+exoracle
: review+
Details | Diff | Splinter Review 
Images in Mozilla should expose the AtkImage interface.

Question: should image buttons expose that as well? For example toolbar buttons?
Blocks: newatk
<html>
<head>
<title>Frame Test</title>
</head>
<frameset name=mainframe id=mainframe frameborder=0 border=0 cols="167,11,*">
        <frame name=menu noresize="true" marginwidth=0 marginheight=0 src="">
        <frame scrolling=no noresize="true" name=toogle marginwidth=0 marginheight=0 src="https://bugzilla.mozilla.org/show_bug.cgi?id=349716">
</frameset>
</html>

sorry, comment#1 was added by mistake. I am creating an test case for another bug. Too many windows were opened... 
so... I am working on this one. 

Shall we support set_image_description and get_image_locale right now?
Assignee: aaronleventhal → gaomingcn
get_locale - yes
set_image_description -- just return PR_FALSE for not completed. Support get_image_description instead. I think we will want to just use the string from GetDescription) in that.


I think the general rule of thumb might be "if it exposes x, it should implement the associated accessibility specialization."  For example:

x          specialization
-------    ----------------
text       Accessible Text 
image      Accessible Image
table      Accessible Table
...

Note that 'text' means it paints text on the screen, so this includes non-user-editable text such as checkboxes, labels, etc.
Attached patch Draft V1 (obsolete) — — Splinter Review 
This is really a draft. Just make sure I am in the right track.
That's the right general idea, but description you can get from nsIAccessible::description, so you can remove that from nsIAccessibleImage.

In fact the only reason you need a separate bounds from what nsIAccessible::bounds gives you is that something like a XUL tooltip button might have both an image and text. The imageBounds need to be the bounds for just the image.

But, the bounds should just be returned with 1 method, not 5 attributes. You don't need coordType there -- it's always going to be the same inside Mozilla. Use the same coord type as nsIAccessible bounds use: http://lxr.mozilla.org/seamonkey/source/accessible/public/nsIAccessible.idl#215

You'll end up with an nsIAccessibleImage interface with just 1 method, something like:
  void getImageBounds(out long x, 
                      out long y, 
                      out long width, 
                      out long height);
In fact for most things the impl will just be:
  return GetBounds(x, y, width, height);
Only widgets that contain both text and an image will need expose a unique imageBounds.
Attached patch patch draft v2 (obsolete) — — Splinter Review 
Addressing Aaron's comments. But I am not sure what getImageLocaleCB() should be..
Attachment #243349 - Attachment is obsolete: true
You can skip image locale for now. We'll support that when we support locale for text attributes.
Attached patch patch draft v3 (obsolete) — — Splinter Review 

        Attachment #244569 -
        Attachment is obsolete: true

        Attachment #244801 -
        Flags: review?(aaronleventhal)

    
  

        Attachment #244801 -
        Flags: review?(aaronleventhal) → review?(ginn.chen)

    
    

    
  
Comment on attachment 244801 [details] [diff] [review]
patch draft v3

First, it doesn't work since no where makes QueryInterface success for nsIAccessibleImage.

Other comments,
1) Do we really need add the new function GetImageBounds? 
2) We don't need get/setImageDescriptionCB, getImageLocaleCB if we don't implement it.
3) It's bad to return null for gchar *, should return an empty string instead. Anyway it should be gone as comment 2)

        Attachment #244801 -
        Flags: review?(ginn.chen) → review-

    
    

    
  
(In reply to comment #11)

> 3) It's bad to return null for gchar *, should return an empty string instead.
> Anyway it should be gone as comment 2)
> 

It's an API doc bug, the return value should be a string or null.
So ignore this comment.
Assignee: gaomingcn → nian.liu

    
    

    
  
is there a list which xul/html element should implement Image interface? or we just expose Image interface on the accessible object which is ATK_ROLE_IMAGE, in other words ROLE_CHARACTER, ROLE_GRAPHIC, ROLE_DIAGRAM

ginn prefered later. aaron, what's your opinion?

    
    

    
  
Not done per-role. It needs to be done on a per-class basis by adding an nsIAccessibleImage. That will allow us to implement both AtkImage and IAccessibleImage (which is part of IAccessible 2 -- see http://www.linux-foundation.org/en/Accessibility/IAccessible2)

So, nsHTMLImageAccessible is the most obvious one. Also, both nsXULButtonAccessible and some of the HTML buttons have an accessible.

We don't use ROLE_CHARACTER, and ROLE_DIAGRAM is currently used to indicate the root of an SVG subtree. SVG is not necessarily a single image but can be anything and can be interactive. Therefore I'd recommend against using this for ROLE_DIAGRAM stuff.

    
    

    
  
(In reply to comment #14)
> Not done per-role. It needs to be done on a per-class basis by adding an
> nsIAccessibleImage. That will allow us to implement both AtkImage and
> IAccessibleImage (which is part of IAccessible 2 -- see
> http://www.linux-foundation.org/en/Accessibility/IAccessible2)
> 
> So, nsHTMLImageAccessible is the most obvious one. Also, both
> nsXULButtonAccessible and some of the HTML buttons have an accessible.
> 
> We don't use ROLE_CHARACTER, and ROLE_DIAGRAM is currently used to indicate the
> root of an SVG subtree. SVG is not necessarily a single image but can be
> anything and can be interactive. Therefore I'd recommend against using this for
> ROLE_DIAGRAM stuff.
> 
nsHTMLImageAccessible seems to be clear enough. others seems not. even if we only consider it in xul scope, lots of xul element has the attribute "image". do we need expose nsIAccessibleImage for all of them?



    
    

    
  
Which XUL elements have an attribute of image? Yes, ideally I think we'd expose nsIAccessibleImage for all of them.

Note that QueryInterface can be "smart". We do that in nsAccessible and nsHyperTextAccessible.

    
    

    
  
We'll need this for IAccessibleImage support as well, so nsIAccessibleImage will be important.
Blocks: ia2, atk
No longer blocks: newatk
Summary: New ATK: expose AtkImage interface → New ATK: expose AtkImage interface via nsIAccessibleImage

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  

        Attachment #244801 -
        Attachment is obsolete: true

        Attachment #254399 -
        Flags: review?(ginn.chen)

    
    

    
  
Comment on attachment 254399 [details] [diff] [review]
patch

bug 369713 filed for xul element

        Attachment #254399 -
        Flags: review?(aaronleventhal)

    
  
Blocks: 369713

    
    

    
  
NS_ConvertUTF16toUTF8 should be used for getImageDescriptionCB
Is there a problem if we reuse getDescriptionCB (AtkObject *aAtkObj) ?

ATK_XY_SCREEN should be deal with or add TODO and file a bug

why we need ~nsHTMLImageAccessible()?

    
    

    
  
(In reply to comment #20)
> NS_ConvertUTF16toUTF8 should be used for getImageDescriptionCB
> Is there a problem if we reuse getDescriptionCB (AtkObject *aAtkObj) ?
i guess it's ok.
> 

> ATK_XY_SCREEN should be deal with or add TODO and file a bug
> 
i'll file it.
> why we need ~nsHTMLImageAccessible()?
> 
NS_INTERFACE_MAP_BEGIN needs that.


    
    

    
  
What about image locale?
Why the empty destructor?
What about support on HTML and XUL image buttons?

Why all this?
NS_IMPL_ADDREF_INHERITED(nsHTMLImageAccessible, nsLinkableAccessible)	
NS_IMPL_RELEASE_INHERITED(nsHTMLImageAccessible, nsLinkableAccessible)
NS_INTERFACE_MAP_BEGIN(nsHTMLImageAccessible)	
  NS_INTERFACE_MAP_ENTRY(nsIAccessibleImage)
NS_INTERFACE_MAP_END_INHERITING(nsLinkableAccessible)

Instead of just:
NS_IMPL_ISUPPORTS_INHERITED1(nsHTMLImageAccessible, nsLinkableAccessible, nsIAccessibleImage)

    
    

    
  
I see our comment about why you need the empty destructor now in comment 21, but again why the interface map instead of NS_IMPL_ISUPPORTS_INHERITED1? 

    
    

    
  
Comment on attachment 254399 [details] [diff] [review]
patch

r= if use NS_IMPL_ISUPPORTS1, remove empty destructor and add comment to bug 287737 to handle image locale.

        Attachment #254399 -
        Flags: review?(aaronleventhal) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patchv2
        — 
          — Splinter Review
      

    


  
addressing ginn and aaron's comments

        Attachment #254504 -
        Flags: review?(ginn.chen)

    
  

        Attachment #254399 -
        Flags: review?(ginn.chen)

    
    

    
  
Comment on attachment 254504 [details] [diff] [review]
patchv2

committed in,
empty lines at end of file removed,
changed "to do" to "TODO",
added a comment at getDescriptionCB

        Attachment #254504 -
        Flags: review?(ginn.chen) → review+

    
  
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: