"haspopup" AtkObject attribute not exposed if multiprocess is enabled

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jdiggs, Assigned: jdiggs)

Tracking

unspecified
mozilla56
Unspecified
Linux
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
Assignee: nobody → jdiggs
Status: NEW → ASSIGNED
Attachment #8880408 - Flags: review?(surkov.alexander)
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 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?
Attachment #8880408 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 6

2 years ago
(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?
(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 :)
(Assignee)

Comment 8

2 years ago
(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!
Keywords: checkin-needed

Comment 9

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c912c7b540cf
"haspopup" AtkObject attribute missing if multiprocess enabled. r=surkov
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c912c7b540cf
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.