Last Comment Bug 761901 - HTML5 progress accessible should fire value change event
: HTML5 progress accessible should fire value change event
Status: RESOLVED FIXED
[mentor=trev.saunders@gmail.com][lang...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla19
Assigned To: James Kitchener (:jkitch)
:
Mentors:
Depends on:
Blocks: html5a11y
  Show dependency treegraph
 
Reported: 2012-06-05 21:17 PDT by alexander :surkov
Modified: 2012-10-16 18:43 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (6.62 KB, patch)
2012-10-06 02:26 PDT, James Kitchener (:jkitch)
surkov.alexander: feedback+
Details | Diff | Splinter Review
Patch v2 (wip) (9.54 KB, patch)
2012-10-13 04:53 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
patch v3 (10.08 KB, patch)
2012-10-13 18:18 PDT, James Kitchener (:jkitch)
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-06-05 21:17:02 PDT

    
Comment 1 alexander :surkov 2012-06-05 22:09:07 PDT
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) 2012-06-16 05:21:48 PDT
> 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 2012-06-16 22:19:25 PDT
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 2012-06-16 22:20:09 PDT
or alternatively we can have IsProgressMeter which is a little bit ugly but simple and works.
Comment 5 Trevor Saunders (:tbsaunde) 2012-06-18 03:46:56 PDT
(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 2012-09-05 07:01:56 PDT
(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 2012-09-05 07:20:18 PDT
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) 2012-10-06 02:26:47 PDT
Created attachment 668753 [details] [diff] [review]
Patch v1

Passes the modified test
Comment 9 alexander :surkov 2012-10-07 19:14:30 PDT
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) 2012-10-11 09:17:48 PDT
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) 2012-10-13 04:53:57 PDT
Created attachment 671077 [details] [diff] [review]
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 2012-10-13 06:14:14 PDT
Keep ChangeInputValue unchanged, add similar ChangeProcessValue to process '%' (pass '50' but compare with '50%', i.e. aValue + "%".
Comment 13 James Kitchener (:jkitch) 2012-10-13 18:18:40 PDT
Created attachment 671153 [details] [diff] [review]
patch v3
Comment 14 Trevor Saunders (:tbsaunde) 2012-10-15 14:39:41 PDT
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) 2012-10-15 20:06:33 PDT
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ef03cd944ba8
Comment 16 James Kitchener (:jkitch) 2012-10-16 00:30:29 PDT
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] 2012-10-16 01:18:03 PDT
https://hg.mozilla.org/mozilla-central/rev/ef03cd944ba8
Comment 18 Trevor Saunders (:tbsaunde) 2012-10-16 17:23:25 PDT
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 2012-10-16 17:28:10 PDT
(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) 2012-10-16 18:43:57 PDT
(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.

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