slider of audio/video element hasn't accessible name

VERIFIED FIXED

Status

()

VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access, verified1.9.1})

unspecified
access, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

10 years ago
Would be nice to expose name by aria-labelledby but we need to change code to handle anonid in aria-labelledby inside XBL binding
(Assignee)

Comment 1

10 years ago
Posted patch wip (obsolete) — Splinter Review
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.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #375145 - Flags: review?(marco.zehe)

Updated

10 years ago
Attachment #375145 - Flags: review?(marco.zehe) → review+
Comment on attachment 375145 [details] [diff] [review]
wip

r=me. Cool, straight-forward approach!
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

10 years ago
We probably need to figure out how to deal with the plural questions that arise here, together with the string concat issues.
Alex, can you come back to this one soon? it's important to get this resolved for possible inclusion in Firefox 3.5.
(Assignee)

Comment 6

10 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #375145 - Attachment is obsolete: true
Attachment #376866 - Flags: superreview?(neil)
Attachment #376866 - Flags: review?(marco.zehe)
Attachment #376866 - Flags: review?(enndeakin)
Attachment #376866 - Flags: review?(dolske)
Attachment #376866 - Flags: review?(david.bolter)

Comment 7

10 years ago
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.

Updated

10 years ago
Attachment #376866 - Flags: review-

Comment 8

10 years ago
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.
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. :-(
(Assignee)

Comment 10

10 years ago
(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).
(Assignee)

Comment 11

10 years ago
Marco, what's the best accessible name for progress slider?
(Assignee)

Comment 12

10 years ago
(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.
(Assignee)

Updated

10 years ago
Blocks: 492516
(Assignee)

Updated

10 years ago
Depends on: 492518
I'm surprised that a DTD works but a <stringbundle> element wouldn't, but I take it you tried?
(Assignee)

Comment 14

10 years ago
(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.
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.
(Assignee)

Comment 16

10 years ago
It's fine with me.
(Assignee)

Comment 17

10 years ago
Posted patch patch2 (obsolete) — Splinter Review
Attachment #376866 - Attachment is obsolete: true
Attachment #376904 - Flags: superreview?(neil)
Attachment #376904 - Flags: review?(marco.zehe)
Attachment #376904 - Flags: review?(l10n)
Attachment #376904 - Flags: review?(enndeakin)
Attachment #376904 - Flags: review?(david.bolter)
Attachment #376866 - Flags: superreview?(neil)
Attachment #376866 - Flags: review?(marco.zehe)
Attachment #376866 - Flags: review?(enndeakin)
Attachment #376866 - Flags: review?(dolske)
Attachment #376866 - Flags: review?(david.bolter)
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 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...
(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 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.
Attachment #376904 - Flags: review?(marco.zehe) → review-
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.]
Attachment #376904 - Flags: review-
(Assignee)

Comment 23

10 years ago
(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.
(Assignee)

Comment 24

10 years ago
Posted patch patch3 (obsolete) — Splinter Review
I hope I addressed reviewer's comments.
Attachment #376904 - Attachment is obsolete: true
Attachment #377080 - Flags: superreview?(neil)
Attachment #377080 - Flags: review?(marco.zehe)
Attachment #377080 - Flags: review?(l10n)
Attachment #377080 - Flags: review?(enndeakin)
Attachment #377080 - Flags: review?(dolske)
Attachment #377080 - Flags: review?(david.bolter)
Attachment #376904 - Flags: superreview?(neil)
Attachment #376904 - Flags: review?(l10n)
Attachment #376904 - Flags: review?(enndeakin)
Attachment #376904 - Flags: review?(david.bolter)
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.
Attachment #377080 - Flags: review?(marco.zehe) → review+
(Assignee)

Comment 26

10 years ago
(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
Attachment #377080 - Flags: review?(dolske) → review+
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.
(Assignee)

Comment 28

10 years ago
Posted patch patch4Splinter Review
with Marco and Justin comments
Attachment #377080 - Attachment is obsolete: true
Attachment #377092 - Flags: superreview?(neil)
Attachment #377092 - Flags: review?(l10n)
Attachment #377092 - Flags: review?(enndeakin)
Attachment #377092 - Flags: review?(david.bolter)
Attachment #377080 - Flags: superreview?(neil)
Attachment #377080 - Flags: review?(l10n)
Attachment #377080 - Flags: review?(enndeakin)
Attachment #377080 - Flags: review?(david.bolter)

Updated

10 years ago
Attachment #377092 - Flags: superreview?(neil) → superreview+
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". -->
Attachment #377092 - Flags: review?(l10n) → review+
Comment on attachment 377092 [details] [diff] [review]
patch4

This one is good. r=me
Attachment #377092 - Flags: review?(david.bolter) → review+
We need a different superreviewer here, Enn is away until the 25th.
Comment on attachment 377092 [details] [diff] [review]
patch4

r=me, looks good.
Attachment #377092 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 33

10 years ago
http://hg.mozilla.org/mozilla-central/rev/d5b86378d271 + followin up localization note fix - http://hg.mozilla.org/mozilla-central/rev/d655dc32a988
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Flags: in-testsuite+
(Assignee)

Updated

10 years ago
Attachment #377092 - Flags: approval1.9.1?
Attachment #377092 - Flags: approval1.9.1? → approval1.9.1+
This is landing on the branch today, right?
(Assignee)

Comment 36

10 years ago
(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.
(Assignee)

Comment 37

10 years ago
(Assignee)

Comment 38

10 years ago
checked in on 1.9.1. - http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1e44b11f515e
Keywords: access, fixed1.9.1
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)
Status: RESOLVED → VERIFIED
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)
Keywords: fixed1.9.1 → verified1.9.1

Updated

10 years ago
Depends on: 494345

Updated

10 years ago
Depends on: 494346
You need to log in before you can comment on or make changes to this bug.