761901 - HTML5 progress accessible should fire value change event

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

some details:

HTML and XUL progress elements both are implemented by ProgressMeterAccessible.

XUL progressmeter element fires ValueChange event (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/progressmeter.xml#43) what is handled by RootAccessible.

For HTML element we can listen 'value' attribute change in DocAccessible ('ValueChange' event is not option, tag names checking is not super cool).

If somebody has nice ideas how to implement it in unified way then please comment.

Comment 2

[Trevor Saunders (:tbsaunde)]

> For HTML element we can listen 'value' attribute change in DocAccessible
> ('ValueChange' event is not option, tag names checking is not super cool).

I'm not absolutely sure, but it looks like listening for the value attribute changing in DocAccessible gets both xul and html.  I guess checking the tag or poking at xul xbl to be sure its a progress meter isn't super cool, but it would allow killing off another DOM event we listen to, and be fairly fast so I'd think its good enough.

Comment 3

[alexander :surkov (:asurkov)]

it should work but keep in mind other possible controls we might not need value change events for and other controls that possibly don't manage @value attribute we need value change events for :)

Comment 4

[alexander :surkov (:asurkov)]

or alternatively we can have IsProgressMeter which is a little bit ugly but simple and works.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> or alternatively we can have IsProgressMeter which is a little bit ugly but
> simple and works.

I'm not really sure what your suggesting here.

it does occur to me though we should probably add somesort of HasValue() or IsValue() which I think we'll need anyway to replace nsIAccessibleValue

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> (In reply to alexander :surkov from comment #4)
> > or alternatively we can have IsProgressMeter which is a little bit ugly but
> > simple and works.
> 
> I'm not really sure what your suggesting here.

I meant process @value attribute change for IsProgressMeter() accessibles. It guarantees we don't fire the event twice (for @value and ValueChange DOM event).

> it does occur to me though we should probably add somesort of HasValue() or
> IsValue() which I think we'll need anyway to replace nsIAccessibleValue

in general it doesn't guarantees us that all @value approach works for every HasValue() accessible. example:
<div role="progressbar" aria-valuenow="3" value="555"></div>
@value doesn't make sense on div and the accessible answers to HasValue()

Comment 7

[alexander :surkov (:asurkov)]

steps to fix:

1) introduce eProgressAccessible flag (see Accessible::mFlags) and add Accessible::IsProgress() method
2) set this flag for ProgressMeterAccessible class
3) RootAccessible::ProcessDOMEvent do not process "ValueChange" event for IsProgress() accessibles
4) add @value attribute change in DocAccessible::AttributeChangedImpl for IsProgress() accessible and fire value_change event
5) add a test into events/test_valuechange.html

Comment 8

[James Kitchener (:jkitch)]

Attachment: Patch v1

Passes the modified test

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 668753 [details] [diff] [review]
Patch v1

Review of attachment 668753 [details] [diff] [review]:
-----------------------------------------------------------------

(it wasn't nice approach but I'm not sure how it can be improved. Trev might have ideas)

James, thank you for the fix! (You might wait for Trevor comment before updating the patch)

::: accessible/src/generic/Accessible.h
@@ +519,5 @@
>  
>    inline bool IsTextLeaf() const { return mFlags & eTextLeafAccessible; }
>    mozilla::a11y::TextLeafAccessible* AsTextLeaf();
>  
> +  inline bool IsProgress() const { return mFlags & eProgressAccessible; }

nit: please put it after IsMenuPopup to keep in alphabetical order

@@ +783,5 @@
>      eRootAccessible = 1 << 19,
>      eTextLeafAccessible = 1 << 20,
>      eXULDeckAccessible = 1 << 21,
> +    eXULTreeAccessible = 1 << 22,
> +    eProgressAccessible = 1 << 23

nit: these should be in alphabetical order too

::: accessible/src/generic/FormControlAccessible.h
@@ +21,5 @@
>    ProgressMeterAccessible(nsIContent* aContent, DocAccessible* aDoc) :
>      LeafAccessible(aContent, aDoc)
>    {
>      mFlags = mFlags | eHasNumericValue;
> +    mFlags = mFlags | eProgressAccessible;

mFlags |= eHasNumericValue | eProcessAccessible;

::: accessible/src/generic/RootAccessible.cpp
@@ +482,1 @@
>    }

nit: add a comment like: We don't process 'ValueChange' events for progress meters since we listen @value attribute change for them.

::: accessible/tests/mochitest/events/test_valuechange.html
@@ +97,5 @@
>        gQueue.push(new changeValue("scrollbar", "6", undefined));
>  
>        gQueue.push(new changeInputValue("combobox", "hello"));
>  
> +      gQueue.push(new changeValue("progress", "50", undefined));

You need to test
<progress value="22" max="100"></progress> 
by changeInputValue

please rename changeValue to changeARIAValue, rename changeInputValue to changeValue.

Comment 10

[Trevor Saunders (:tbsaunde)]

Comment on attachment 668753 [details] [diff] [review]
Patch v1

sorry about delay :(

>+  if (aAttribute == nsGkAtoms::value && this->IsProgress()) {

accessible->IsProgress()

>+      FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_VALUE_CHANGE,
>+                                 aContent);
>+      return;

return isn't needed, and might get in the way please remove it

>+    if (!accessible->IsProgress())
>+      targetDocument->
>+        FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_VALUE_CHANGE,
>+                                   targetNode, AccEvent::eRemoveDupes);

might as well remove the eRemoveDups since your here and its the default

Comment 11

[James Kitchener (:jkitch)]

Attachment: Patch v2 (wip)

Doesn't pass tests yet.

The ChangeValue method is failing with the following message
"failed | Wrong value for 'progress' - got 50%, expected 50"

I tried passing 50% into the ChangeValue function, but the error appears as "got 0%, expected 50%"

Do you have any suggestions?

Comment 12

[alexander :surkov (:asurkov)]

Keep ChangeInputValue unchanged, add similar ChangeProcessValue to process '%' (pass '50' but compare with '50%', i.e. aValue + "%".

Comment 13

[James Kitchener (:jkitch)]

Attachment: patch v3

Comment 14

[Trevor Saunders (:tbsaunde)]

Comment on attachment 671153 [details] [diff] [review]
patch v3

>+  if (aAttribute == nsGkAtoms::value) {
>+    Accessible* accessible = GetAccessible(aContent);
>+    if(accessible->IsProgress()) {

you need to check accessible is not null, you could have node that is not accessible have its value attribute change.

Comment 15

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ef03cd944ba8

Comment 16

[James Kitchener (:jkitch)]

Given that the patch without the null check has been pushed to inbound, how did you want me to do the correction?  A new patch that only fixes the null check?

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ef03cd944ba8

Comment 18

[Trevor Saunders (:tbsaunde)]

yeah, I was sure I fixed that before landing, but apparently I didn't, so I pushed remote:   https://hg.mozilla.org/mozilla-central/rev/044d1b974385

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #18)
> yeah, I was sure I fixed that before landing, but apparently I didn't, so I
> pushed remote:   https://hg.mozilla.org/mozilla-central/rev/044d1b974385

btw, accessible can't be null at this point

Comment 20

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #19)
> (In reply to Trevor Saunders (:tbsaunde) from comment #18)
> > yeah, I was sure I fixed that before landing, but apparently I didn't, so I
> > pushed remote:   https://hg.mozilla.org/mozilla-central/rev/044d1b974385
> 
> btw, accessible can't be null at this point

yeah, your right ugh :(
I remembered you commenting on what I thought was similar code a while ago saying it could be, but it sems pretty clear if that was the case it can't be now.