577727 - Make pinned tabs distinguishable from other tabs for accessibility

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Right now, there is no way to distinguish a pinned tab from a non-pinned one for the accessibility APIs. Is there something in the markup we can add ARIA magic to to make screen readers say "pinned tabs grouping" when entering the pinned tabs via the keyboard? For example, is there an extra hbox which could get role="groupbox" and "aria-label="pinned tabs" for example?

Comment 1

[James Teh [:Jamie]]

A few thoughts:
* The user visible name is "App Tab" from what I can see in current builds, so this is the name that should probably be used, unless this is changing?
* Normal Tabs should probably also be placed beneath a grouping; i.e. the App Tabs and Normal Tabs groupings would be siblings. The reason for this is that it generally makes sense to announce when entering a grouping, but it becomes overly verbose to announce when leaving a grouping, so this isn't done. Therefore, if normal tabs aren't placed in a grouping, with NVDA at least, the user won't know when they've moved to a normal tab.
* It may be better to call these groupings "App" and "Normal", removing the "Tabs" suffix. This is because it's already obvious that you're on a tab control, plus the Browser Tabs toolbar is named. This eliminates redundant information and therefore unnecessary verbosity. This point is a bit subjective, though, and is only a suggestion.

Comment 2

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

Attachment: Early WIP (needs test)

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> Created attachment 490660 [details] [diff] [review]
> Early WIP (needs test)

How does it help?

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to comment #3)
> (In reply to comment #2)
> > Created attachment 490660 [details] [diff] [review] [details]
> > Early WIP (needs test)
> 
> How does it help?

Right now, there is no way for screen readers to distinguish a regular tab from one that is pinned to the left side of the tab bar. When a tab is pinned, it receives the attribute "pinned" set to "true". Since this is no ARIA attribute, we do not expose it as an object attribute on the accessible object. So our thinking was that we'd expose an attribute named "pinned" on those tab accessibles that have a pinned="true" set on them. We first thought about putting logic into the XBL or JS that controls this, by simply using an ARIA attribute we do not know about in the accessible module, but it turns out that this is being set and removed in so many places across so many files that we thought it's easier to put logic into nsXULTabAccessible instead.

Whenever an AT sees the object attribute "pinned" set to "true", they can speak soomething special or make a sound or whatever they want to indicate to their users that this tab is a pinned one.

Comment 5

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

Also, just a WIP. Patch doesn't work yet.

Comment 6

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

Oh, it does work if I compile it :) Coffee!

Comment 7

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

Attachment: patch 1

Comment 8

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

Attachment: patch 1

Added test.

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 490868 [details] [diff] [review]
patch 1

r=me. I first thought we should only create the object attribute ONLY if its value is "true", but passing whatever the author sets is the better approach.

Comment 10

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

Jamie, will this help?

Comment 11

[James Teh [:Jamie]]

