124448 - Active Accessibility: support HTML plugins (<object>, <embed>, <applet>, etc.)

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P1)

Description

[Aaron Leventhal]

Test cases are in the URL above.

We need to do several things.
1. Report the <object> and an IAccessible with as much info as possible,
including bounds.
2. If the content in the object supports MSAA, pass it through3. Support
anything in the alternative renderings

Comment 1

[Aaron Leventhal]

-> John Gaunt

Comment 2

[John Gaunt (redfive)]

looking at those testcases it seems we don't support the object tag very well.
:-( I'll probably get to file some bugs on that support as I implement this. :-)

Are there specific, high profile apps that use the <object> tag I should focus
on first? Like Java Applets, images....

Time to fire up IE and see how they do...

Comment 3

[Aaron Leventhal]

I'd first go for the object that is simply a container for another HTML
document, since that should be the simplest case. I'd go for PDF's second.

Comment 4

[Jaime Rodriguez, Jr.]

nsbeta1+ per ADT/Embed triage.

Comment 5

[John Gaunt (redfive)]

Attachment: v1.0 of Object Element support

v1.0 - There is a problem with hit testing (as always), but I am able to walk
the sibling tree into Object tags pointing to HTML files, and images show up as
images. 

I haven't tested on txt files yet, but I suspect they will behave the same as
the HTML since they use the same code. Plugins have not really been addressed
in this code yet. And there is a crash situation I have to figure out too.

todo:

Comment 6

[John Gaunt (redfive)]

whoops, tab takes you out of the text area - duh!

todo:
    - test txt file support
    - fix hit testing for HTML and presumably TXT files
    - verify that images support is working properly and completly
    - fix the crash situation
    - get plugins working

( piece of cake. :-) )

Comment 7

[John Gaunt (redfive)]

Attachment: v1.3 of <object> support

This fixes the crash that was happening and the html hit test too (same thing
oddly fixed both!?). The image support has also been tested and cleaned up. It
looks like I can just add a couple things to Accessible.cpp and I won't have to
introduce any new classes ( yay! ).

Text file support is still a little wonky. I'm getting the information from the
document, but it is coming in one big chunk, no line breaks or anything.
Perhaps that is how we do it? One big text element? Will poke a little more. 

Still to do:
    - text file support ( is it actually working right? )
    - plugin support
    - clean up

Comment 8

[Deneb Meketa]

Hi there - Flash Player developer chiming in.  Our Netscape plugin doesn't 
support MSAA yet, but we've made it work for ActiveX, so it seems likely that 
we'll turn this on in the Netscape plugin eventually.

My question is: what pass-through arrangement is intended for plugins?  The 
MSAA architecture is pretty simple but I'd like to confirm a few details.  IE 
does MSAA proxying as well, and there are some interesting lessons there.

The most obvious piece is that you have to represent the plugin as a child 
object of whatever contains it - HTML pane, table, whatever.  But the 
connection between the HTML container element and the plugin's root object is 
not straightforward.  The MSAA client is probably going to be assuming that it 
can just traverse the tree of IAccessible objects in the HTML page, regardless 
of whether they are contained in plugins.  Thus, when the IAccessible for the 
HTML container is asked for its children, the code behind that IAccessible 
needs to recognize that a plugin is present, send a WM_GETOBJECT message to the 
plugin (probably via AccessibleObjectFromWindow), and include the returned root 
object from the plugin among the child objects.  Thereafter, the client will 
communicate directly with the plugin via the IAccessible to which you returned 
a pointer.  However, you must be careful in your COM reference-count 
accounting; once you obtain an IAccessible pointer from the plugin, treat it as 
you would any COM pointer, calling AddRef() and Release() as needed.

It is possible that I am wrong about this proxying strategy.  A plausible 
alternative is that if you hand out an IAccessible of role WINDOW to represent 
the plugin, the client will be smart enough to try and locate the HWND for that 
object (can't be done directly, client would have to compare screen rects or QI 
for IOleWindow if you support it) and then call AccessibleObjectFromWindow on 
that HWND, relieving the browser of any responsibility for the plugin's 
IAccessible objects.  This might explain why IE always provides an IAccessible 
of role WINDOW to wrap a plugin.  And, by the way, if anything weird is 
happening with plugins, it's possible that clients are depending on that IE-
esque behavior of providing a WINDOW wrapper, meaning that you might consider 
doing the same.

There's a possibility that I hope no one is considering, because it would be 
evil: the browser could pretend it was an MSAA client, and recursively obtain 
all the IAccessibles for a plugin, and then hand out proxies of those objects 
itself.  This would be bad!  An unnecessary hassle for the browser, and might 
interfere with subtle forms of communication between client and plugin.

