926812 - State change event not fired when both disabled and aria-disabled are toggled on button

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Hans Hillen]

Attachment: test_case_aria_disabled_virtual_buffer.html

When a button both has disabled="disabled" AND aria-disabled="true", and both those attributes are turned off at the same time (i.e. disabled="disabled" is removed, and aria-disabled is set to "false") then NVDA still announces the button as "unavailable". This is because Firefox does not fire the applicable OBJ_STATECHANGE event when both attributes are change at the same time.

The issue is avoided when only one of "disabled" or "aria-disabled" attributes are used, but it would still be useful if this can be fixed on the Firefox side.

Test case has been provided on ​https://dl.dropboxusercontent.com/u/573324/testcases/bugreports/NVDA/test_case_aria_disabled_virtual_buffer.html (also attached).

To use this test case:
Using NVDA (or AccEvent), tab to the third button, labelled "Toggle both aria-disabled AND disabled", and activate it. This will apply both attributes on the fourth button, which is labelled "save".
Activate the third button a second time. This will remove the disabled attribute and set aria-disabled="false"
Press tab to move focus to the next button, labeled "Save". NVDA will now incorrectly announce it as "unavailable".
Use the NVDA + F5 shortcut to refresh the virtual buffer. Now the button will be announced correctly again.
Note that the page in the test case also has controls that toggle only the disabled attribute or the aria-disabled attribute. The issue only occurs when both attributes are turned off at the same time.

This issue was initially filed with NVDA: http://community.nvda-project.org/ticket/3565

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Trevor Saunders (:tbsaunde)]

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

>+  // Note. We use the attribute instead of the disabled state bit because
>+  // ARIA's aria-disabled does not affect the disabled state bit.

what? afaik there's only an enabled bit, and it very much seems to be effected bythe aria attribute.

>+    // Do nothing if state wasn't changed (like @aria-disabled was removed but
>+    // @disabled is still presented).
>+    if (aAccessible->Unavailable() == mStateBitWasOn)
>+      return;

is this really need or can you just rely on coalescing?

> 
>     nsRefPtr<AccEvent> enabledChangeEvent =
>-      new AccStateChangeEvent(aAccessible, states::ENABLED);
>+      new AccStateChangeEvent(aAccessible, states::ENABLED, mStateBitWasOn);

shouldn't it be the current state not the old one?

So the bug here is that we used the AccStateChangeEvent constructor that called State() that seems like even more of a foot gun now, what still uses it?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Comment on attachment 831040 [details] [diff] [review]
> patch
> 
> >+  // Note. We use the attribute instead of the disabled state bit because
> >+  // ARIA's aria-disabled does not affect the disabled state bit.
> 
> what? afaik there's only an enabled bit, and it very much seems to be
> effected bythe aria attribute.

I think they meant content disabled state but I agree the comment is rather confusing.

> >+    // Do nothing if state wasn't changed (like @aria-disabled was removed but
> >+    // @disabled is still presented).
> >+    if (aAccessible->Unavailable() == mStateBitWasOn)
> >+      return;
> 
> is this really need or can you just rely on coalescing?

Idea is if nothing was changed then we don't have to have an event and make to coalesce them. You could say that if nothing was changed then event coalescence could coalesce it but it doesn't seem possible until we know previous accessible state.

> > 
> >     nsRefPtr<AccEvent> enabledChangeEvent =
> >-      new AccStateChangeEvent(aAccessible, states::ENABLED);
> >+      new AccStateChangeEvent(aAccessible, states::ENABLED, mStateBitWasOn);
> 
> shouldn't it be the current state not the old one?

mStateBitWasOn is about UNAVAILABLE state but event is about ENABLED state

> So the bug here is that we used the AccStateChangeEvent constructor that
> called State() that seems like even more of a foot gun now, what still uses
> it?

