Closed Bug 483573 Opened 15 years ago Closed 15 years ago

Expose HTML5 video and audio elements' embedded controls through accessibility APIs.

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: MarcoZ, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access, verified1.9.1)

Attachments

(2 files, 1 obsolete file)

Currently, if an HTML5 audio or video element is used and the @controls attribute is specified, the controls are not exposed to accessibility APIs. They are tabbable, if a screen reader's virtual buffer mode is turned off, and the buttons are announced as buttons without labels, so something is already there, it just needs to be hooked up properly so that

a) the buttons have labels and
b) the virtual buffers of screen readers are able to pick them up.

One possible idea is to expose the video element itself as an nsIAccessibleRole::SCROLL_PANE or something similarly container-ish that groups these controls together, and also will provide space for later inclusion of text captions etc.
However, we might also want to invent a whole new role for this to be included in future IAccessible2 and ATK/AT-SPI specifications for better flexibility.
Flags: wanted1.9.2?
Just one item on the design.  You should probably make sure that whatever you build in doesn't depend on the specific controls implementation.  For example:

http://chrisblizzard.blip.tv/file/1881486/

Shows that it's pretty easy to replace the controls.

What you're talking about above might actually include that, I can't be sure.  But I thought I would bring it up just in case.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #371788 - Flags: review?(marco.zehe)
Attachment #371788 - Flags: review?(david.bolter)
Comment on attachment 371788 [details] [diff] [review]
patch

Nit: I believe we can remove the test for input @type="file" from test_role_nsHyperTextAcc.html, since you're now testing its tree in the new file. To avoid redundancy.
Attachment #371788 - Flags: review?(marco.zehe) → review+
Comment on attachment 371788 [details] [diff] [review]
patch

With NVDA, I now see the buttons. They don't have proper labels yet, but I'm sure that can be rectified in a follow-up bug. Also, the progress bars show way over the top percentage values such as 1437536%, and these numbers increase the more the video progresses. I could even interact with the slider! Very cool!

Note that JAWS, due to its different way of parsing our trees, doesn't see the contents of the video element yet, so testing with NVDA on Windows or Orca on Linux is our best bet right now.

r=me for the tests and the general functionality part.
(In reply to comment #3)
> (From update of attachment 371788 [details] [diff] [review])
> Nit: I believe we can remove the test for input @type="file" from
> test_role_nsHyperTextAcc.html, since you're now testing its tree in the new
> file. To avoid redundancy.

That's fine I think. We test the same but we start from different points. The test we have is about nsHyperTextAccessible roles, the new test is about file input control. I would prefer to save them both. Any way it won't hit us.
(In reply to comment #4)
> (From update of attachment 371788 [details] [diff] [review])
> With NVDA, I now see the buttons. They don't have proper labels yet, but I'm
> sure that can be rectified in a follow-up bug.

Yes, we could put aria-label but I think it's worth to request to put tooltips on them. It will be good for sighted users and for screen readers users.

> Also, the progress bars show way
> over the top percentage values such as 1437536%, and these numbers increase the
> more the video progresses. I could even interact with the slider! Very cool!

That's really cool. Though percentages are strange. I guess we need to fix it.
 
> Note that JAWS, due to its different way of parsing our trees, doesn't see the
> contents of the video element yet, so testing with NVDA on Windows or Orca on
> Linux is our best bet right now.

That's very interesting. Video control is much similar with file control element. Their content is not available from ISimpleDOMNode. But I guess JAWS works successfully with file control element and therefore it sounds strange it doesn't for video control. We have two choices I think 1) extend ISimpleDOMNode to allow to go into native anonymous content 2) contact with JAWS team to figure out what's wrong.

Marco, could you file following up bugs, please, for all stuffs you found?
Attachment #371788 - Flags: superreview?(roc)
Instead of adding a new test video to the tree, it would be better to add a rule to content/media/video/test/Makefile.in to copy one of the videos there to the test directory you want.

+    // If the frame implements nsIAnonymousContentCreator interface then go down
+    // through the frames and obtain anonymous nodes for them.
+    nsIAnonymousContentCreator* creator = do_QueryFrame(mState.frame);