Another general issue to be aware of is focus.  Some screen readers (esp. JAWS) 
depend heavily on receiving correct sequences of focus events.  This may 
include focus events sent via MSAA, or also hooking the overall Windows system 
focus in some way.  There are some subtle timing bugs in IE that cause focus 
events to come out in the wrong order.  This can destroy the ability of a 
plugin to work correctly with a screen reader.

I guess if I were being maximally anal about it, I'd say get in touch directly 
with the screen reader vendors.  That's what we had to do in order to get Flash 
working.  The idea of just implementing MSAA support to a generic spec, and 
then having screen readers just instantly see your content perfectly, is a bit 
misleading.  Although in the case of Mozilla, you do have the precedent that IE 
sets, whereas with Flash, we were working with a new and different kind of 
content, which made for confusion.  So maybe you're okay without contacting the 
screen reader vendors.  And they do have a habit of wanting to be paid in order 
to support your application.  Hrm.

Comment 9

[John Gaunt (redfive)]

I'm going to clean this up and start reviews today. Without an accessible plugin
to test the plugin portion of the object tags we are a bit hung up on that. All
the rest of the object tag support is working cool. I'll test that is jives with
the embed tags ( it should according to a discussion I had with peterl a while
back ). I haven't even looked at applet tags yet, as the original bug was just
object support. That may have to be another bug. I'll test those too.

Comment 10

[John Gaunt (redfive)]

Attachment: v2.0 of patch

This may not quite be the final patch, but I forsee the only changes coming to
be refinement of the default plugin reporting ( in nsHTMLNatWinWAccessible.cpp
) and maybe some mac/unix build-fu changes

Comment 11

[John Gaunt (redfive)]

Attachment: mac project changes

to be rolled into the revised global patch

Comment 12

[John Gaunt (redfive)]

Attachment: v3.0 - includes mac and linux build changes

Comment 13

[Aaron Leventhal]

Are the full page plugins supposed to be handled with this patch? I thought
earlier you said that would be part of the PDF bug.

I don't like the name nsHTMLNatWinWAccessible - how about
nsHTMLNativeWindowAccessible or nsHTMLNativeWin32WindowAccessible?

This needs to be capitalized correctly (occurs more than once)
#include "nsplugindefs.h"

What do these changes do?
-        nsCOMPtr<nsIAccessible> innerRootAccessible =
+        nsHTMLIFrameRootAccessible *innerRootAccessible =
           new nsHTMLIFrameRootAccessible(aDOMNode, innerWeakShell);
 
         if (innerRootAccessible) {
-          nsHTMLIFrameAccessible* outerRootAccessible =
+          nsHTMLIFrameAccessible *outerRootAccessible =
             new nsHTMLIFrameAccessible(aDOMNode, innerRootAccessible,
                                        outerWeakShell, sub_doc);
 
           if (outerRootAccessible) {
+            innerRootAccessible->Init(outerRootAccessible);
             *_retval = outerRootAccessible;
-            NS_ADDREF(*_retval);
-
-            return NS_OK;
+            if (*_retval) {
+              NS_ADDREF(*_retval);
+              return NS_OK;
+            }

This looks like it was supposed to be removed:
+// JG3

Your cases 1, 2 & 3 in the long comment about GetHTMLObjectAccessibleFor()
disagree with the cases
in the method itself. For example:
  * so, we can have several cases here. 
  *  1) An image or imagemap, where the image frame points back to 
  *     the object element DOMNode
and
// 1) for object elements containing either HTML or TXT documents

Put some kind of NS_WARN here so we can find out if we ever do get here:
+          // we will probaby never be here though because peterl said there is
no DOM(or doc?) when we load
+          //  a full page plugin. that will have to be handled at the root
IAccessible level I guess

Does this code work for embed or applet tags? We should comment in here how
those are handled
+  // ---- Check if we are an Object tag
+  nsCOMPtr<nsIDOMHTMLObjectElement> object(do_QueryInterface(aNode));
+  if (object)
+    return GetHTMLObjectAccessibleFor(aNode, shell, _retval);

In what cases do we need this second check? Would this second check suffice - do
we still need to check the tag when we have this?
+  // ---- Need this second check for object frames because we sometimes get
+  //      a document that doesn't get picked up in the dom node check earlier
+  nsCOMPtr<nsIAtom> frameType;
+  frame->GetFrameType(getter_AddRefs(frameType));
+  if (frameType.get() == nsLayoutAtoms::objectFrame) {
+    nsObjectFrame* objectFrame = NS_STATIC_CAST(nsObjectFrame*, frame);
+    if (objectFrame)
+      return GetHTMLObjectAccessibleFor(aNode, shell, _retval);
+  }

No one else understands what JG3 means :)  Is this your new thing, to put this
in all over? I think CVS blame handles that better
+    // JG3 -- need to check for plugin focus?
+  /* JG3 This should not be needed anymore
etc.

Are mozilla/accessible/src/base/nsSelectAccessible.*,
mozilla/accessible/src/html/nsHTMLSelectAccessible.* and
mozilla/accessible/src/xul/nsXULSelectAccessible.* supposed to be part of the patch?

I think the in argument should be called aOuterAccessible. I would think a node
would be for a dom node
+/** called when creating an Iframe for Object tags */
+void nsHTMLIFrameRootAccessible::Init(nsIAccessible * outerNode)
+{
+  if (outerNode) {
+    mOuterAccessible = outerNode;
+  }
+}
and
+    void Init(nsIAccessible * outerNode);

