Closed Bug 960228 Opened 10 years ago Closed 10 years ago

Lists with selectable children should have ATK_ROLE_LIST_BOX

Categories

(Core :: Disability Access APIs, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jdiggs, Assigned: jwei)

References

(Blocks 2 open bugs)

Details

(Whiteboard: scr)

Attachments

(5 files)

Steps to reproduce:
1. Launch the attached accessible-event listener in a terminal
2. Launch the test case in the browser
3. Tab to move focus into the list

Expected result: The control with the selectable children would have ROLE_LIST_BOX.

Actual result: The control with the selectable children has ROLE_LIST.

Giving this object the same role as (un)ordered lists means that additional checks have to be made by ATs trying to figure out what sort of object a given list item (or list) really is.
Jonathan, this is an easy one, just fix a role map. Please feel free to take it.

(for the reference, the mapping was introduced in bug 391490, no comments about it in the bug)
Assignee: nobody → jwei
Attachment #8362640 - Flags: review?(surkov.alexander)
Attachment #8362640 - Flags: review?(surkov.alexander) → review+
(In reply to Jonathan Wei [:jwei] from comment #4)
> Created attachment 8362640 [details] [diff] [review]
> Modified list box ATK mapping.

uhm I don't think this is ok without falling back to the previous role if we detect the atk version we're dealing with is less than 2.1 when this role was introduced.
(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> (In reply to Jonathan Wei [:jwei] from comment #4)
> > Created attachment 8362640 [details] [diff] [review]
> > Modified list box ATK mapping.
> 
> uhm I don't think this is ok without falling back to the previous role if we
> detect the atk version we're dealing with is less than 2.1 when this role
> was introduced.

are there some old Orcas running? Is there any ifdef thing for detection?
(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > (In reply to Jonathan Wei [:jwei] from comment #4)
> > > Created attachment 8362640 [details] [diff] [review]
> > > Modified list box ATK mapping.
> > 
> > uhm I don't think this is ok without falling back to the previous role if we
> > detect the atk version we're dealing with is less than 2.1 when this role
> > was introduced.
> 
> are there some old Orcas running? Is there any ifdef thing for detection?

who knows what all lives out in the wild. I'm hopeful we can just call atk_get_version() but I need  to check that won't break the redhat people, worst case we can dlsym for that function though.
is it common thing is Linux world to use old distro and updated softwares? I don't really like to bloat the code by adding if/else stuff to support something pretty old.
(In reply to alexander :surkov from comment #8)
> is it common thing is Linux world to use old distro and updated softwares? I
> don't really like to bloat the code by adding if/else stuff to support
> something pretty old.

In the case of updated firefox especially with shorter release cycle yes.  ANyway its not that hard, and its in a pretty out of the way spot so I don't think its a big deal.
(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> (In reply to alexander :surkov from comment #8)
> > is it common thing is Linux world to use old distro and updated softwares? I
> > don't really like to bloat the code by adding if/else stuff to support
> > something pretty old.
> 
> In the case of updated firefox especially with shorter release cycle yes. 

old distros are supposed to use ESR builds I think

I'm curious how latest Orca is doing on ATK 1.x distros?

> ANyway its not that hard, and its in a pretty out of the way spot so I don't
> think its a big deal.

some ugly code, more ifelse checks, more paths to maintain that's all doesn't keep me happier
(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > (In reply to alexander :surkov from comment #8)
> > > is it common thing is Linux world to use old distro and updated softwares? I
> > > don't really like to bloat the code by adding if/else stuff to support
> > > something pretty old.
> > 
> > In the case of updated firefox especially with shorter release cycle yes. 
> 
> old distros are supposed to use ESR builds I think

even those are only around for a year or so.

> I'm curious how latest Orca is doing on ATK 1.x distros?

who said people were using latest orca?

> > ANyway its not that hard, and its in a pretty out of the way spot so I don't
> > think its a big deal.
> 
> some ugly code, more ifelse checks, more paths to maintain that's all
> doesn't keep me happier

I didn't say it was super nice thing, just that its not a big deal.
I'd say those people should use ESR builds as long as they can and then they should update their systems since basically it's a requirement of ESR: if you want to stay secure and use latest Firefox then you should make sure your system fits it. So I wouldn't spend our resources to maintain Firefox accessibility for old platforms just in case.
Do we have other voices on this issue? (It doesn't seem we have a policy on this so extra input should be helpful to resolve it this or that way)
Flags: needinfo?
I'm pretty sure that ATK 1.x is what was used back in the old corba/bonobo days. In which case, we're not talking about maintaining support for old stuff; we're talking about maintaining support for dead stuff (insofar as no Orca user in his/her right mind would be using Orca from way back then). ATK 2.1 (the minimum version required for the requested role) is old. ;)

(My $0.02)
Flags: needinfo?
Hi.

Joanie pointed me this bug. So some comments:

1. About Trevor suggestion on comment 7 about using atk_get_version. Some releases ago (2.8) we added more version goodness. You can see it at atk-version.h. For doing what suggested probably ATK_CHECK_VERSION is a better option. Although of course, that is not possible right now, as Firefox is using 1.XX

2. About Alex Surkov concerns of multiple paths. FWIW, webkitgtk have something of this, using ATK_CHECK_VERSION. For example, to handle a recently added roles (still on unstable release):
http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp#L707

but it is true that the idea is having those multiple path as a temporal thing, as webkitgtk does dependencies bump more often. So that path will be there for just some cycles. 

3. As Joanie said, ATK 1.x is old. First atk 2.x (that btw, didn't break atk 1.x API or ABI), atk 2.0, is from April 2011. Almost 4 years. I agree that Firefox shouldn't use an unstable release, but imho, using a four years old API, that had several improvements since then, seems being too conservative to me. Also, note the "improvements". Some new APIs were added, and several methods were deprecated. I hope that eventually Firefox will bump the atk dependency. As much as Firefox waits, more work would be accumulated.

4. AFAIK, there is a bug open about moving to gtk3. atk 2.0 came out when gtk3 appear. If firefox is open to bump their gtk dependency, why not atk?
* I'm ok with backward compatibility as long as it makes sense, especially when it can be done at compilation time.

* I tried to update ATK headers in bug 843926 but I ran into some problems I couldn't resolve. If you have  ideas then it'd be awesome.

* I think the patch should be landed without backward compatibility stuff per comment #14, comment #15.
> 1. About Trevor suggestion on comment 7 about using atk_get_version. Some
> releases ago (2.8) we added more version goodness. You can see it at
> atk-version.h. For doing what suggested probably ATK_CHECK_VERSION is a

no, its not. What roes are understood by the version of atk on the build machine doesn't matter to us, what matters is the roles understood by the version of atk on the machine where firefox is running.

> better option. Although of course, that is not possible right now, as
> Firefox is using 1.XX

not exactly, the build machines are cent OS 6 so we link against atk 1.30.  As for the in tree headers those are sort of a mixture of versions, we've pulled in newer headers as bugs require it.

> 2. About Alex Surkov concerns of multiple paths. FWIW, webkitgtk have
> something of this, using ATK_CHECK_VERSION. For example, to handle a
> recently added roles (still on unstable release):
> http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/atk/
> WebKitAccessibleWrapperAtk.cpp#L707
> 
> but it is true that the idea is having those multiple path as a temporal
> thing, as webkitgtk does dependencies bump more often. So that path will be
> there for just some cycles. 

yeah, nobody wants to keep backward compat stuff forever, but one shouldn't hold ones breath when rhel is involved.

> 3. As Joanie said, ATK 1.x is old. First atk 2.x (that btw, didn't break atk
> 1.x API or ABI), atk 2.0, is from April 2011. Almost 4 years. I agree that
> Firefox shouldn't use an unstable release, but imho, using a four years old
> API, that had several improvements since then, seems being too conservative
> to me. Also, note the "improvements". Some new APIs were added, and several
> methods were deprecated. I hope that eventually Firefox will bump the atk
> dependency. As much as Firefox waits, more work would be accumulated.

I don't think anyone is arguing for not using newer bits of the API.

> 4. AFAIK, there is a bug open about moving to gtk3. atk 2.0 came out when
> gtk3 appear. If firefox is open to bump their gtk dependency, why not atk?

I haven't been paying too much attention to that bug, but afaik using gtk2 will still be supported for a while (current minimum gtk requirement is 2.10 we don't build official firefox against it, but red hat does for rhel 5)

(In reply to alexander :surkov from comment #12)
> I'd say those people should use ESR builds as long as they can and then they
> should update their systems since basically it's a requirement of ESR: if
> you want to stay secure and use latest Firefox then you should make sure
> your system fits it. So I wouldn't spend our resources to maintain Firefox
> accessibility for old platforms just in case.

I don't see where it (what exactly is it?) being a requirement comes from.  afaik we've never had an official minimum version requirement other than inheriting the minimum version the version of gtk we build against requires.   Currently we build against gtk 2.18 which iirc requires atk 1.<ancient>  and we support building against gtk 2.10 which requires an even more ancient version of atk.  The only people I'm aware of who use gtk 2.10 have atk 1.12 so I don't think we need to support compiling and linking against anything before that.  I don't care at all about being accessible there, but I'd prefer firefox didn't crash when accessible was enabled for whatever reason.

btw people don't always get a choice in what systems they use (e.g. library computers and school computers)

(In reply to Joanmarie Diggs from comment #14)
> I'm pretty sure that ATK 1.x is what was used back in the old corba/bonobo
> days. In which case, we're not talking about maintaining support for old
> stuff; we're talking about maintaining support for dead stuff (insofar as no
> Orca user in his/her right mind would be using Orca from way back then). ATK
> 2.1 (the minimum version required for the requested role) is old. ;)

yeah, iirc cent OS 6 shipps gnome 2.32 so corba stuff, and yes I agree its dead and deserves to be buried, but saddly not everyone agrees.

So I guess I should summerize.

- I believe we have to build against atk 1.12  (and need to build against 1.30 to not burn the tree)
- building against a version of atk only means we don't directly reference symbols only available in later versions, we can use constants and whatever from any version, but we should be careful to not cause crashes.
- we don't need to care about being accessible with atk before 2.1 or so, I care a little about 1.30 because of cent os but not much.
- Even if we don't need to be accessible we shouldn't crash when there's atk < 2.1 or 1.30.
> * I tried to update ATK headers in bug 843926 but I ran into some problems I
> couldn't resolve. If you have  ideas then it'd be awesome.

My feeling is that its going to be easiest to deal with piece by piece as we need newer things.

> * I think the patch should be landed without backward compatibility stuff
> per comment #14, comment #15.

I'd still prefer  we avoided returning roles that don't exist in the version of atk we're dealing with.  Not really because I think its important to be accessible, but I worry about crashes if  say the role we return is used as an index into an array.  That said I'm perfectly fine with doing that work.
(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> > 1. About Trevor suggestion on comment 7 about using atk_get_version. Some
> > releases ago (2.8) we added more version goodness. You can see it at
> > atk-version.h. For doing what suggested probably ATK_CHECK_VERSION is a
> 
> no, its not. What roes are understood by the version of atk on the build
> machine doesn't matter to us, what matters is the roles understood by the
> version of atk on the machine where firefox is running.

I'm curious what webkit makes different?

> (In reply to alexander :surkov from comment #12)
> > I'd say those people should use ESR builds as long as they can and then they
> > should update their systems since basically it's a requirement of ESR: if
> > you want to stay secure and use latest Firefox then you should make sure
> > your system fits it. So I wouldn't spend our resources to maintain Firefox
> > accessibility for old platforms just in case.
> 
> I don't see where it (what exactly is it?) being a requirement comes from. 
> afaik we've never had an official minimum version requirement other than
> inheriting the minimum version the version of gtk we build against requires.

It's not about minimal versions, we recommend to use latest Firefox version and latest screen reader. We don't promise any maintenance for different kind of combinations like new firefox + old screen reader. That's our policy in case of NVDA. I don't see why we should do differently in case of Orca.
(In reply to Trevor Saunders (:tbsaunde) from comment #18)
> > * I tried to update ATK headers in bug 843926 but I ran into some problems I
> > couldn't resolve. If you have  ideas then it'd be awesome.
> 
> My feeling is that its going to be easiest to deal with piece by piece as we
> need newer things.

just replacing all at once and don't care about this in future is an easy way (supposed to be)

> > * I think the patch should be landed without backward compatibility stuff
> > per comment #14, comment #15.
> 
> I'd still prefer  we avoided returning roles that don't exist in the version
> of atk we're dealing with.  Not really because I think its important to be
> accessible, but I worry about crashes if  say the role we return is used as
> an index into an array.  That said I'm perfectly fine with doing that work.

I have a feeling it's an overthinking as you trying to solve the problem that don't need to be solved. If we really get crashes then our crash reporter should ping us about that.
(In reply to alexander :surkov from comment #20)
> (In reply to Trevor Saunders (:tbsaunde) from comment #18)
> > > * I tried to update ATK headers in bug 843926 but I ran into some problems I
> > > couldn't resolve. If you have  ideas then it'd be awesome.
> > 
> > My feeling is that its going to be easiest to deal with piece by piece as we
> > need newer things.
> 
> just replacing all at once and don't care about this in future is an easy
> way (supposed to be)
> 
> > > * I think the patch should be landed without backward compatibility stuff
> > > per comment #14, comment #15.
> > 
> > I'd still prefer  we avoided returning roles that don't exist in the version
> > of atk we're dealing with.  Not really because I think its important to be
> > accessible, but I worry about crashes if  say the role we return is used as
> > an index into an array.  That said I'm perfectly fine with doing that work.
> 
> I have a feeling it's an overthinking as you trying to solve the problem
> that don't need to be solved. If we really get crashes then our crash
> reporter should ping us about that.

maybe, on the other hand its not hard to not run the risk so why risk it?

(In reply to alexander :surkov from comment #19)
> (In reply to Trevor Saunders (:tbsaunde) from comment #17)
> > > 1. About Trevor suggestion on comment 7 about using atk_get_version. Some
> > > releases ago (2.8) we added more version goodness. You can see it at
> > > atk-version.h. For doing what suggested probably ATK_CHECK_VERSION is a
> > 
> > no, its not. What roes are understood by the version of atk on the build
> > machine doesn't matter to us, what matters is the roles understood by the
> > version of atk on the machine where firefox is running.
> 
> I'm curious what webkit makes different?

I suspect when they use headers for atk > 2.1 they also link against atk > 2.1 and so they won't even run if you don't have that version of atk.

> > (In reply to alexander :surkov from comment #12)
> > > I'd say those people should use ESR builds as long as they can and then they
> > > should update their systems since basically it's a requirement of ESR: if
> > > you want to stay secure and use latest Firefox then you should make sure
> > > your system fits it. So I wouldn't spend our resources to maintain Firefox
> > > accessibility for old platforms just in case.
> > 
> > I don't see where it (what exactly is it?) being a requirement comes from. 
> > afaik we've never had an official minimum version requirement other than
> > inheriting the minimum version the version of gtk we build against requires.
> 
> It's not about minimal versions, we recommend to use latest Firefox version
> and latest screen reader. We don't promise any maintenance for different
> kind of combinations like new firefox + old screen reader. That's our policy
> in case of NVDA. I don't see why we should do differently in case of Orca.

Sure, best accessibility will be with new orca, with old version of API we should follow its rules so we don't crash or cause other breakage.
(In reply to Trevor Saunders (:tbsaunde) from comment #21)
> > I have a feeling it's an overthinking as you trying to solve the problem
> > that don't need to be solved. If we really get crashes then our crash
> > reporter should ping us about that.
> 
> maybe, on the other hand its not hard to not run the risk so why risk it?

it looks like quite a low risk (comment #4), thus I wouldn't worry about it

> (In reply to alexander :surkov from comment #19)

> > > no, its not. What roes are understood by the version of atk on the build
> > > machine doesn't matter to us, what matters is the roles understood by the
> > > version of atk on the machine where firefox is running.
> > 
> > I'm curious what webkit makes different?
> 
> I suspect when they use headers for atk > 2.1 they also link against atk >
> 2.1 and so they won't even run if you don't have that version of atk.

can we do the same?

> > > (In reply to alexander :surkov from comment #12)
> > It's not about minimal versions, we recommend to use latest Firefox version
> > and latest screen reader. We don't promise any maintenance for different
> > kind of combinations like new firefox + old screen reader. That's our policy
> > in case of NVDA. I don't see why we should do differently in case of Orca.
> 
> Sure, best accessibility will be with new orca, with old version of API we
> should follow its rules so we don't crash or cause other breakage.

We can't be perfect and probably we shouldn't be. We should maintain backward compatibility code but only when it's really necessary.
(In reply to alexander :surkov from comment #22)
> (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> > > I have a feeling it's an overthinking as you trying to solve the problem
> > > that don't need to be solved. If we really get crashes then our crash
> > > reporter should ping us about that.
> > 
> > maybe, on the other hand its not hard to not run the risk so why risk it?
> 
> it looks like quite a low risk (comment #4), thus I wouldn't worry about it

I'm not really sure why you think that, a quick check of the dbus code to get the atk role shows it has an array of size ATK_ROLE_LAST_DEFINED which of course would break with roles it doesn't know about.  Now, only the corba code is relevant for this particular role, but it seems pretty reasonable to expect it will do the same.  In any case it would have been faster to just add the checks than do all this argueing...

> > (In reply to alexander :surkov from comment #19)
> 
> > > > no, its not. What roes are understood by the version of atk on the build
> > > > machine doesn't matter to us, what matters is the roles understood by the
> > > > version of atk on the machine where firefox is running.
> > > 
> > > I'm curious what webkit makes different?
> > 
> > I suspect when they use headers for atk > 2.1 they also link against atk >
> > 2.1 and so they won't even run if you don't have that version of atk.
> 
> can we do the same?

I don't think either of us gets to choose what versions of linux the firefox binaries we distribute run on, so no.

> > > > (In reply to alexander :surkov from comment #12)
> > > It's not about minimal versions, we recommend to use latest Firefox version
> > > and latest screen reader. We don't promise any maintenance for different
> > > kind of combinations like new firefox + old screen reader. That's our policy
> > > in case of NVDA. I don't see why we should do differently in case of Orca.
> > 
> > Sure, best accessibility will be with new orca, with old version of API we
> > should follow its rules so we don't crash or cause other breakage.
> 
> We can't be perfect and probably we shouldn't be. We should maintain
> backward compatibility code but only when it's really necessary.

When the code is to work nicely I agree, but I think we should try reasonably hard to not crash in odd cases.
Whiteboard: scr
(In reply to Trevor Saunders (:tbsaunde) from comment #23)

> > > maybe, on the other hand its not hard to not run the risk so why risk it?
> > 
> > it looks like quite a low risk (comment #4), thus I wouldn't worry about it
> 
> I'm not really sure why you think that, a quick check of the dbus code to
> get the atk role shows it has an array of size ATK_ROLE_LAST_DEFINED which
> of course would break with roles it doesn't know about.  Now, only the corba
> code is relevant for this particular role, but it seems pretty reasonable to
> expect it will do the same.  In any case it would have been faster to just
> add the checks than do all this argueing...

I rely on comment #4: the checks you suggest to introduce are for dead code

> > > (In reply to alexander :surkov from comment #19)

> > > > I'm curious what webkit makes different?
> > > 
> > > I suspect when they use headers for atk > 2.1 they also link against atk >
> > > 2.1 and so they won't even run if you don't have that version of atk.
> > 
> > can we do the same?
> 
> I don't think either of us gets to choose what versions of linux the firefox
> binaries we distribute run on, so no.

Is the difference between webkit-based browsers and Firefox is in distribution policy? I'm still not sure though what exact difference is.
(In reply to alexander :surkov from comment #24)
> (In reply to Trevor Saunders (:tbsaunde) from comment #23)
> 
> > > > maybe, on the other hand its not hard to not run the risk so why risk it?
> > > 
> > > it looks like quite a low risk (comment #4), thus I wouldn't worry about it
> > 
> > I'm not really sure why you think that, a quick check of the dbus code to
> > get the atk role shows it has an array of size ATK_ROLE_LAST_DEFINED which
> > of course would break with roles it doesn't know about.  Now, only the corba
> > code is relevant for this particular role, but it seems pretty reasonable to
> > expect it will do the same.  In any case it would have been faster to just
> > add the checks than do all this argueing...
> 
> I rely on comment #4: the checks you suggest to introduce are for dead code
> 
> > > > (In reply to alexander :surkov from comment #19)
> 
> > > > > I'm curious what webkit makes different?
> > > > 
> > > > I suspect when they use headers for atk > 2.1 they also link against atk >
> > > > 2.1 and so they won't even run if you don't have that version of atk.
> > > 
> > > can we do the same?
> > 
> > I don't think either of us gets to choose what versions of linux the firefox
> > binaries we distribute run on, so no.
> 
> Is the difference between webkit-based browsers and Firefox is in
> distribution policy? I'm still not sure though what exact difference is.

that is the route of it yes (I'm pretty sure you can't get useful binaries out of webkit.org or whatever) you get webkit built for your particular version of linux.
err, sorry only responded to half :(
(In reply to Trevor Saunders (:tbsaunde) from comment #25)
> (In reply to alexander :surkov from comment #24)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #23)
> > 
> > > > > maybe, on the other hand its not hard to not run the risk so why risk it?
> > > > 
> > > > it looks like quite a low risk (comment #4), thus I wouldn't worry about it
> > > 
> > > I'm not really sure why you think that, a quick check of the dbus code to
> > > get the atk role shows it has an array of size ATK_ROLE_LAST_DEFINED which
> > > of course would break with roles it doesn't know about.  Now, only the corba
> > > code is relevant for this particular role, but it seems pretty reasonable to
> > > expect it will do the same.  In any case it would have been faster to just
> > > add the checks than do all this argueing...
> > 
> > I rely on comment #4: the checks you suggest to introduce are for dead code

if you take as a given that we don't require newer dependancies for the binaries on mozilla.org or require newer dependancies to build for a good reason then that's trivially false.
(In reply to Trevor Saunders (:tbsaunde) from comment #26)

> > > I rely on comment #4: the checks you suggest to introduce are for dead code
> 
> if you take as a given that we don't require newer dependancies for the
> binaries on mozilla.org or require newer dependancies to build for a good
> reason then that's trivially false.

can you elaborate this please? Do you talk about ATK_ROLE_LIST_BOX role or in general?
Flags: needinfo?(trev.saunders)
Comment on attachment 8370368 [details] [diff] [review]
bug 960228 - use ATK_ROLE_LIST_BOX and add machinary to detct loaded atk version

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

::: accessible/src/atk/AccessibleWrap.cpp
@@ +689,5 @@
>  
>  #undef ROLE
>  
> +  if (aAtkObj->role == ATK_ROLE_LIST_BOX && !IsAtkVersionAtLeast(2, 1))
> +    aAtkObj->role = ATK_ROLE_LIST;

the more the merrier, I wish we hadn't it, but I'm ok as long as you're strong on it

::: accessible/src/atk/Platform.cpp
@@ +152,5 @@
> +    const char* version = atkGetVersion();
> +    if (version) {
> +      char* endPtr = nullptr;
> +      atkMajorVersion = strtol(version, &endPtr, 10);
> +      if (endPtr != version && *endPtr == '.')

it's likely you don't need first part since atk version is not supposed to start from '.'? But even if it is then major version failed anyway and no one IsAtkVersionAtLeast check should pass
Attachment #8370368 - Flags: review?(surkov.alexander) → review+
https://hg.mozilla.org/mozilla-central/rev/7b983e7bf06f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Flags: needinfo?(trev.saunders)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: