Closed
Bug 577727
Opened 14 years ago
Closed 11 years ago
Make pinned tabs distinguishable from other tabs for accessibility
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
VERIFIED
FIXED
mozilla24
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 24+ |
People
(Reporter: MarcoZ, Assigned: davidb)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(2 files, 5 obsolete files)
12.49 KB,
patch
|
Details | Diff | Splinter Review | |
2.40 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → bolterbugz
Assignee | ||
Updated•14 years ago
|
Component: Tabbed Browser → Disability Access APIs
Product: Firefox → Core
QA Contact: tabbed.browser → accessibility-apis
Assignee | ||
Comment 2•14 years ago
|
||
Comment 3•14 years ago
|
||
(In reply to comment #2) > Created attachment 490660 [details] [diff] [review] > Early WIP (needs test) How does it help?
Reporter | ||
Comment 4•14 years ago
|
||
(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.
Assignee | ||
Comment 5•14 years ago
|
||
Also, just a WIP. Patch doesn't work yet.
Assignee | ||
Comment 6•14 years ago
|
||
Oh, it does work if I compile it :) Coffee!
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #490660 -
Attachment is obsolete: true
Attachment #490866 -
Flags: review?(marco.zehe)
Assignee | ||
Comment 8•14 years ago
|
||
Added test.
Attachment #490866 -
Attachment is obsolete: true
Attachment #490868 -
Flags: review?(marco.zehe)
Attachment #490866 -
Flags: review?(marco.zehe)
Reporter | ||
Comment 9•14 years ago
|
||
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.
Attachment #490868 -
Flags: review?(marco.zehe) → review+
Assignee | ||
Comment 10•14 years ago
|
||
Jamie, will this help?
Assignee | ||
Updated•14 years ago
|
Comment 11•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
(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.
Assignee | ||
Comment 14•14 years ago
|
||
IA2 list thread: https://lists.linux-foundation.org/pipermail/accessibility-ia2/2010-November/001241.html
Assignee | ||
Comment 15•14 years ago
|
||
(Note: IA2_STATE_ICONIFIED)
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 16•11 years ago
|
||
Had a two hour meeting :) Let's start with Marco for review.
Attachment #490868 -
Attachment is obsolete: true
Attachment #757584 -
Flags: review?(marco.zehe)
Comment 17•11 years ago
|
||
you forgot to expose it in IA2
Assignee | ||
Comment 18•11 years ago
|
||
Ha!
Assignee | ||
Updated•11 years ago
|
Attachment #757584 -
Flags: review?(marco.zehe)
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #757584 -
Attachment is obsolete: true
Attachment #758020 -
Flags: review?(marco.zehe)
Assignee | ||
Comment 20•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f2816486b9e8
Reporter | ||
Comment 21•11 years ago
|
||
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".
Assignee | ||
Comment 22•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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.
Reporter | ||
Comment 25•11 years ago
|
||
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.
Attachment #758020 -
Flags: review?(marco.zehe) → review+
Assignee | ||
Comment 26•11 years ago
|
||
Attachment #758020 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 27•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f00dba4fef63
Comment 28•11 years ago
|
||
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.
Assignee | ||
Comment 29•11 years ago
|
||
(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?
Assignee | ||
Comment 30•11 years ago
|
||
Yeah seems so.
Assignee | ||
Comment 31•11 years ago
|
||
Trevor how's this look? I opportunistically tweaked an atk comment that was bothering me too.
Attachment #758725 -
Flags: review?(trev.saunders)
Comment 32•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f00dba4fef63
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 33•11 years ago
|
||
(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•11 years ago
|
||
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
Attachment #758725 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 35•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ece77961055
Updated•11 years ago
|
relnote-firefox:
--- → ?
Comment 37•11 years ago
|
||
note to self :Lump this with other accessibility bugs if the collection is significant.Check with :dbolter with accessibility features in 24.
Assignee | ||
Comment 38•11 years ago
|
||
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•11 years ago
|
||
(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 41•11 years ago
|
||
I haven't noticed any changes from 23 to 24. What has changed?
You need to log in
before you can comment on or make changes to this bug.
Description
•