I take it this needs to be removed?
+/* JG3 clean up
+//  nsPluginPort *tempPort = mInstanceOwner->GetPluginPort();
+//  nsPluginPlatformWindowRef hwnd = nsPluginPlatformWindowRef(tempPort);
+//  aPort = &tempPort;
+//  PRUint32 tempInt = (PRUint32)tempPort;
+//  *aPort = (PRInt32)hwnd;
+//  *aPort = (PRInt32)tempPort;
+//  *aPort = mInstanceOwner->GetPluginPort();
+*/
+}

There are still printf's that need to be removed.

I'm not sure why plugin stuff is handled in the taboanel code. What about
browsers or ebeding projects tahat don't use tabpanels?

Based on all the scaffolfing left in, such as extrea comments, it seems like you
meant to refine this more before I r=d it.

Looking fwd to the next patch.

Comment 14

[John Gaunt (redfive)]

Attachment: v4, revamped approach

Taking a slightly differenet approach. Text HTML and image object tags are
handled the same, in the case of object tags referencing content needing a
plugin we are going to wrap the window with an accessible object. The single
child of that object will ultimately be the IAccessible for the Plugin window.
Internally we wrap that as well, but never return an IAccessible corresponding
to our nsIAccessible. A check in  Accessible::NewAccessible() checks for the
special nsIAccessibleWin32Object interface and then gets the proper IAccessible
from the window handle.

Comment 15

[Aaron Leventhal]

Should CreateHTMLPluginAccessible just be called CreatePluginAccessible?
My thinking is that plugins are not really inherently HTML. I don't think it's a
big deal keeping this the way it is, just raising the issue.

Either way, shouldn't nsAccessibilityService::GetHTMLObjectAccessibleFor() now
be named GetPluginAccessibleFor() or GetHTMLPluginAccessibleFor() ?

In nsHTMLWin32ObjectAccessible.*
I don't think you need to refer to Accessible.h or |IAccessible   *mpAcc;|
Don't you want remove those and any places in accessible's build system where
you added a reference to the widget subsystem?

Other than that looks great.

Comment 16

[John Gaunt (redfive)]

The IAccessible stuff is not needed. Left over from some proxy-ing stuff I had
in there earlier. 

The GetHTMLObjectAccessible() method should still stand because only in one case
do we actually get a Plugin, in the other two case we return different types of
accessibles, but the DOM node we are originally dealing with is an ObjectElement.

I think HTMLPluginAccessible is correct since we are only dealing with the HTML
space at this time. Plus there isn't a way to do plugins or object tag type
stuff in XUL that I can see.

I'll repost a patch with the changes just to have it current in the bug.

Comment 17

[John Gaunt (redfive)]

Attachment: v4.1 - diff with current tree and the slight changes from review

Comment 18

[John Gaunt (redfive)]

Attachment: new mac build changes, includes nsHTMLPluginAcc...

Comment 19

[Aaron Leventhal]

Comment on attachment 82988 [details] [diff] [review]
v4.1 - diff with current tree and the slight changes from review

r=aaronl

Comment 20

[Aaron Leventhal]

Comment on attachment 83050 [details] [diff] [review]
new mac build changes, includes nsHTMLPluginAcc...

r=aaronl

Comment 21

[John Gaunt (redfive)]

Attachment: v4.2 - rolled up the mac build changes with a linux build change or two and the 4.1 patch

Just added a header file, a depends dir and a -I to the linux Makefile.in for
src/html, plus the mac build changes that added nsHTMLPluginAccessible.cpp to
the project.

Comment 22

[John Gaunt (redfive)]

Comment on attachment 83061 [details] [diff] [review]
v4.2 - rolled up the mac build changes with a linux build change or two and the 4.1 patch

rolling forward aaronl's review

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 83061 [details] [diff] [review]
v4.2 - rolled up the mac build changes with a linux build change or two and the 4.1 patch