I'm not sure I follow it, can you elaborate?

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > Comment on attachment 831040 [details] [diff] [review]
> > patch
> > 
> > >+  // Note. We use the attribute instead of the disabled state bit because
> > >+  // ARIA's aria-disabled does not affect the disabled state bit.
> > 
> > what? afaik there's only an enabled bit, and it very much seems to be
> > effected bythe aria attribute.
> 
> I think they meant content disabled state but I agree the comment is rather
> confusing.

yeah, that would make sense want to try and make it better?

> > >+    // Do nothing if state wasn't changed (like @aria-disabled was removed but
> > >+    // @disabled is still presented).
> > >+    if (aAccessible->Unavailable() == mStateBitWasOn)
> > >+      return;
> > 
> > is this really need or can you just rely on coalescing?
> 
> Idea is if nothing was changed then we don't have to have an event and make
> to coalesce them. You could say that if nothing was changed then event
> coalescence could coalesce it but it doesn't seem possible until we know
> previous accessible state.

I was thinking this would be only case where it could happen and then we'd fire two state change events both doing the same thing so they'd already get coalesced, but I guess that isn't a great assumption.

> > > 
> > >     nsRefPtr<AccEvent> enabledChangeEvent =
> > >-      new AccStateChangeEvent(aAccessible, states::ENABLED);
> > >+      new AccStateChangeEvent(aAccessible, states::ENABLED, mStateBitWasOn);
> > 
> > shouldn't it be the current state not the old one?
> 
> mStateBitWasOn is about UNAVAILABLE state but event is about ENABLED state

that's pretty confusing.

btw can you add assertions about  which bit of union is being used when?

> > So the bug here is that we used the AccStateChangeEvent constructor that
> > called State() that seems like even more of a foot gun now, what still uses
> > it?
> 
> I'm not sure I follow it, can you elaborate?

I was trying to figure out how this patch fixes the problem, but actually I'm not sure my explanation makes sense either.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> > I think they meant content disabled state but I agree the comment is rather
> > confusing.
> 
> yeah, that would make sense want to try and make it better?

actually I don't see why we can't handle aria-disabled attribute change or content disabled state change, so I would just remove the comment

> > Idea is if nothing was changed then we don't have to have an event and make
> > to coalesce them. You could say that if nothing was changed then event
> > coalescence could coalesce it but it doesn't seem possible until we know
> > previous accessible state.
> 
> I was thinking this would be only case where it could happen and then we'd
> fire two state change events both doing the same thing so they'd already get
> coalesced, but I guess that isn't a great assumption.

we could coalesce them by state but that doesn't handle the case when a state turned off and on quickly so no change for AT

> > mStateBitWasOn is about UNAVAILABLE state but event is about ENABLED state
> 
> that's pretty confusing.

well, we have a bunch of confusing states but that's a normal state of our module :)

> btw can you add assertions about  which bit of union is being used when?

can you give me an example?

> I was trying to figure out how this patch fixes the problem, but actually
> I'm not sure my explanation makes sense either.

the fix is since we don't fire state change event when state wasn't changed then coalescence of fired events always works right.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> 
> > > I think they meant content disabled state but I agree the comment is rather
> > > confusing.
> > 
> > yeah, that would make sense want to try and make it better?
> 
> actually I don't see why we can't handle aria-disabled attribute change or
> content disabled state change, so I would just remove the comment

wfm

> > > mStateBitWasOn is about UNAVAILABLE state but event is about ENABLED state
> > 
> > that's pretty confusing.
> 
> well, we have a bunch of confusing states but that's a normal state of our
> module :)

doesn't mean we should add more :p  any reason to not name it mWasUnavailable or mWasDisabled to be more clear?

> > btw can you add assertions about  which bit of union is being used when?
> 
> can you give me an example?
> 
> > I was trying to figure out how this patch fixes the problem, but actually
> > I'm not sure my explanation makes sense either.
> 
> the fix is since we don't fire state change event when state wasn't changed
> then coalescence of fired events always works right.

ok that was more or less what I was trying to say.

Comment 7

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0e244d1ff393

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e244d1ff393