Last Comment Bug 577727 - Make pinned tabs distinguishable from other tabs for accessibility
: Make pinned tabs distinguishable from other tabs for accessibility
Status: VERIFIED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla24
Assigned To: David Bolter [:davidb]
:
Mentors:
Depends on: IA2_1.3
Blocks: ia2
  Show dependency treegraph
 
Reported: 2010-07-09 11:45 PDT by Marco Zehe (:MarcoZ)
Modified: 2013-09-17 06:52 PDT (History)
9 users (show)
mzehe: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
24+


Attachments
Early WIP (needs test) (2.89 KB, patch)
2010-11-15 12:49 PST, David Bolter [:davidb]
no flags Details | Diff | Review
patch 1 (3.55 KB, patch)
2010-11-16 06:20 PST, David Bolter [:davidb]
no flags Details | Diff | Review
patch 1 (5.78 KB, patch)
2010-11-16 06:23 PST, David Bolter [:davidb]
mzehe: review+
Details | Diff | Review
Expose new pinned state. (10.57 KB, patch)
2013-06-03 13:31 PDT, David Bolter [:davidb]
no flags Details | Diff | Review
expose IA2_STATE_PINNED (11.31 KB, patch)
2013-06-04 10:18 PDT, David Bolter [:davidb]
mzehe: review+
Details | Diff | Review
patch to land (r=marcoz) (12.49 KB, patch)
2013-06-05 07:49 PDT, David Bolter [:davidb]
no flags Details | Diff | Review
follow up and driveby comment cleanup (2.40 KB, patch)
2013-06-05 12:17 PDT, David Bolter [:davidb]
tbsaunde+mozbugs: review+
Details | Diff | Review

Description Marco Zehe (:MarcoZ) 2010-07-09 11:45:05 PDT
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] 2010-07-09 12:13:25 PDT
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] 2010-11-15 12:49:07 PST
Created attachment 490660 [details] [diff] [review]
Early WIP (needs test)
Comment 3 alexander :surkov 2010-11-15 20:27:06 PST
(In reply to comment #2)
> Created attachment 490660 [details] [diff] [review]
> Early WIP (needs test)

How does it help?
Comment 4 Marco Zehe (:MarcoZ) 2010-11-16 05:57:28 PST
(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] 2010-11-16 06:07:44 PST
Also, just a WIP. Patch doesn't work yet.
Comment 6 David Bolter [:davidb] 2010-11-16 06:18:29 PST
Oh, it does work if I compile it :) Coffee!
Comment 7 David Bolter [:davidb] 2010-11-16 06:20:39 PST
Created attachment 490866 [details] [diff] [review]
patch 1
Comment 8 David Bolter [:davidb] 2010-11-16 06:23:31 PST
Created attachment 490868 [details] [diff] [review]
patch 1

Added test.
Comment 9 Marco Zehe (:MarcoZ) 2010-11-16 06:33:56 PST
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] 2010-11-16 07:04:04 PST
Jamie, will this help?
Comment 11 James Teh [:Jamie] 2010-11-16 13:43:00 PST
(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 2010-11-16 15:35:53 PST
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] 2010-11-16 15:43:23 PST
(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] 2010-11-18 11:29:23 PST
IA2 list thread:
https://lists.linux-foundation.org/pipermail/accessibility-ia2/2010-November/001241.html
Comment 15 David Bolter [:davidb] 2010-12-13 15:55:12 PST
(Note: IA2_STATE_ICONIFIED)
Comment 16 David Bolter [:davidb] 2013-06-03 13:31:38 PDT
Created attachment 757584 [details] [diff] [review]
Expose new pinned state.

Had a two hour meeting :)

Let's start with Marco for review.
Comment 17 alexander :surkov 2013-06-03 19:20:21 PDT
you forgot to expose it in IA2
Comment 18 David Bolter [:davidb] 2013-06-04 06:35:43 PDT
Ha!
Comment 19 David Bolter [:davidb] 2013-06-04 10:18:26 PDT
Created attachment 758020 [details] [diff] [review]
expose IA2_STATE_PINNED
Comment 20 David Bolter [:davidb] 2013-06-04 10:23:03 PDT
https://tbpl.mozilla.org/?tree=Try&rev=f2816486b9e8
Comment 21 Marco Zehe (:MarcoZ) 2013-06-04 10:41:31 PDT
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] 2013-06-04 10:48:33 PDT
(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] 2013-06-04 14:29:55 PDT
(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) 2013-06-04 16:30:35 PDT
(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) 2013-06-05 06:51:48 PDT
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] 2013-06-05 07:49:17 PDT
Created attachment 758580 [details] [diff] [review]
patch to land (r=marcoz)
Comment 28 Trevor Saunders (:tbsaunde) 2013-06-05 08:54:20 PDT
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] 2013-06-05 11:33:36 PDT
(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] 2013-06-05 12:14:17 PDT
Yeah seems so.
Comment 31 David Bolter [:davidb] 2013-06-05 12:17:55 PDT
Created attachment 758725 [details] [diff] [review]
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] 2013-06-05 13:36:17 PDT
https://hg.mozilla.org/mozilla-central/rev/f00dba4fef63
Comment 33 Trevor Saunders (:tbsaunde) 2013-06-05 13:47:30 PDT
(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) 2013-06-05 13:48:32 PDT
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 36 Ryan VanderMeulen [:RyanVM] 2013-06-10 12:51:13 PDT
https://hg.mozilla.org/mozilla-central/rev/2ece77961055
Comment 37 bhavana bajaj [:bajaj] 2013-06-21 13:08:12 PDT
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] 2013-06-21 13:15:55 PDT
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] 2013-06-26 10:12:02 PDT
(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] 2013-07-21 04:23:37 PDT
Verified fixed in Firefox 25.0a1 (2013-07-18)
Comment 41 Firefox Portable user 2013-09-17 06:52:24 PDT
I haven't noticed any changes from 23 to 24. What has changed?

Note You need to log in before you can comment on or make changes to this bug.