- In nsAccessibilityService.cpp:

-	 nsCOMPtr<nsIAccessible> innerRootAccessible =
+	 nsHTMLIFrameRootAccessible *innerRootAccessible =
	   new nsHTMLIFrameRootAccessible(aDOMNode, innerWeakShell);

	 if (innerRootAccessible) {
-	   nsHTMLIFrameAccessible* outerRootAccessible =
+	   nsHTMLIFrameAccessible *outerRootAccessible =
	     new nsHTMLIFrameAccessible(aDOMNode, innerRootAccessible,
					outerWeakShell, sub_doc);

	   if (outerRootAccessible) {
+	     innerRootAccessible->Init(outerRootAccessible);
	     *_retval = outerRootAccessible;
-	     NS_ADDREF(*_retval);
-
-	     return NS_OK;
+	     if (*_retval) {
+	       NS_ADDREF(*_retval);
+	       return NS_OK;
+	     }
	   }
	 }
       }

If "new nsHTMLIFrameAccessible(...)" fails, we'll leak innerRootAccessible.

- In nsAccessibilityService::GetHTMLObjectAccessibleFor():

+  if (pluginInstance) {
     ...
+    return NS_OK;
+  }
+
+  // 3) for images and imagemaps
+  else {

else-after-return! Loose the pointless else here.

And why return an error code at the end of this method? Isn't setting the out
param to null enough?

- Further down in the same file:

+  nsCOMPtr<nsIAtom> frameType;
+  frame->GetFrameType(getter_AddRefs(frameType));
+  if (frameType.get() == nsLayoutAtoms::objectFrame) {
+    nsObjectFrame* objectFrame = NS_STATIC_CAST(nsObjectFrame*, frame);
+    if (objectFrame)

This check is pointless, you know frame is non-null, the static cast won't
change that.

- In nsHTMLIFrameRootAccessible::Init():

+      nsIFrame* frame = nsnull;
+      parentShell->GetPrimaryFrameFor(content, &frame);
+      NS_ASSERTION(frame, "No outer frame.");
+      frame->GetAccessible(getter_AddRefs(mOuterAccessible));

What if content (the iframe) is hidden, then there won't be a frame, and we'll
assert, and then crash. Can this not happen, or do we need to replace the
assert with a null check?

- In nsHTMLPluginAccessible::GetAccFirstChild(), initialize the out param to
null even in error cases, and is the lack of a frame for the plugin really an
error? Also:

+  if (!frame)
+    return NS_ERROR_FAILURE;
+
+  nsObjectFrame* objectFrame = NS_STATIC_CAST(nsObjectFrame*, frame);
+  if (objectFrame) {

pointless if check...

- Nitpick of the day:

nsHTMLPluginAccessible::GetAccRole()
+{
+  *_retval= ROLE_WINDOW;

Add a space before '='.

Hmm, are windows window handles really just 32 bit integers? Is it safe to pass
these to JS and back?

- In nsHTMLWin32ObjectAccessible.h:

+class nsHTMLWin32ObjectAccessible : public nsAccessible,
+					    nsIAccessibleWin32Object

Missing 'public' (for consistency if nothing else).

- In nsObjectFrame::GetAccessible(), initialize the out param to null if error.

Passing window handles as 32 bit integers, possibly through JS is the one thing
in this patch that doesn't make me feel good, but if you know that this is the
best option here, then I guess we'll haveto live with this and just make sure
we don't run into problems with window handle lifetime or such...

sr=jst with the above fixed.

Comment 24

[John Gaunt (redfive)]

- leak plugged
- else removed
- pointless if removed
- null check added
- retval nulled
- another pointless if removed
- nit picked
- public added 

code checked in (finally - the whale, she is dead!)

Comment 25

[Jaime Rodriguez, Jr.]

Doron/Shrir - Can you verify this on the trunk, and ensure no regressions have
been introduced. thanks!

Comment 26

[Jaime Rodriguez, Jr.]

Mass removing adt1.0.0, and adding adt1.0.1 because, we are now on 1.0.1.

Comment 27

[scottputterman]

What bug does the user see with this not fixed? Does this mean they won't be
able to access any content requiring a plugin? Also, we need for this to be
verified per Jaime's previous comments.

Comment 28

[Aaron Leventhal]

This allows screen reader vendors to make not just Gecko itself accessible to
blind users, but any plugin content inside our browser as well.

Comment 29

[Aaron Leventhal]

Not sure how this should be marked, but it doesn't need to go in on the branch.
We're not going to get enough mileage for the risk at this point.

Comment 30

[scottputterman]

adding adt1.0.1- per Aaron's comments. It'll be in the next release.

Comment 31

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381