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

VERIFIED FIXED

Status

()

VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: MarcoZ, Assigned: surkov)

Tracking

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

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Comment 1

10 years ago
Posted patch patchSplinter Review
Marco, this should help.

Firefox 3.1 introduces new attribute "max" on progressmeter that is used inside of videocontrols.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #374452 - Flags: review?(marco.zehe)
(Assignee)

Updated

10 years ago
Attachment #374452 - Flags: review?(david.bolter)
(Assignee)

Comment 2

10 years ago
In the patch I didn't updated bug info in mochitest, I did this locally
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?
(Assignee)

Comment 4

10 years ago
(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 ;)
(Reporter)

Comment 5

10 years ago
Comment on attachment 374452 [details] [diff] [review]
patch

Tested with a video with controls inline, and it works nice. r=me
Attachment #374452 - Flags: review?(marco.zehe) → review+
Comment on attachment 374452 [details] [diff] [review]
patch

OK r=me
Attachment #374452 - Flags: review?(david.bolter) → review+
(Assignee)

Updated

10 years ago
Attachment #374452 - Flags: approval1.9.1?
(Assignee)

Updated

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

Comment 7

10 years ago
http://hg.mozilla.org/mozilla-central/rev/f0f3349148f7
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 8

10 years ago
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)
Status: RESOLVED → VERIFIED
(Assignee)

Updated

10 years ago
Blocks: 492516
Attachment #374452 - Flags: approval1.9.1? → approval1.9.1+
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.
Keywords: fixed1.9.1
(Reporter)

Comment 10

10 years ago
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
You need to log in before you can comment on or make changes to this bug.