490287 - slider of audio/video element hasn't accessible name

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Would be nice to expose name by aria-labelledby but we need to change code to handle anonid in aria-labelledby inside XBL binding

Comment 1

[alexander :surkov (:asurkov)]

Attachment: wip

Patch (it doesn't use locale string, I'll fix it once we get the bug 489549 is landed). Patch uses idea of bug 337680. I need to ensure it goes with you. I think this is the case when XBL binding accessible properties implementation is the easiest way we can choose.

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 375145 [details] [diff] [review]
wip

r=me. Cool, straight-forward approach!

Comment 3

[Marco Zehe (:MarcoZ)]

Hoping to get this into 1.9.1 when it lands successfully in mozilla-central. But because of these new strings, we might have to find a different solution for branch because Firefox 3.5 is in string freeze already.

Comment 4

[Axel Hecht [:Pike]]

We probably need to figure out how to deal with the plural questions that arise here, together with the string concat issues.

Comment 5

[Marco Zehe (:MarcoZ)]

Alex, can you come back to this one soon? it's important to get this resolved for possible inclusion in Firefox 3.5.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 7

[Neil Deakin]

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

This seems OK, but I don't think it will localize properly.

A string like 'something X of Y' may need to be written 'X of Y something' or even 'Y of X'.

The text is english is also a bit odd. Maybe remove the word 'Play' from startLabel.

Comment 8

[Axel Hecht [:Pike]]

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

Yes, this will most certainly break l10n.

Please use a .properties file, together with plurals, https://developer.mozilla.org/en/Localization_and_Plurals#Developing_with_PluralForm. I confess, I don't think that somebody tried that from XBL yet.

Things that break are the order of the placables as well as the lack of a trailing string, even if it'd be empty in English. Plural forms should be able to cover both.

Comment 9

[Marco Zehe (:MarcoZ)]

I just built with this patch and don't see NVDA render the accessible name for these sliders at all. So in theory it looks good, and the tests pass as well, but the practice doesn't do it justice. :-(

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> (From update of attachment 376866 [details] [diff] [review])
> Yes, this will most certainly break l10n.
> 
> Please use a .properties file, together with plurals,
> https://developer.mozilla.org/en/Localization_and_Plurals#Developing_with_PluralForm.

I can't use .properties files (because I can't use string bundles) and I think it shouldn't be able to use plural component because of security (as Neil said videocontrols ran with page privileges, which would prevent them accessing components).

Comment 11

[alexander :surkov (:asurkov)]

Marco, what's the best accessible name for progress slider?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> I just built with this patch and don't see NVDA render the accessible name for
> these sliders at all. So in theory it looks good, and the tests pass as well,
> but the practice doesn't do it justice. :-(

I think it's because we fire focus event on internal xul:slider element which is not accessible, at least this is the thing accprobe shows me.

Comment 13

[Axel Hecht [:Pike]]

I'm surprised that a DTD works but a <stringbundle> element wouldn't, but I take it you tried?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> I'm surprised that a DTD works but a <stringbundle> element wouldn't, but I
> take it you tried?