This will descend into the scrollbars of an overflow:auto/scroll frame. Is that really what you want?
(In reply to comment #7)
> Instead of adding a new test video to the tree, it would be better to add a
> rule to content/media/video/test/Makefile.in to copy one of the videos there to
> the test directory you want.

Great. I'll add.

> +    // If the frame implements nsIAnonymousContentCreator interface then go
> down
> +    // through the frames and obtain anonymous nodes for them.
> +    nsIAnonymousContentCreator* creator = do_QueryFrame(mState.frame);
> 
> This will descend into the scrollbars of an overflow:auto/scroll frame. Is that
> really what you want?

Yes. See bug 285167. I didn't start to put mochitests for scrollbar because I wanted to reflect this in following up bug where I'll ensure we expose scrollbars correctly.
(In reply to comment #4)
> Also, the progress bars show way
> over the top percentage values such as 1437536%, and these numbers increase the
> more the video progresses.

I would randomly guess that this is because the raw values in the video controls widgets are not on a 0-100 scale...

The buffering bar sets .max to the file size (in bytes), so .value is literally the number of bytes received so far. (In some cases it can be 0/100 or 100/100, to indicate the extreme points when no other info is known).

The video scrubber sets .max to the media duration in milliseconds, so the indicated .value is the actual time (in milliseconds).

If code is assuming these UI elements always have a 0-100 range, you'd end up with giant percentages.
Comment on attachment 371788 [details] [diff] [review]
patch

>--- a/accessible/src/base/nsAccessibleTreeWalker.cpp
>+++ b/accessible/src/base/nsAccessibleTreeWalker.cpp
>@@ -32,18 +32,21 @@
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsAccessibleTreeWalker.h"
>+
> #include "nsAccessibilityAtoms.h"
> #include "nsAccessNode.h"
>+

Why the line spacing? :)

>   if (aTryFirstChild) {
>-    nsIContent *containerContent = mState.frame->GetContent();
>+    // If the frame implements nsIAnonymousContentCreator interface then go down
>+    // through the frames and obtain anonymous nodes for them.
>+    nsIAnonymousContentCreator* creator = do_QueryFrame(mState.frame);
>     mState.frame = mState.frame->GetFirstChild(nsnull);
>+
>+    if (creator && mState.frame && mState.siblingIndex < 0) {
>+      mState.domNode = do_QueryInterface(mState.frame->GetContent());
>+      mState.siblingIndex = eSiblingsWalkFrames;
>+    }
> // temporary workaround for Bug 359210. We never want to walk frames.
> // Aaron Leventhal will refix :before and :after content later without walking frames.
> #if 0
>     if (mState.frame && mState.siblingIndex < 0) {
>       // Container frames can contain generated content frames from
>       // :before and :after style rules, so we walk their frame trees
>       // instead of content trees
>       // XXX Walking the frame tree doesn't get us Aural CSS nodes, e.g. 
>@@ -249,27 +260,16 @@ void nsAccessibleTreeWalker::UpdateFrame
>       // nsStyleContext *styleContext = primaryFrame->GetStyleContext();
>       // if (aContent) {
>       //   pseudoContext = presContext->StyleSet()->
>       //     ProbePseudoStyleFor(content, nsAccessibilityAtoms::after, aStyleContext);
>       mState.domNode = do_QueryInterface(mState.frame->GetContent());
>       mState.siblingIndex = eSiblingsWalkFrames;
>     }
> #endif

Offtopic: do we have a bug filed for the Aaron Leventhal "refix" mentioned here?


> /**
>+ * Compare expected and actual accessibles trees.
>+ */
>+function testAccessibleTree(aAccOrElmOrID, aAccTree)
>+{
>+  var acc = getAccessible(aAccOrElmOrID);
>+  if (!acc)
>+    return;
>+
>+  for (var prop in aAccTree) {
>+    var msg = "Wrong value of property '" + prop + "'.";
>+    if (prop == "role")
>+      is(roleToString(acc[prop]), roleToString(aAccTree[prop]), msg);
>+    else if (prop != "children")
>+      is(acc[prop], aAccTree[prop], msg);
>+  }
>+
>+  if ("children" in aAccTree) {
>+    var children = acc.children;
>+    is(aAccTree.children.length, children.length,
>+       "Different amount of expected children.");
>+

I wonder... maybe we should bail here if the aAccTree.children.length != children.length ?

(Nice work)
(In reply to comment #10)

> > #include "nsAccessibleTreeWalker.h"
> >+
> > #include "nsAccessibilityAtoms.h"
> > #include "nsAccessNode.h"
> >+
> 
> Why the line spacing? :)

to improve readability, grouping headers logically

> Offtopic: do we have a bug filed for the Aaron Leventhal "refix" mentioned
> here?
> 

yes, bug 346833

> 
> I wonder... maybe we should bail here if the aAccTree.children.length !=
> children.length ?
> 

Ok.
Attached patch patch2Splinter Review
Attachment #371788 - Attachment is obsolete: true
Attachment #373248 - Flags: superreview?(roc)
Attachment #373248 - Flags: review?(david.bolter)
Attachment #371788 - Flags: superreview?(roc)
Attachment #371788 - Flags: review?(david.bolter)
Attachment #373248 - Flags: superreview?(roc) → superreview+
Attachment #373248 - Flags: review?(david.bolter) → review+
Comment on attachment 373248 [details] [diff] [review]
patch2

Woot! Let's get this baking on the tree.
Pushed upon Alexander's request in changeset:
http://hg.mozilla.org/mozilla-central/rev/cb56ac937481
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #373248 - Flags: approval1.9.1?
Depends on: 489306
Flags: in-testsuite+
Marco, please file following up bugs.
Depends on: 489549
Depends on: 489551
Comment on attachment 373248 [details] [diff] [review]
patch2

What's the impact of changing this API on screen readers? Is the new method being added at the bottom to not affect the vtable?
(In reply to comment #16)
> (From update of attachment 373248 [details] [diff] [review])
> What's the impact of changing this API on screen readers?

This patch makes accessible video/audio control elements (like play button, progress meters) accessible to screen readers. Otherwise screen readers users might be not able to control video/audio element.

> Is the new method
> being added at the bottom to not affect the vtable?

I didn't think about vtable and it sounds I didn't add new method at the bottom. Could you give me please additional details what I need to keep in mind when I write patches backported to mozilla branches?
cc'ing Mike
Well, you're sort of hosed either way.  Changing the IID will break anyone properly acquiring pointers to objects implementing the interface (because their QueryInterface calls will use the old IID, and you won't respond to it, so they'll get nulls or throws when they try), but not changing it will break anyone who uses an interface which inherits from that interface.  I doubt that's a concern for a11y, and I doubt it's a concern most of the time (only for interfaces which are designed to be implemented by multiple classes, and by classes outside Gecko), but it theoretically does matter for exact binary compatibility.

If you don't change the IID, which would only break those specific cases but not existing users, tho, you should add the method to the end of the interface so that the virtual table (containing all the function pointers for the interface in an object implementing it) isn't reordered, which would result in calling the wrong function with the wrong arguments.  Even if you do change the IID it may be worthwhile to do this, just to keep the number of changes made here smaller.

That's probably not quite clear, so if you don't understand it please ask for any clarifications necessary.
So, on one hand we must make video controls accessible, on another hand we should broke anyone. Though nsIAccessibleService is designed for internal usage only (to be called from layout to create accessible and invalidate a11y tree). So it shouldn't be used outside of mozilla. But we can't be sure it's not used. Jeff suggested to get rid nsIAccessibleService interface at all and use C++ calls inside of mozilla. But it's long term. So what will we decide?
So, talked to Jeff and David Baron on irc, David suggested to change IID (different than trunk's one) and put method to the bottom.
Is this really relevant still, since we're talking about backporting this to 1.9.1 only, which isn't in a released state yet. It's not like we're asking to backport it to 1.9.0, which has been in a released state for almost a year.
Attached patch patch 1.9.1Splinter Review
Attachment #374236 - Flags: approval1.9.1?
Attachment #373248 - Flags: approval1.9.1?
test_elm_media.html fails for me on OS X:

Passed: 1
Failed: 1
Todo: 0
ok - Wrong value of property 'role'.
not ok - Different amount of expected children. got 5, expected 6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Update your tree and retest? bz landed a fix for the test as a result of bug 475318 breaking it, should be working now. See http://hg.mozilla.org/mozilla-central/rev/a0b5b3125f60
no failures for me, marking as fixed again.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Depends on: 490287
marco, can you verify this fix on trunk?  Thanks.
Just to follow up on Justin's comment #25, yeah tests pass for me now thanks.
(In reply to comment #27)
> marco, can you verify this fix on trunk?  Thanks.

This is verified in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090503 Minefield/3.6a1pre (.NET CLR 3.5.30729)
Status: RESOLVED → VERIFIED
Blocks: 492516
Comment on attachment 374236 [details] [diff] [review]
patch 1.9.1

a191=beltzner
Attachment #374236 - Flags: approval1.9.1? → approval1.9.1+
Verified fixed on 1.9.1 in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090517 Shiretoko/3.5b5pre (.NET CLR 3.5.30729)
Flags: wanted1.9.2?
You need to log in before you can comment on or make changes to this bug.