478032 - Fire delayed value changed event for aria-valuetext changes

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

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

It looks like we currently do this for aria-valuenow changes but not aria-valuetext changes. Filing in case we need to do this. (see nsDocAccessible::ARIAAttributeChanged)

Comment 1

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

Attachment: wip

What do you think? (I'm thinking we might want eRemoveDupes to consider valuenow and valuetext changes the same)

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 376279 [details] [diff] [review]
wip

Do we make sure that if valueno and valuetext change, that we get updates for both? Or will either trigger an update of both?

Comment 3

[alexander :surkov (:asurkov)]

I believe value change event goes with change of return value of nsIAccessible::GetValue method, GetValue() returns aria-valuetext and then aria-valuenow if aria-valuetext is missed. So you should bring this logic here I think.

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 376279 [details] [diff] [review]
wip

cancelling reviews until consensus with us :)

Comment 5

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

(In reply to comment #2)
> (From update of attachment 376279 [details] [diff] [review])
> Do we make sure that if valueno and valuetext change, that we get updates for
> both? Or will either trigger an update of both?

Well, with the patch, a change to either attribute would fire an event, so if both changed, we would at least queue up two events.

(In reply to comment #3)
OK thanks for the IRC chat about this. So the idea is to care about aria-valuenow changes, only if aria-valuetext seems unused. So when we get an aria-valuenow change we check to see if valuetext is empty (like GetValue does), and fire only if it is empty. Value text changes will always fire.

...but, what if we have a situation where there isn't a 1:1 mapping of valuenow to valuetext? Consider a slider from 0 to 10. valuenow could be the numbers from 0 to 10, whereas, the content author might only update valuetext for 1, 5, and 10, with "none", "half", and "all".

Always firing seems more bulletproof maybe...

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> ...but, what if we have a situation where there isn't a 1:1 mapping of valuenow
> to valuetext? Consider a slider from 0 to 10. valuenow could be the numbers
> from 0 to 10, whereas, the content author might only update valuetext for 1, 5,
> and 10, with "none", "half", and "all".
> 
> Always firing seems more bulletproof maybe...

The question should be how are IAccessibleValue and IAccessible::value plus value change events used. If AT prefers to announce IAccessible::value when it gets value change event then it doesn't make sense to fire an event when aria-valuenow is changed but aria-valuetext.

Marco, you're turn please.

Comment 7

[Marco Zehe (:MarcoZ)]

Well, for backward compatibility with older ATs we should fire valuechange whenever IAccessible::Value changes.
For the IAccessibleValue implementation, I'm CC'ing Mick and Jamie to help us determine whether we should also fire events if something changes that's only exposed through IAccessibleValue and not through IAccessible::Get_accValue. I would think yes, but want to make sure... Jamie? Mick? Any word on this?

Comment 8

[James Teh [:Jamie]]

NVDA doesn't currently use the IAccessibleValue interface at all. However, this is more because we haven't needed it so far rather than an actual policy decision. Nevertheless:

If something is exposed through IAccessibleValue but not accValue, we think a value change event should be fired. Otherwise, there is no notification of the change. While value change events are technically designed to accompany changes to accValue, there is no event specific to IAccessibleValue changes, so value change event should be used.

Comment 9

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

Comment on attachment 376279 [details] [diff] [review]
wip

OK, r? for Marco, and Alex... shall we go for it?

Comment 10

[alexander :surkov (:asurkov)]

I would say let's introduce field for landmark in nsRoleMapEntry

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> I would say let's introduce field for landmark in nsRoleMapEntry

wrong bug comment

Comment 12

[alexander :surkov (:asurkov)]

How does it fix the bug?

Comment 13

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

OK Alex, from our IRC chat here is the plan:
If we get a valuenow change, fire if valuetext is empty.
If we get a valuetext change, fire.

I'll work this up, make sure we don't regress and post. Thanks!

Comment 14

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

Attachment: patch 2

Comment 15

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

Attachment: patch 2.1

Comment 16

[Marco Zehe (:MarcoZ)]

Comment on attachment 377781 [details] [diff] [review]
patch 2.1

This looks correct as per our discussion on this. Do you have a test case wip'ed up that you can transform into a mochitest?

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 377781 [details] [diff] [review]
patch 2.1

>diff --git a/accessible/src/base/nsDocAccessible.cpp b/accessible/src/base/nsDocAccessible.cpp
>--- a/accessible/src/base/nsDocAccessible.cpp
>+++ b/accessible/src/base/nsDocAccessible.cpp
>@@ -1282,19 +1282,21 @@ nsDocAccessible::ARIAAttributeChanged(ns
>     nsCOMPtr<nsIAccessibleStateChangeEvent> event =
>       new nsAccStateChangeEvent(targetNode,
>                                 nsIAccessibleStates::STATE_READONLY,
>                                 PR_FALSE);
>     FireDelayedAccessibleEvent(event);
>     return;
>   }
> 
>-  if (aAttribute == nsAccessibilityAtoms::aria_valuenow) {
>-    FireDelayedToolkitEvent(nsIAccessibleEvent::EVENT_VALUE_CHANGE,
>-                            targetNode);
>+  if (aAttribute == nsAccessibilityAtoms::aria_valuetext ||
>+      (aAttribute == nsAccessibilityAtoms::aria_valuenow &&
>+      !nsAccUtils::HasDefinedARIAToken(aContent,
>+          nsAccessibilityAtoms::aria_valuetext))) {


Why do you check it on undefined? It's a string.

You could add value_aria.html file using value.js, register value change event listeners and change attributes. Otherwise patch looks good.

Comment 18

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

(In reply to comment #16)
> (From update of attachment 377781 [details] [diff] [review])
> This looks correct as per our discussion on this. Do you have a test case
> wip'ed up that you can transform into a mochitest?

Not yet. Will do.

(In reply to comment #17)

> >+      !nsAccUtils::HasDefinedARIAToken(aContent,
> >+          nsAccessibilityAtoms::aria_valuetext))) {
> 
> 
> Why do you check it on undefined? It's a string.

OK I'll just check empty string (just in case "undefined" is useful for valuetext).

> You could add value_aria.html file using value.js, register value change event
> listeners and change attributes. Otherwise patch looks good.

I'll add that with this patch.

Thanks guys.

Comment 19

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

Attachment: patch 3

Comment 20

[Marco Zehe (:MarcoZ)]

Comment on attachment 378384 [details] [diff] [review]
patch 3

r=me with a few nits:
>+  <style>
>+    div.displayNone a { display:none; }
>+    div.visibilityHidden a { visibility:hidden; }
>+</style>

Copy & Paste leftover from the file you used a s a template? ;-) You don't use these styles at all, so I think they can be removed.

>+        // Note: this should always fire fire an EVENT_VALUE_CHANGE

The word "fire" is duplicated here.

>+        // Note: always test against valuetext first because the existence of 
>+        // aria-valuetext takes precendence over aria-valuenow in gecko.

"Precedence" has one extraneous n before the d.

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 378384 [details] [diff] [review]
patch 3


>-  if (aAttribute == nsAccessibilityAtoms::aria_valuenow) {
>-    FireDelayedToolkitEvent(nsIAccessibleEvent::EVENT_VALUE_CHANGE,
>-                            targetNode);
>+  if (aAttribute == nsAccessibilityAtoms::aria_valuetext ||      
>+      (aAttribute == nsAccessibilityAtoms::aria_valuenow &&
>+          // we care about valuenow only if there is no valuetext

I would like to move this comment right before "if" statement and possibly to wide it like
"Fire value change event whenever aria-valuetext is changed or aria-valuenow is changed if aria-valuetext is empty"

>- * When event is caught then current invoker object is asked to check wether
>+ * When event is caught then current invoker object is asked to check whether

thank you for fixing this :)

>+  <script type="application/javascript"
>+          src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>

you don't use EventUtils.js here.

>+      this.getID = function changeValue_getID() { return aNodeOrID +
on new line please and use prettyName

r=me

Comment 22

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

Thanks guys, I've made the corrections locally and will push when I can.

Comment 23

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

Pushed as changeset: http://hg.mozilla.org/mozilla-central/rev/ddc05e517cd3

Comment 24

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090618 Minefield/3.6a1pre (.NET CLR 3.5.30729)