Sure, I tried this in bug 489549 where I was forced to switch to DTD (see bug 489549 comment #10). If I get right the reason is DTD doesn't require security privileges in contrast to component usage.

Comment 15

[Axel Hecht [:Pike]]

Man, that's backwards. I suggest that you do a single string then, and use js to replace values. Something like 
<!ENTITY scrubberScale.label "Play progress is #1 of #2">
with a localization note what each is going to be replaced with is probably best then.

Comment 16

[alexander :surkov (:asurkov)]

It's fine with me.

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 18

[[e-mail]]

Comment on attachment 376904 [details] [diff] [review]
patch2

>+ * This interface is used to change accessible properties of accesible created
Nit: typo (but you got it right the first time!)

>+          // Be careful to localize "scubberLabel" attribute properly, it should
>+          // be a string with '#1' and '#2' inclusions to point current
>+          // progress and duration time of the record.
This belongs as a LOCALIZATION NOTE in the .dtd file.

>+          var match = 0;
>+          function replacer()
>+          {
>+            match++;
>+            if (match == 1)
>+              return currTime;
>+            else if (match == 2)
>+              return totalTime;
>+            return "";
>+          }
>+
>+          return label.replace(/#\d/g, replacer);
I'm not sure what this is trying to achieve. All you really need is
return label.replace(/#1/, currTime).replace(/#2/, totalTime);

>                     this.durationLabel.setAttribute("value", timeString);
>+                    this.scrubber.setAttribute("durationLabel", timeString);
Could comment that this is for a11y, otherwise people might wonder?

Comment 19

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

Comment on attachment 376904 [details] [diff] [review]
patch2

Thanks Alex. My initial comments:

>+/**
>+ * This interface is used to change accessible properties of accesible created
>+ * for control having XBL binding if XBL binding implements it.

How about: "Controls can implement this XBL binding interface to provide access to accessible properties."


>           { // slider of progress bar
>-            role: ROLE_SLIDER
>+            role: ROLE_SLIDER,
>+            name: "0:00 of 0:01 progress"

Marco, is this a good name?

>+          var match = 0;
>+          function replacer()
>+          {
>+            match++;

Keeping count of the matches seems unfortunate but I don't have a great alternate suggestion. Note the .replace docs tend to use 'match' as the param name to the replacer function so it might through the code reader off. You could change it to matchCount is you agree.

Maybe Axel or one of the Neils have a suggestion for how to do the label, and updating?

>                     this.durationLabel.setAttribute("value", timeString);
>+                    this.scrubber.setAttribute("durationLabel", timeString);

I guess we need this duplication...

Comment 20

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

(In reply to comment #18)
> (From update of attachment 376904 [details] [diff] [review])
> >+
> >+          return label.replace(/#\d/g, replacer);
> I'm not sure what this is trying to achieve. All you really need is
> return label.replace(/#1/, currTime).replace(/#2/, totalTime);

Ah yes. Upon closer inspection I see Alex was just replacing the original label each time. I had assumed he was updating the label based on its latest value. Ignore my comment here.

Alex if you use this you can get rid of your replacer :)

> 
> >                     this.durationLabel.setAttribute("value", timeString);
> >+                    this.scrubber.setAttribute("durationLabel", timeString);
> Could comment that this is for a11y, otherwise people might wonder?

Good idea.

(Sorry my comments seem redundant as I hadn't read Neil's and I didn't get a collision because I had to login to submit mine)

Comment 21

[Marco Zehe (:MarcoZ)]

Comment on attachment 376904 [details] [diff] [review]
patch2

This one doesn't work. It throws an unknown object at NVDA it doesn't know what to do with, and the accessible name is nowhere to be found.

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 376904 [details] [diff] [review]
patch2

A few additional driveby comments:

>+          var currTime = this.thumb.timeLabel.getAttribute("value");
>+          var totalTime = this.getAttribute("durationLabel");
>+          var label = this.getAttribute("scrubberLabel");

I think you should try to use <field>s on the binding for some of these, instead of just sticking extra attributes onto the bound element.

Also, note that this binding is also attached to the volume slider. So you'll either need to add guards to prevent making it look like the position scrubber, or add if/else support for making the volume slider properly accessible. 

Nit: this file users 4-space indents.

>+                        <scale class="scrubber" movetoclick="true"
>+                               scrubberLabel="&scrubberScale.label;"/>

"scrubberLabel" seems misleading, since it's not actually a label in the UI.

But I think this can just become something like <field name="fooFormat">&fooFormat;</field>

>+                    this.scrubber.setAttribute("durationLabel", timeString);

Again, make this a field and use something like this.scrubber.durationString?

[I'll note that we've been forced to use get/setAttribute in the past because accessing values as .properties was throwing security errors in some cases; but I think that's fixed now.]

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> (From update of attachment 376904 [details] [diff] [review])
> This one doesn't work. It throws an unknown object at NVDA it doesn't know what
> to do with, and the accessible name is nowhere to be found.

Marco, I believe you talk about bug 492518. Let's deal with them separately. The problem is xul:scale (slider accessible) is not focusable and child xul:slider (generic accessible) is focusable. We put accessible name on xul:scale not on xul:slider. Once we fix this bug and bug 492518 then NVDA should work.

Comment 24

[alexander :surkov (:asurkov)]

Attachment: patch3

I hope I addressed reviewer's comments.

Comment 25

[Marco Zehe (:MarcoZ)]

Comment on attachment 377080 [details] [diff] [review]
patch3

Two nits:
>+      <field name="dutationValue">""</field>

Should be "durationValue", r instead of t after the u.

>+<!ENTITY scrubberScale.label "#1 of #2 progress">

I believe "progress" should be replaced with "elapsed", unless others disagree.

r=me with that and the explanation from comment #23.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> (From update of attachment 377080 [details] [diff] [review])
> Two nits:
> >+      <field name="dutationValue">""</field>
> 
> Should be "durationValue", r instead of t after the u.
> 
> >+<!ENTITY scrubberScale.label "#1 of #2 progress">
> 
> I believe "progress" should be replaced with "elapsed", unless others disagree.

fixed locally

Comment 27

[Justin Dolske [:Dolske]]

Comment on attachment 377080 [details] [diff] [review]
patch3

A few small nits, r+ with these fixed:

>+    <implementation implements="nsIXBLAccessible">
>+      <!-- nsIXBLAccessible -->
>+      <property name="accessibleName" readonly="true">
>+        <getter>
>+          if (this.type != "scrubber")

Indents here should be 4-spaces per level, not 2.

>+          return this.scrubberName.replace(/#1/, currTime).
>+            replace(/#2/, totalTime);

Formatting:

    return this.scrubberName.replace(/#1/, currTime)
                            .replace(/#2/, totalTime);

>+      <field name="scrubberName">"&scrubberScale.label;"</field>

I think it would be clearer to include "format" in the fieldname and entity, so it's clearer that this is just the localized format string and not a finished value -- it really only exists as an L10N implementation artifact. "scrubberNameFormat", if I may paint this bikeshed...

>--- a/toolkit/locales/en-US/chrome/global/videocontrols.dtd
...
>+<!-- Be careful to localize "scubberLabel" properly, it should be a string with
>+'#1' and '#2' inclusions to point current progress and duration time of the
>+record.
>+-->
>+<!ENTITY scrubberScale.label "#1 of #2 progress">

Comment doesn't match the entity name. Rephrase to remove any ambiguity about which number is what. EG:
---
Localization note: the #1 string is the current media position, and the #2 string is the total duration. For example, when at the 5 minute mark in a 6 hour long video, #1 would be "5:00" and #2 would be "6:00:00"
---

I also agree that "#1 of #2 elapsed" is better en-US phrasing.

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch4

with Marco and Justin comments

Comment 29

[Axel Hecht [:Pike]]

Comment on attachment 377092 [details] [diff] [review]
patch4

r=me with a format change to the localization note, those are in a format that many of our tools can read, https://developer.mozilla.org/En/Localization_notes.

<!-- LOCALIZATION NOTE (scrubberScale.nameFormat): the #1 string is the current media position, and the #2 string is the total duration. For example, when at the 5 minute mark in a 6
hour long video, #1 would be "5:00" and #2 would be "6:00:00", result string would be "5:00 of 6:00:00 elapsed". -->

Comment 30

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

Comment on attachment 377092 [details] [diff] [review]
patch4

This one is good. r=me

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

We need a different superreviewer here, Enn is away until the 25th.

Comment 32

[Mike Connor [:mconnor]]

Comment on attachment 377092 [details] [diff] [review]
patch4

r=me, looks good.

Comment 33

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/rev/d5b86378d271 + followin up localization note fix - http://hg.mozilla.org/mozilla-central/rev/d655dc32a988

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 377092 [details] [diff] [review]
patch4

a191=beltzner

Comment 35

[Axel Hecht [:Pike]]

This is landing on the branch today, right?

Comment 36

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> This is landing on the branch today, right?

I hope. It's next. Now I'm rebuilding firefox to ensure it works.

Comment 37

[alexander :surkov (:asurkov)]

Attachment: patch for 1.9.1

Comment 38

[alexander :surkov (:asurkov)]

checked in on 1.9.1. - http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1e44b11f515e

Comment 39

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090517 Minefield/3.6a1pre (.NET CLR 3.5.30729)

Comment 40

[Marco Zehe (:MarcoZ)]

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)