559773 - make HTML5 progress element accessible

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Should be similar to XUL analogue. Should fire value change events and implement nsIAccessibleValue.

Comment 1

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

Yep, looks like patches are r+'ed over there.

Comment 2

[Mounir Lamouri (:mounir)]

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

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

Try server builds are available here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b90e2723adad/

You can check this page for <progress> examples:
http://oldworld.fr/mozilla/progress.html

Comment 5

[Marco Zehe (:MarcoZ)]

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

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

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

(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)]

(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)]

Attachment: 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)]

(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 (:asurkov)]

(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)]

Attachment: 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)]

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 (:asurkov)]

(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)]

Attachment: 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] (NeedInfo me for attention)]

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 (:asurkov)]

Attachment: patch

Comment 19

[Marco Zehe (:MarcoZ)]

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 (:asurkov)]

Thanks for the catch! fixed locally

Comment 21

[Marco Zehe (:MarcoZ)]

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

r=me for the tests (with nits addressed) and the functionality.

Comment 22

[alexander :surkov (:asurkov)]

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

Attachment: 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)]

Attachment: landed patch with ProgressMeterAccessible constructor reindented

Comment 25

[Trevor Saunders (:tbsaunde)]

Attachment: changed the indentation

Comment 26

[Trevor Saunders (:tbsaunde)]

http://hg.mozilla.org/mozilla-central/rev/2f5f0bc44bdf

Comment 27

[Marco Zehe (:MarcoZ)]

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

a11y related, is there a way for users to differentiate indeterminate progress bars with progress bar with value=0?

Comment 29

[alexander :surkov (:asurkov)]

(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)]

(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 (:asurkov)]

(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 (:asurkov)]

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