489551 - Values of sliders and progress bars in HTML 5 audio and video element's control sets are not percentages

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

In https://bugzilla.mozilla.org/show_bug.cgi?id=483573#c9, Justin Dolske writes:

>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.

I suggest this course of action:
1. Calculate percentages properly and make sure the accValue exposes these so screen readers get accurate values in a 0 to 100 range.

2. In the iAccessibleValue interface and the ATK pendant, feed the valuemin, valuemax and valuetext properties (like would be done for ARIA sliders) with the actual value as Justin describes them. We could think whether it makes sense to expose these as hour:minute:second etc. values. I think milliseconds don't actually make much sense when watching a video, unless you want to edit them, which is not within the scope of exposing this element.

Thoughts?

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Marco, this should help.

Firefox 3.1 introduces new attribute "max" on progressmeter that is used inside of videocontrols.

Comment 2

[alexander :surkov (:asurkov)]

In the patch I didn't updated bug info in mochitest, I did this locally

Comment 3

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

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

>+nsXULProgressMeterAccessible::GetMaximumValue(double *aMaximumValue)
> {
>+  nsresult rv = nsFormControlAccessible::GetMaximumValue(aMaximumValue);
>+  if (rv != NS_OK_NO_ARIA_VALUE)
>+    return rv;
>+
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>+
>+  nsAutoString value;
>+  if (content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::max, value)) {

I'd sleep easier with: if (content && content->...

>-NS_IMETHODIMP nsXULProgressMeterAccessible::GetCurrentValue(double *aCurrentValue)
>+NS_IMETHODIMP
>+nsXULProgressMeterAccessible::GetCurrentValue(double *aCurrentValue)
> {
>-  *aCurrentValue = 0;
>+  nsresult rv = nsFormControlAccessible::GetCurrentValue(aCurrentValue);
>+  if (rv != NS_OK_NO_ARIA_VALUE)
>+    return rv;
>+
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>+
>   nsAutoString currentValue;
>-  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>-  if (!content) {
>-    return NS_ERROR_FAILURE;
>-  }

Why do we trust nsIContent support now?

>   content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::value, currentValue);

I didn't catch any problems. Are all the touched code paths under test coverage?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)

> I'd sleep easier with: if (content && content->...
> Why do we trust nsIContent support now?

It's progressmeter, there is no possible document stuffs (where nsIContent isnt' implemented), nsAccessile methods check node on defunct. So it must be safe. Excess check aren't good, so I'm trying to remove them where it's possible.

> 
> I didn't catch any problems. Are all the touched code paths under test
> coverage?

sounds so, tested on windows. But we can't brake test coverage because progressmeter isn't tested at all there ;)

Comment 5

[Marco Zehe (:MarcoZ)]

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

Tested with a video with controls inline, and it works nice. r=me

Comment 6

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

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

OK r=me

Comment 7

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/rev/f0f3349148f7

Comment 8

[Marco Zehe (:MarcoZ)]

Verified fixed 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)

Comment 9

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

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

a191=beltzner

Marco already landed this as:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2dbfcd5332026e5aa34cf5b15970c926cae5cf19

which is fine, I'd given him the a+ in email ahead of time.

Comment 10

[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)