(In reply to comment #10)
> Jamie, will this help?
The question is really whether "pinned" is a standard concept. If it is specific to only a few applications, I'd argue this should be embedded in the label or description of the control, as ATs shouldn't be implementing support for non-standard states for only a handful of applications. However, I suspect the idea of pinning things is becoming more common, as Windows 7's interface supports pinning and I believe the Mac has some idea of pinning as well.

In short, an object attribute is fine, but I think we should query the IA2 group to see whether others are happy with this becoming a standard object attribute. Technically, it really should be a state.

Comment 12

[alexander :surkov (:asurkov)]

Originally I thought this should be fixed by ARIA/XUL markup similar to how it was suggested in comment #0 and comment #1. So I was confused by approach taken in the patch. But if the idea of pinned stuff gets more common like Jamie said in comment #11 then I'm fine with the approach. Though, guys, you should give more details on approaches you keep in mind, otherwise until Jamie gave some clarifications it wasn't clear for me. About technical side: I think it's not nice to expose pinned by object attributes since boolean values doesn't look good there. The state might be preferable (per Jamie comment).

Comment 13

[James Teh [:Jamie]]

(In reply to comment #12)
> you should give
> more details on approaches you keep in mind, otherwise until Jamie gave some
> clarifications it wasn't clear for me.
I knew nothing of this change in approach either until Marco explained in comment #4. :)

> About technical side: I think it's not
> nice to expose pinned by object attributes since boolean values doesn't look
> good there. The state might be preferable (per Jamie comment).
On second thoughts, if this really is something that should be standard, it should definitely be a state. If it's an object attribute, that suggests that it is a custom hack, which I would be inclined not to support and should instead be exposed as per comment #0/comment #1.

Comment 14

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

IA2 list thread:
https://lists.linux-foundation.org/pipermail/accessibility-ia2/2010-November/001241.html

Comment 15

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

(Note: IA2_STATE_ICONIFIED)

Comment 16

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

Attachment: Expose new pinned state.

Had a two hour meeting :)

Let's start with Marco for review.

Comment 17

[alexander :surkov (:asurkov)]

you forgot to expose it in IA2

Comment 18

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

Ha!

Comment 19

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

Attachment: expose IA2_STATE_PINNED

Comment 20

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

https://tbpl.mozilla.org/?tree=Try&rev=f2816486b9e8

Comment 21

[Marco Zehe (:MarcoZ)]

Comment on attachment 758020 [details] [diff] [review]
expose IA2_STATE_PINNED

Tentative r+, logic and tests seem quite all right. I will wait for the try build to finish. You may have to do a new one, though, because:

>diff --git a/accessible/public/nsIAccessibleStates.idl b/accessible/public/nsIAccessibleStates.idl

You need to update the UUID for this IDL for this change.

Also I'm hoping that NVDA already does something with the IA2 state "pinned".

Comment 22

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

(In reply to Marco Zehe (:MarcoZ) from comment #21)

> Also I'm hoping that NVDA already does something with the IA2 state "pinned".

Doubt it. Chicken and egg ;)

Comment 23

[James Teh [:Jamie]]

(In reply to Marco Zehe (:MarcoZ) from comment #21)
> Also I'm hoping that NVDA already does something with the IA2 state "pinned".
Unfortunately, it doesn't. We haven't updated to the latest IA2 yet. I started preliminary work on this, but the initial version of the new IDL was broken and I never got around to resuming work on it.

That said, if you press NVDA+f1 while the navigator object is on a tab, you'll see something like this:
IAccessible2 states: IA2_STATE_OPAQUE, IA2_STATE_HORIZONTAL (1040)
Pinned won't get a mention here yet, but the final number will be different. Assuming the same states as above, it'll be 525328 (1040 | 0x80000).

Comment 24

[Trevor Saunders (:tbsaunde)]

(In reply to Marco Zehe (:MarcoZ) from comment #21)
> Comment on attachment 758020 [details] [diff] [review]
> expose IA2_STATE_PINNED
> 
> Tentative r+, logic and tests seem quite all right. I will wait for the try
> build to finish. You may have to do a new one, though, because:

nah, absolutely nothing should use that iid for anything, and even if they did changing it is only important for things that aren't built into gecko.

> >diff --git a/accessible/public/nsIAccessibleStates.idl b/accessible/public/nsIAccessibleStates.idl
> 
> You need to update the UUID for this IDL for this change.

hygenically maybe its a good idea but not really necessary since constants don't effect ABI.

Comment 25

[Marco Zehe (:MarcoZ)]

Comment on attachment 758020 [details] [diff] [review]
expose IA2_STATE_PINNED

r=me provided the code gets adjusted to handle the case that pinned can have the attribute of "false", in which case the state should not be set. Discussed over IRC.

Comment 26

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

Attachment: patch to land (r=marcoz)

Comment 27

[Marco Zehe (:MarcoZ)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f00dba4fef63

Comment 28

[Trevor Saunders (:tbsaunde)]

Comment on attachment 758580 [details] [diff] [review]
patch to land (r=marcoz)

>+    if (mContent && mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::pinned) &&
>+        mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::pinned,
>+                              nsGkAtoms::_true, eCaseMatters))
>+      state |= states::PINNED;

HasAttr check isn't needed, and maybe not mContent one, but that needs checked.

Comment 29

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

(In reply to Trevor Saunders (:tbsaunde) from comment #28)
> Comment on attachment 758580 [details] [diff] [review]
> patch to land (r=marcoz)
> 
> >+    if (mContent && mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::pinned) &&
> >+        mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::pinned,
> >+                              nsGkAtoms::_true, eCaseMatters))
> >+      state |= states::PINNED;
> 
> HasAttr check isn't needed, and maybe not mContent one, but that needs
> checked.

Oh does AttrValueIs fail nicely when the attribute is missing?

Comment 30

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

Yeah seems so.

Comment 31

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

Attachment: follow up and driveby comment cleanup

Trevor how's this look? I opportunistically tweaked an atk comment that was bothering me too.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Trevor Saunders (:tbsaunde)]

(In reply to Trevor Saunders (:tbsaunde) from comment #28)
> Comment on attachment 758580 [details] [diff] [review]
> patch to land (r=marcoz)
> 
> >+    if (mContent && mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::pinned) &&
> >+        mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::pinned,
> >+                              nsGkAtoms::_true, eCaseMatters))
> >+      state |= states::PINNED;
> 
> HasAttr check isn't needed, and maybe not mContent one, but that needs
> checked.

actually, it should have been obvious mContent can't be null because tab can only be non-null if mContent was not null.

Comment 34

[Trevor Saunders (:tbsaunde)]

Comment on attachment 758725 [details] [diff] [review]
follow up and driveby comment cleanup

>-    if (mContent && mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::pinned) &&
>-        mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::pinned,
>-                              nsGkAtoms::_true, eCaseMatters))
>+    if (mContent && mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::pinned,
>+                                          nsGkAtoms::_true, eCaseMatters))

this is fine if you remove mContent check too since it can't possibly be false

Comment 35

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ece77961055

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ece77961055

Comment 37

[bhavana bajaj [:bajaj]]

note to self :Lump this with other accessibility bugs if the collection is significant.Check with :dbolter with accessibility features in 24.

Comment 38

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

Bhavana, also note: http://www.marcozehe.de/2013/06/21/new-features-for-talkback-users-in-firefox-for-android-24/
And Alex will probably have something on http://asurkov.blogspot.ca/

Comment 39

[bhavana bajaj [:bajaj]]

(In reply to David Bolter [:davidb] from comment #38)
> Bhavana, also note:
> http://www.marcozehe.de/2013/06/21/new-features-for-talkback-users-in-
> firefox-for-android-24/
> And Alex will probably have something on http://asurkov.blogspot.ca/

Thanks David, I will group all accessibility improvements as one note as I see few other bugs on this list as well(Bug 785852,Bug 803021).

Comment 40

[James Teh [:Jamie]]

Verified fixed in Firefox 25.0a1 (2013-07-18)

Comment 41

[Firefox Portable user]

I haven't noticed any changes from 23 to 24. What has changed?