485270 - embed and object HTML tags should be given an accessible role of embedded object

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090324 Minefield/3.6a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090324 Minefield/3.6a1pre

Currently, HTML embed and object tags expose an IAccessible2 role of either IA2_ROLE_UNKNOWN or IA2_ROLE_TEXT_FRAME. (Strangely, I can't work out how Gecko determines which will be chosen; reloading the page sometimes causes it to change.) These elements should always be mapped to an IAccessible2 role of IA2_ROLE_EMBEDDED_OBJECT. I'm not sure of the situation for ATK, but the situation is probably similar.

Note that accRole does return "object" or "embed". However, since there is an appropriate IA2 role, it should be mapped.

Reproducible: Always

Comment 1

[Marco Zehe (:MarcoZ)]

I'm not sure we can do anything about this. According to the documentation in accessible/src/msaa/nsHTMLWin32ObjectAccessible:

  * This class is used only internally, we never! send out an IAccessible linked
  *   back to this object. This class is used to represent a plugin object when
  *   referenced as a child or sibling of another nsAccessible node. We need only
  *   a limited portion of the nsIAccessible interface implemented here. The
  *   in depth accessible information will be returned by the actual IAccessible
  *   object returned by us in Accessible::NewAccessible() that gets the IAccessible
  *   from the windows system from the window handle.

We never create an IAccessible for any such embedded object, but let the plugin return its root accessible instead.

Surkov, did I understand this correctly?

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

We might want to create an intermediate dummy object with an embedded object role. comment 0, is worrisome in any event... since the current broken role can change on page reload... not sure what's going on there.

James, can you attach a test file?

Comment 3

[alexander :surkov (:asurkov)]

Marco, this comment might be out of date. Iirc we have windowed and windowless plugins, I'm not sure whether windowless plugins have window handle. In this case we could expose embedded accessible as container for plugin hierarchy.

Comment 4

[James Teh [:Jamie]]

Okay. I have finally managed to track down a bit more info for this one.

First of all, I can't seem to reproduce the issue where the role changes intermittently. The web site on which this issue occurred doesn't seem to use embeds anymore. Therefore, let's remove that part from the discussion.

On a normal Flash embed (which I assume has a different hwnd), I get an IA2 role of IA2_ROLE_UNKNOWN. This is undesirable (we should get IA2_ROLE_EMBEDDED_OBJECT, but for IA2_ROLE_UNKNOWN, we can (and NVDA does) fall back to IAccessible's accRole, so we can at least work around this.

However, on a Flash embed with wmode="transparent" set, we get an IA2 role of IA2_ROLE_TEXT_FRAME. (I'm guessing that wmode="transparent" makes it windowless, but this is only a guess.) This is much worse because we got a valid IA2 role and therefore assume it is correct. I'm pretty sure Firefox is doing this because afaik Flash doesn't support IA2.

I've attached a test case which includes the same Flash movie both with and without wmode="transparent". Observe that the IA2 role for the first (non-transparent) embed is IA2_ROLE_UNKNOWN. Observe that the IA2 role for the second (transparent) embed is IA2_ROLE_TEXT_FRAME.

Comment 5

[James Teh [:Jamie]]

Attachment: Test case.

Comment 6

[alexander :surkov (:asurkov)]

James, thank for the example. @wmode="transparent" actually makes it windowless and plugin hasn't HWND, we don't create it embed accessible in this case. As well, this plugin hasn't accessible hierarchy, I guess because there is no HWND. It sounds we should do similar stuffs like in bug 480317. As well it's worth to contact with a11y Flash player developers to get their opinion.

I'll put the patch for the first case. The second case should be a subject of different bug I guess because the problem is deeper than wrong role.

Comment 7

[Marco Zehe (:MarcoZ)]

As far as I remember talking to a web developer quite versed in how to make Flash accessible, she told me that the windowless plugin mode indeed doesn't support accessibility right now. So this *is* a different and deeper issue. Surkov, could we at least help NVDA by not exposing this one as a textframe object so they don't try to walk into it? e. g. by checking the tag and the attribute and return role_unkonw or something?

Comment 8

[alexander :surkov (:asurkov)]

Ok. We had a long discussion with Jamie per IRC and I didn't figure out should we do this or not :) There is no big win, but it would be nice I guess. So it should be easy enough to fix, so I think I could add this hack to the patch until we come to the right solution.

Comment 9

[James Teh [:Jamie]]

(In reply to comment #7)
> Surkov, could we at least help NVDA by not exposing this one as a textframe
> object so they don't try to walk into it? e. g. by checking the tag and the
> attribute and return role_unkonw or something?
Walking into it isn't an issue; there's no subtree, so there's nothing to walk into. :) We have to render "unknown" objects because Gecko currently renders some valid objects with IA2_ROLE_UNKNOWN, so there's no real way to avoid rendering this. Exposing it as IA2_ROLE_EMBEDDED_OBJECT would at least make it obvious to the user that there is some sort of embedded object present, even if broken, but perhaps this is misleading.

Comment 10

[Marco Zehe (:MarcoZ)]

Idea: If we exposed this as embedded object but with state_disabled, meaning it can't be interacted with because it's an object without subtree, would that help?

Comment 11

[James Teh [:Jamie]]

(In reply to comment #10)
> Idea: If we exposed this as embedded object but with state_disabled, meaning it
> can't be interacted with because it's an object without subtree, would that
> help?
Marco, you're a genius. Sounds great. Ship it! :)

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 13

[Marco Zehe (:MarcoZ)]

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

>-    if (pluginPort) {
>-      *aAccessible = new nsHTMLWin32ObjectOwnerAccessible(node, weakShell, pluginPort);
>-      if (*aAccessible) {
>-        NS_ADDREF(*aAccessible);
>-        return NS_OK;
>-      }
>-    }
>+
>+    *aAccessible =
>+      new nsHTMLWin32ObjectOwnerAccessible(node, weakShell, pluginPort);
>+    NS_ENSURE_TRUE(*aAccessible, NS_ERROR_OUT_OF_MEMORY);
>+
>+    NS_ADDREF(*aAccessible);
>+    return NS_OK;

Why did you remove the null check for  pluginPort?

r=me for the test part.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> (From update of attachment 405012 [details] [diff] [review])
> >-    if (pluginPort) {
> >-      *aAccessible = new nsHTMLWin32ObjectOwnerAccessible(node, weakShell, pluginPort);
> >-      if (*aAccessible) {
> >-        NS_ADDREF(*aAccessible);
> >-        return NS_OK;
> >-      }
> >-    }
> >+
> >+    *aAccessible =
> >+      new nsHTMLWin32ObjectOwnerAccessible(node, weakShell, pluginPort);
> >+    NS_ENSURE_TRUE(*aAccessible, NS_ERROR_OUT_OF_MEMORY);
> >+
> >+    NS_ADDREF(*aAccessible);
> >+    return NS_OK;
> 
> Why did you remove the null check for  pluginPort?

pluginPort is HWND, I removed the check to use this class for windowless plugins as well. This class will expose embedded_object role and disabled state for windowless plugins.

Comment 15

[Marco Zehe (:MarcoZ)]

Try server builds: https://build.mozilla.org/tryserver-builds/[e-mail]-try-6f5f14d03f59/

Comment 16

[Marco Zehe (:MarcoZ)]

Hmm I just tested this and see something weird, don't know if that's expected:

1. accProbe shows me an MSAA role of "embed", but an IA2 role of textFrame.

2. NVDA doesn't recognize that the second one should be disabled in the above testcase. I also don't see those states in accProbe.

This is with the above try-server build.

Jamie, is this working for you, and accProbe just gives me the wrong picture for some reason?

Comment 17

[alexander :surkov (:asurkov)]

Marco, I wonder whether you have installed flash player. I just tried local build again and see everything is ok in accprobe. I just assume if there is no installed flash player then we won't create special embed accessible and will create generic text accessible instead.

Comment 18

[Marco Zehe (:MarcoZ)]

I definitely have the latest Flash Player installed and working (v10.032.18). I can start the video just fine from NVDA.

Comment 19

[Marco Zehe (:MarcoZ)]

One more thing:
1. NVDA doesn't see these objects as "embedded objects", whereas with the regular nightly build it does. This may be an adjustment needed for the new situation, but it corresponds to me seeing an IA2 role of TEXT_FRAME instead of EMBED.
2. NVDA doesn't pick up the unavailable state of the second object. Again, this might be an adjustment needed in NVDA, but it's a thing I'm noticing.

Comment 20

[alexander :surkov (:asurkov)]

I think we need some testing from Jamie additionally because neither me nor David can see Marco's results.

Comment 21

[James Teh [:Jamie]]

For me, both objects now get an IA2 role of IA2_ROLE_EMBEDDED_OBJECT. I do see the unavailable state on the transparent object. Unfortunately, NVDA doesn't seem to render the first (non-transparent) object anymore with this build, even though it is present in the a11y tree. This is possibly an NVDA issue, but I haven't quite worked out what is causing the change yet.

Comment 22

[James Teh [:Jamie]]

It seems changing the role to IA2_ROLE_EMBEDDED_OBJECT for an object which has children breaks NVDA. This is definitely an NVDA bug, which I have now fixed. I'm not concerned about breaking older versions of NVDA in this case, as we didn't properly support embedded objects in any previous release. Therefore, I think this patch should go ahead as is; it works for me with this fix to NVDA. It should probably be checked with other ATs as well, though I think they use ISimpleDOM, so it probably won't be an issue.

Marco, not sure what's going on in your testing. :)

Comment 23

[alexander :surkov (:asurkov)]

David, you're turn then.

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

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

r=me thanks! I didn't catch anything that would explain the manual testing weirdness and since Jamie is blessing this one, I think we are good for trunk.

>diff --git a/accessible/public/nsIAccessibleRole.idl b/accessible/public/nsIAccessibleRole.idl
>--- a/accessible/public/nsIAccessibleRole.idl
>+++ b/accessible/public/nsIAccessibleRole.idl
>@@ -772,14 +772,19 @@ interface nsIAccessibleRole : nsISupport
>   /**
>    * Represents a cell within a grid. It is used for role="gridcell". Unlike
>    * ROLE_CELL, it allows the calculation of the accessible name from subtree.
>    * Also, see ROLE_TABLE.
>    */
>   const unsigned long ROLE_GRID_CELL = 121;
> 
>   /**
>+   * Represents an embedded object. It is used for html:object or html:embed.
>+   */
>+  const unsigned long ROLE_EMBEDDED_OBJECT = 122;
>+

Are we ok not revving the uuid because this role is tacked on the end?

aFrame->GetPluginInstance(*getter_AddRefs(pluginInstance));
>   if (pluginInstance) {
>     HWND pluginPort = nsnull;
>     aFrame->GetPluginPort(&pluginPort);
>-    if (pluginPort) {
>-      *aAccessible = new nsHTMLWin32ObjectOwnerAccessible(node, weakShell, pluginPort);
>-      if (*aAccessible) {
>-        NS_ADDREF(*aAccessible);
>-        return NS_OK;
>-      }
>-    }
>+

Can you also add a comment here like "note: pluginPort will be null if windowless"

>+    *aAccessible =
>+      new nsHTMLWin32ObjectOwnerAccessible(node, weakShell, pluginPort);


>+  // nsIAccessibleRole::ROLE_EMBEDDED_OBJECT
>+  { USE_ROLE_STRING, IA2_ROLE_EMBEDDED_OBJECT },
>+

This is okay for this patch... but note I'm working on getting agreement for USE_ROLE_STRING in MSAA... MSAA currently considers this non-spec usage.

Comment 25

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/c42a859b6718

Comment 26

[alexander :surkov (:asurkov)]

additional fixes: mac bustage fix - http://hg.mozilla.org/mozilla-central/rev/8637b7b853a6, and linux/mac mochitest fix - http://hg.mozilla.org/mozilla-central/rev/82d36cd2bb12

Comment 27

[Marco Zehe (:MarcoZ)]

Thanks for taking this one to a closure guys!

Jamie, given the fact that this might impact other ATs as well, would you be OK if we not ported this to 3.6 immediately, but kept it on trunk AKA 3.7 for the time being only so we can evangelize the change?

Comment 28

[James Teh [:Jamie]]

That's fine. We will still work with *most* embedded objects in 3.6, but this just improves the situation somewhat for windowless plugins (and fixes the role for windowed). I actually don't think it will affect other ATs, as I believe most of them use ISimpleDom* (except perhaps System Access), but we should be certain of that first. Might affect Orca, though...

Comment 29

[alexander :surkov (:asurkov)]

One thing we should fix on linux I think is to replace ATK_ROLE_PANEL by ATK_ROLE_EMBEDDED.