1375460 - "haspopup" AtkObject attribute not exposed if multiprocess is enabled

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Steps to reproduce *with multiprocess support enabled*:
1. Load data:text/html,<div role="button" aria-haspopup="true">foo</div>
2. Use Accerciser to examine the object attributes

Expected results: the object attributes would contain 'haspopup':'true'
Actual results: haspopup is not in the object attributes at all

If multiprocess support is disabled, one gets the expected results.

Comment 1

[Marco Zehe (:MarcoZ)]

On Windows, this works as expected.

Comment 2

[Joanmarie Diggs]

Marco, please define "works as expected." I don't see any object attribute when I use Accessibility Viewer. Also, looking at the Core AAM 1.0, exposure as an object attribute is not expected -- just exposure as a state. However, the Core AAM 1.1 expects exposure as an object attribute for IA2 in addition to ATK. The reason why is that the value of aria-haspopup switched from a boolean to a token and the state will not be sufficient to convey the type.

Comment 3

[Joanmarie Diggs]

Attachment: proposed patch

Comment 4

[Marco Zehe (:MarcoZ)]

Sorry, my bad. I forgot that we're still exposing the states thing in IA2 and NVDA is keying off of that rather than the object attribute.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 8880408 [details] [diff] [review]
proposed patch

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

looks good with me

::: accessible/generic/Accessible.cpp
@@ +945,4 @@
>                             NS_LITERAL_STRING("true"));
>    }
>  
> +  // XXX: In ARIA 1.1, the value of aria-haspopup became a token (bug 1355449).

why is it XXX comment?

Comment 6

[Joanmarie Diggs]

(In reply to alexander :surkov from comment #5)
> Comment on attachment 8880408 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 8880408 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good with me
> 
> ::: accessible/generic/Accessible.cpp
> @@ +945,4 @@
> >                             NS_LITERAL_STRING("true"));
> >    }
> >  
> > +  // XXX: In ARIA 1.1, the value of aria-haspopup became a token (bug 1355449).
> 
> why is it XXX comment?

Because bug 1355449 isn't fixed yet (Based on observation, I concluded XXX is a TODO/FIXME thing. If that's wrong, oops.)

In previous patch reviews, you've indicated that when fixing multiple things it is preferable to split things up into separate bugs. This bug and patch is about exposing the object attribute which is broken for multiprocess. This bug's fix causes us to expose the same thing regardless of whether or not multiprocess is enabled. The next step is to turn the "true" into the token type -- after validating it, because the spec also states that items not in the list (e.g. "foo") should be treated as False (Gecko treats them as "true" -- independent of any changes made here or in other patches I've done).

With that in mind, would you like me to change the comment?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Joanmarie Diggs from comment #6)
> (In reply to alexander :surkov from comment #5)
> > Comment on attachment 8880408 [details] [diff] [review]
> > proposed patch
> > 
> > Review of attachment 8880408 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > looks good with me
> > 
> > ::: accessible/generic/Accessible.cpp
> > @@ +945,4 @@
> > >                             NS_LITERAL_STRING("true"));
> > >    }
> > >  
> > > +  // XXX: In ARIA 1.1, the value of aria-haspopup became a token (bug 1355449).
> > 
> > why is it XXX comment?
> 
> Because bug 1355449 isn't fixed yet (Based on observation, I concluded XXX
> is a TODO/FIXME thing. If that's wrong, oops.)

you are correct, XXX is a todo thing.

> In previous patch reviews, you've indicated that when fixing multiple things
> it is preferable to split things up into separate bugs. This bug and patch
> is about exposing the object attribute which is broken for multiprocess.
> This bug's fix causes us to expose the same thing regardless of whether or
> not multiprocess is enabled. The next step is to turn the "true" into the
> token type -- after validating it, because the spec also states that items
> not in the list (e.g. "foo") should be treated as False (Gecko treats them
> as "true" -- independent of any changes made here or in other patches I've
> done).
> 
> With that in mind, would you like me to change the comment?

It's totatly up to you. Usually people put XXX for something that will stay for a while for other developers who might be looking into this code. It's probably not your case, since you move quite fast :)

Comment 8

[Joanmarie Diggs]

(In reply to alexander :surkov from comment #7)
 
> It's totatly up to you. Usually people put XXX for something that will stay
> for a while for other developers who might be looking into this code. It's
> probably not your case, since you move quite fast :)

Don't jinx me. ;) I'll try to remove it quickly, but leave it "just in case." Thanks again!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c912c7b540cf
"haspopup" AtkObject attribute missing if multiprocess enabled. r=surkov

Comment 10

[Carsten Book [:Tomcat]]

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