Last Comment Bug 559773 - make HTML5 progress element accessible
: make HTML5 progress element accessible
Status: VERIFIED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: 7 Branch
: All All
: -- normal with 2 votes (vote)
: mozilla6
Assigned To: Marco Zehe (:MarcoZ) on PTO until August 15
:
Mentors:
http://dev.w3.org/html5/spec/forms.ht...
Depends on: 514437
Blocks: html5a11y 633207
  Show dependency treegraph
 
Reported: 2010-04-16 00:19 PDT by alexander :surkov
Modified: 2011-07-11 23:17 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP patch (15.97 KB, patch)
2011-05-09 04:01 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
no flags Details | Diff | Splinter Review
WIP (doesn't compile) (18.27 KB, patch)
2011-05-10 05:57 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
no flags Details | Diff | Splinter Review
Patch, but doesn't work completely yet (18.95 KB, patch)
2011-05-10 10:29 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
no flags Details | Diff | Splinter Review
patch (23.15 KB, patch)
2011-05-11 08:39 PDT, alexander :surkov
mzehe: review+
Details | Diff | Splinter Review
fix linux build (23.48 KB, patch)
2011-05-11 21:09 PDT, Trevor Saunders (:tbsaunde)
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
landed patch with ProgressMeterAccessible constructor reindented (602 bytes, patch)
2011-05-12 04:17 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
changed the indentation (656 bytes, patch)
2011-05-12 04:35 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review

Description alexander :surkov 2010-04-16 00:19:22 PDT
Should be similar to XUL analogue. Should fire value change events and implement nsIAccessibleValue.
Comment 1 David Bolter [:davidb] 2010-06-17 12:54:59 PDT
Yep, looks like patches are r+'ed over there.
Comment 2 Mounir Lamouri (:mounir) 2011-05-05 06:45:15 PDT
I would like to have <progress> in Firefox 6 and we are only a few patches away. Could someone work on the a11y part? How can I help with that?

My patch queue is available here: https://hg.mozilla.org/users/mlamouri_mozilla.com/html5forms-patchqueue/

I'm going to send something to try so you guys can test it.
Comment 3 Marco Zehe (:MarcoZ) on PTO until August 15 2011-05-05 07:12:53 PDT
Yes, a try-server build would be  good, and also if you have a page that demonstrates the feature, this would be helpful as well. David, was there not talk about unifying our progress support for XUL and HTML along this way? Do you or has Alex started anything on that front?
Comment 4 Mounir Lamouri (:mounir) 2011-05-06 03:22:18 PDT
Try server builds are available here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mlamouri@mozilla.com-b90e2723adad/

You can check this page for <progress> examples:
http://oldworld.fr/mozilla/progress.html
Comment 5 Marco Zehe (:MarcoZ) on PTO until August 15 2011-05-09 00:01:43 PDT
From what I see from the spec, here are the things we need to do:

1. Create an accessible for the <progress> element and support its role.

2. Support its getting of a value. This is already being done by the nsFormcontrolAttribute I believe, but we need the logic as in nsXULProgressMeterAccessible to get the correct percentage. Need to also see if the max attribute is specified.

3. Support the max attribute for the maximum value of the nsIAccessibleValue interface. The minimum value cannot be specified here, so I believe this is always 0.0 and we should efault to returning that as the minimum value in the same interface.

4. Question: In the spec , there's also a form attribute specified to tell which form this <progress> element belongs to. Should we implement a relation here like RELATION_CONTROLLED_BY or the like?

5. Make sure to not expose child text to screen readers that would be shown by legacy browsers. We currently don't do it, but if we create an accessible for the <progress> element, this may change.

From what I can see, we can use a lot of code from nsXULProgressMeterAccessible for this.
Comment 6 Marco Zehe (:MarcoZ) on PTO until August 15 2011-05-09 00:02:37 PDT
Forgot to paste the spec link I used for the above suggestions: http://www.w3.org/wiki/HTML/Elements/progress
Comment 7 Marco Zehe (:MarcoZ) on PTO until August 15 2011-05-09 01:23:36 PDT
I just started to work on this, and found that I cluld completely eliminate nsXULProgressMeterAccessible, since all of its logic is what is also needed for the HTML5 <progress> element. I'm wiping up a patch I'm going to attach to the bug (note that this may even work without the actual content/layout changes!).
Comment 8 Mounir Lamouri (:mounir) 2011-05-09 03:19:20 PDT
(In reply to comment #5)
> 2. Support its getting of a value. This is already being done by the
> nsFormcontrolAttribute I believe, but we need the logic as in
> nsXULProgressMeterAccessible to get the correct percentage. Need to also see
> if the max attribute is specified.

nsHTMLProgressElement::GetPosition returns the correct percentage (from 0.0 to 1.0).

> 3. Support the max attribute for the maximum value of the nsIAccessibleValue
> interface. The minimum value cannot be specified here, so I believe this is
> always 0.0 and we should efault to returning that as the minimum value in
> the same interface.

There is no concept of minimum value for the progress element so if you need such a thing, 0 seems correct. Though, I'm not sure I follow why it's needed.

> 4. Question: In the spec , there's also a form attribute specified to tell
> which form this <progress> element belongs to. Should we implement a
> relation here like RELATION_CONTROLLED_BY or the like?

I guess so. It's like label elements: they are form associated but not really that related to the form. I mean you can't submit their values nor reset them. There is a spec bug related to this issue: http://www.w3.org/Bugs/Public/show_bug.cgi?id=12254

(In reply to comment #7)
> I just started to work on this, and found that I cluld completely eliminate
> nsXULProgressMeterAccessible, since all of its logic is what is also needed
> for the HTML5 <progress> element. I'm wiping up a patch I'm going to attach
> to the bug (note that this may even work without the actual content/layout
> changes!).

That's great :) I would like to push the progress element work to the tree but I also want it to be accessible when shipped (with Firefox 6 obviously). Do you think your patch might be ready for the branching?
Comment 9 Marco Zehe (:MarcoZ) on PTO until August 15 2011-05-09 03:49:44 PDT
(In reply to comment #8)
> nsHTMLProgressElement::GetPosition returns the correct percentage (from 0.0
> to 1.0).

Hm, doesn't percentage always go from 0 to 100? And what about your example where the current value is 21 where the max is 42?

> There is no concept of minimum value for the progress element so if you need
> such a thing, 0 seems correct. Though, I'm not sure I follow why it's needed.

ARIA allows to determine a minimum value. Since the interface needs to expose the minimum value getter, we must return *something*. With XUL it's similar, it doesn't support a minimum value natively, either, so we return 0 there, too.

> I guess so. It's like label elements: they are form associated but not
> really that related to the form. I mean you can't submit their values nor
> reset them. There is a spec bug related to this issue:
> http://www.w3.org/Bugs/Public/show_bug.cgi?id=12254

OK, let's ignore it for now, we don't support @form on labels currently, either.

> Do you think your patch might be ready for the branching?

I'm going to attach a patch in a few minutes. I'm having trouble with the tests actually running and need help from my fellow a11y devs on this, but other than that, I believe this is as simple as making the class we previously had for XUL Progress Meters only available to also work with <progress>.
Comment 10 Marco Zehe (:MarcoZ) on PTO until August 15 2011-05-09 04:01:45 PDT
Created attachment 531008 [details] [diff] [review]
WIP patch

For some reason, this breaks all the tests that somehow access nsIAccessibleValue I think. Those test files start and finish without any test pass, test fail or other such output, within a range of 50 to 100 ms after starting. Even those that I didn't touch like test_value.html and test_value.xul. However, with NVDA, I actually get values and an accessible <progress> element! Don#t quite understand what's going on. So I'd like to request help on this.
Comment 11 Mounir Lamouri (:mounir) 2011-05-09 04:03:31 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > nsHTMLProgressElement::GetPosition returns the correct percentage (from 0.0
> > to 1.0).
> 
> Hm, doesn't percentage always go from 0 to 100?

HTML specs require progress.position to return a value from 0.0 to 1.0. If you want a percentage, you can just do position*100. I guess it's like that because it's more useful to get a value from 0.0 to 1.0 to do some computations. For human readability, percentage are better I guess ;)

> And what about your example where the current value is 21 where the max is 42?

progress.position will return 0.5.
Comment 12 alexander :surkov 2011-05-09 11:20:29 PDT
(In reply to comment #10)
> Created attachment 531008 [details] [diff] [review] [review]
> WIP patch
> 
> For some reason, this breaks all the tests that somehow access
> nsIAccessibleValue I think.

You don't change anything that could affect on ARIA widgets, so I assume something wrong with your test, for example, you missed the test output part.

nit: nsProgressMeterAccessible -> ProgressMeterAccessible

NS_DECL_NSIACCESSIBLEVALUE and nsISupports should be public.
Comment 13 Marco Zehe (:MarcoZ) on PTO until August 15 2011-05-10 05:57:38 PDT
Created attachment 531307 [details] [diff] [review]
WIP (doesn't compile)

Here's what's in this patch:

1. Found out why the tests didn't work and made them working (or at least running).
2. Found that, according to https://developer.mozilla.org/En/XUL/Attribute/progressmeter.max, the default max value for the XUL:progressmeter element is 100, not 1, as for html:progress.
3. To that end, I created what I think should become a template class where we can simply pass in the default maximum value for HTML or XUL from nsAccessibilityService.

And with the declaration and definition of that class I have some trouble. Please see the patch.

The .h file is fine, but the .cpp file barfs on line 51 with an "undeclared identifier", and on line 52 with a "needs template param declaration" compiler error.

How do I declare this correctly? I didn't find any example digging through code elsewhere that I thought applied here.
Comment 14 Mounir Lamouri (:mounir) 2011-05-10 06:09:04 PDT
Comment on attachment 531307 [details] [diff] [review]
WIP (doesn't compile)

>diff --git a/accessible/src/base/nsAccessibilityService.cpp b/accessible/src/base/nsAccessibilityService.cpp
>     case nsIAccessibleProvider::XULProgressMeter:
>-      accessible = new nsXULProgressMeterAccessible(aContent, aWeakShell);
>+      accessible = new ProgressMeterAccessible<100>(aContent, aWeakShell);
>       break;

>+  if (tag == nsAccessibilityAtoms::progress) {
>+    nsAccessible* accessible = new ProgressMeterAccessible<1>(aContent, aWeakShell);
>+    NS_IF_ADDREF(accessible);
>+    return accessible;
>+  }
>+
>   return nsnull;
>  }

Couldn't you just add another parameter to ProgressMeterAccessible constructor? Seems easier and likely better.
Comment 15 alexander :surkov 2011-05-10 08:24:30 PDT
(In reply to comment #14)

> Couldn't you just add another parameter to ProgressMeterAccessible
> constructor? Seems easier and likely better.

easier - yes, better - I would argue. It shouldn't be member of the class because it related to the widget type (we have two and widget types only and it doesn't look like we will have more). So we shouldn't spend extra memory per instance. If we reject templates approach due to some reason then I would go with subclassing rather than member.
Comment 16 Marco Zehe (:MarcoZ) on PTO until August 15 2011-05-10 10:29:23 PDT
Created attachment 531369 [details] [diff] [review]
Patch, but doesn't work completely yet

Tossed out the template approach and went with an extra parameter in this patch.

However, this patch still doesn't do all we want. It throws the following errors:

14063 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/test_progress.html | Wrong value of  'pr_zeropointfive'  - got "0.5%", expected "50%"
14064 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/test_progress.html | Wrong current value of  'pr_zeropointfive'  - got 0.5, expected 50

Do we have to override the getCurrentValue method after all since we're dealing with different max values? This one seems to base the HTML values on a maximum of 100, not 1, for some reason.

14068 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/test_progress.html | Wrong value of  'pr_one'  - got "1%", expected "100%"
14069 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/test_progress.html | Wrong current value of  'pr_one'  - got 1, expected 100

The same question?

14073 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/test_progress.html | Wrong value of  'pr_42'  - got "42%", expected "100%"

Surkov, what do you think of the general approach?
Comment 17 David Bolter [:davidb] 2011-05-11 05:43:37 PDT
Comment on attachment 531369 [details] [diff] [review]
Patch, but doesn't work completely yet

Marco, for HTML, when getting the current value maybe you should use nsHTMLProgressElement::GetPosition * 100 instead of checking the attribute.
Comment 18 alexander :surkov 2011-05-11 08:39:40 PDT
Created attachment 531642 [details] [diff] [review]
patch
Comment 19 Marco Zehe (:MarcoZ) on PTO until August 15 2011-05-11 09:06:17 PDT
Comment on attachment 531642 [details] [diff] [review]
patch

>diff --git a/accessible/tests/mochitest/test_value.html b/accessible/tests/mochitest/value/test_general.html
>rename from accessible/tests/mochitest/test_value.html
>rename to accessible/tests/mochitest/value/test_general.html
>--- a/accessible/tests/mochitest/test_value.html
>+++ b/accessible/tests/mochitest/value/test_general.html
>@@ -17,17 +17,17 @@
>   </style>
> 
>   <script type="application/javascript"
>           src="chrome://mochikit/content/MochiKit/packed.js"></script>
>   <script type="application/javascript"
>           src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> 
>   <script type="application/javascript"
>-          src="common.js"></script>
>+          src="../common.js"></script>
> 
>   <script type="application/javascript"
>           src="chrome://mochikit/content/chrome-harness.js"/>

Nit: You need a </script> here and not the short form for closing the tag. Otherwise this file won't run.

> 
>   <script type="application/javascript">
>     function doTest()
>     {
>       function testValue(aID, aValue)
>@@ -72,14 +72,14 @@
>   <a id="aria_menuitem_link" role="menuitem" href="foo">menuitem</a>
>   <a id="aria_button_link" role="button" href="foo">button</a>
>   <a id="aria_checkbox_link" role="checkbox" href="foo">checkbox</a>
> 
>   <!-- landmark links -->
>   <a id="aria_application_link" role="application" href="foo">app</a>
>   <a id="aria_main_link" role="main" href="foo">main</a>
>   <a id="aria_navigation_link" role="navigation" href="foo">nav</a>
>-  
>+
>   <!-- strange edge case: please don't do this in the wild -->
>   <a id="aria_link_link" role="link" href="foo">link</a>
> 
> </body>
> </html>

Nit: Please add the test output lines I had in my original patch, too, so this file is in sync with all the others.

And now onto testing the patch itself.
Comment 20 alexander :surkov 2011-05-11 09:19:21 PDT
Thanks for the catch! fixed locally
Comment 21 Marco Zehe (:MarcoZ) on PTO until August 15 2011-05-11 10:20:17 PDT
Comment on attachment 531642 [details] [diff] [review]
patch

r=me for the tests (with nits addressed) and the functionality.
Comment 22 alexander :surkov 2011-05-11 10:29:32 PDT
on linux I get linking errors:


../../accessible/src/base/nsAccessibilityService.o: In function `nsAccessibilityService::CreateAccessibleByType(nsIContent*, nsIWeakReference*)':
/builds/slave/try-lnx-dbg/build/obj-firefox/accessible/src/base/../../../../accessible/src/base/nsAccessibilityService.cpp:1435: undefined reference to `ProgressMeterAccessible<100>::ProgressMeterAccessible(nsIContent*, nsIWeakReference*)'
../../accessible/src/base/nsAccessibilityService.o: In function `nsAccessibilityService::CreateHTMLAccessibleByMarkup(nsIFrame*, nsIContent*, nsIWeakReference*)':
/builds/slave/try-lnx-dbg/build/obj-firefox/accessible/src/base/../../../../accessible/src/base/nsAccessibilityService.cpp:1696: undefined reference to `ProgressMeterAccessible<1>::ProgressMeterAccessible(nsIContent*, nsIWeakReference*)'
/usr/bin/ld: libxul.so: hidden symbol `ProgressMeterAccessible<1>::ProgressMeterAccessible(nsIContent*, nsIWeakReference*)' isn't defined
/usr/bin/ld: final link failed: Nonrepresentable section on output

so the magic
template class ProgressMeterAccessible<1>;
template class ProgressMeterAccessible<100>;

doesn't work on linux.

Trevor, could you look at issue please?
Comment 23 Trevor Saunders (:tbsaunde) 2011-05-11 21:09:40 PDT
Created attachment 531847 [details] [diff] [review]
fix linux build

Move the ProgressMeterAccessible constructor into nsFormControlAccessible.h as we considered on irc, I'm still not sure why this didn't work on linux / did on windows.

I pushed http://tbpl.mozilla.org/?tree=Try&rev=1061693a4949  to make sure everything actually works.
Since this is basically the same as the last patch which I'd r+ but am obsoleting r=me for the non tests on this, but feel free to look more :)
Comment 24 Trevor Saunders (:tbsaunde) 2011-05-12 04:17:39 PDT
Created attachment 531886 [details] [diff] [review]
landed patch with ProgressMeterAccessible constructor reindented
Comment 25 Trevor Saunders (:tbsaunde) 2011-05-12 04:35:02 PDT
Created attachment 531888 [details] [diff] [review]
changed the indentation
Comment 26 Trevor Saunders (:tbsaunde) 2011-05-12 05:02:17 PDT
http://hg.mozilla.org/mozilla-central/rev/2f5f0bc44bdf
Comment 27 Marco Zehe (:MarcoZ) on PTO until August 15 2011-05-13 07:01:31 PDT
Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110513 Firefox/6.0a1. Thanks everyone!
Comment 28 Mounir Lamouri (:mounir) 2011-06-02 08:20:31 PDT
a11y related, is there a way for users to differentiate indeterminate progress bars with progress bar with value=0?
Comment 29 alexander :surkov 2011-06-06 02:00:51 PDT
(In reply to comment #28)
> a11y related, is there a way for users to differentiate indeterminate
> progress bars with progress bar with value=0?

I afraid there's no way from AT API point of view.
Comment 30 Mounir Lamouri (:mounir) 2011-06-06 02:06:49 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > a11y related, is there a way for users to differentiate indeterminate
> > progress bars with progress bar with value=0?
> 
> I afraid there's no way from AT API point of view.

So, there is no way to expose the :indeterminate pseudo-class to the user? :(
Comment 31 alexander :surkov 2011-06-06 02:14:55 PDT
(In reply to comment #30)
> > I afraid there's no way from AT API point of view.
> 
> So, there is no way to expose the :indeterminate pseudo-class to the user? :(

There's couple of options how to do that but no known standard way. If screen readers need it then we'll find a way. Let me check.
Comment 32 alexander :surkov 2011-07-11 23:17:15 PDT
(In reply to comment #30)

> So, there is no way to expose the :indeterminate pseudo-class to the user? :(

I filed bug 670853 for that.

Note You need to log in before you can comment on or make changes to this bug.