The default bug view has changed. See this FAQ.

HTML5 progress accessible should fire value change event

RESOLVED FIXED in mozilla19

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: jkitch)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla19
x86
Mac OS X
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
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.
> 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.
(Reporter)

Comment 3

5 years ago
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 :)
(Reporter)

Comment 4

5 years ago
or alternatively we can have IsProgressMeter which is a little bit ugly but simple and works.
(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
(Reporter)

Comment 6

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

Comment 7

5 years ago
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
Whiteboard: [mentor=trev.saunders@gmail.com][lang=c++]
(Assignee)

Comment 8

5 years ago
Created attachment 668753 [details] [diff] [review]
Patch v1

Passes the modified test
Attachment #668753 - Flags: review?(trev.saunders)
(Assignee)

Updated

5 years ago
Assignee: nobody → jkitch.bug
(Reporter)

Comment 9

5 years ago
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.
Attachment #668753 - Flags: feedback+
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
Attachment #668753 - Flags: review?(trev.saunders)
(Assignee)

Comment 11

5 years ago
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?
Attachment #668753 - Attachment is obsolete: true
(Reporter)

Comment 12

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

Comment 13

5 years ago
Created attachment 671153 [details] [diff] [review]
patch v3
Attachment #671077 - Attachment is obsolete: true
Attachment #671153 - Flags: review?(trev.saunders)
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.
Attachment #671153 - Flags: review?(trev.saunders) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ef03cd944ba8
(Assignee)

Comment 16

5 years ago
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?
https://hg.mozilla.org/mozilla-central/rev/ef03cd944ba8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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
(Reporter)

Comment 19

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