Closed
Bug 1489324
Opened 6 years ago
Closed 6 years ago
Add collection-related attributes to group container
Categories
(Core :: Disability Access APIs, enhancement)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: eeejay, Assigned: eeejay)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
23.94 KB,
patch
|
Jamie
:
review+
|
Details | Diff | Splinter Review |
In Android some of the group information is in the parent and not the items: 1. The group size (ie. setsize in items) is part of the parent's attributes. 2. The container has a flag that tells if the set is hierarchical, for example nested lists. If I understand, this may be useful in windows too. If not, this could be added in the android platform wrappers exclusively.
Comment 1•6 years ago
|
||
This was discussed for Windows in bug 764757 (starting points: bug 764757 comment 5, bug 764757 comment 17). As we've discussed, there are some annoying edge cases (like trees and menus with separated groups where there are no hierarchical containers) which make this a bit tricky. If we do expose this on other platforms, we should consider using a name other than setsize (e.g. itemcount as suggested by Alex in bug 764757 comment 17), just to make it clear that there's a difference (and so we can document any potential pitfalls more easily).
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #9008489 -
Flags: review?(jteh)
Assignee | ||
Comment 3•6 years ago
|
||
I forgot to mention above that one case this patch doesn't work well is with children with aria-setsize. There is really no established way of determining the total item count if not all children have the same setsize value.
Comment 4•6 years ago
|
||
Comment on attachment 9008489 [details] [diff] [review] Add 'child-item-count' and 'hierarchal' attributes to collection containers. r?Jamie Review of attachment 9008489 [details] [diff] [review]: ----------------------------------------------------------------- While hierarchal is a valid word (relating to a hierarch), the correct word here is hierarchical. Sorry; I know that's a *lot* of changes in this diff. :) ::: accessible/base/AccGroupInfo.cpp @@ +239,5 @@ > + AccGroupInfo* groupInfo = childItem->GetGroupInfo(); > + if (groupInfo) { > + Accessible* parent = groupInfo->ConceptualParent(); > + if (groupInfo->SetSize() && (!parent || parent == aContainer)) { > + itemCount++; When we discussed these attributes, you noted it wouldn't hurt performance because it only has to look at the first item. This appears to be looking at all children. I guess we'll probably end up calculating group info later anyway and we do cache it, but can you give me reasonable assurance this isn't going to adversely affect performance on other platforms? Keep in mind that Windows screen readers will fetch attributes for almost every accessible in the tree. Also, why not just use the SetSize reported by the first item? I'm sure there's a reason, but I'm missing it. Perhaps this is because of containers containing multiple groups, but as you noted, this isn't going to work so well anyway. ::: accessible/generic/Accessible.cpp @@ +1057,5 @@ > + uint32_t itemCount = AccGroupInfo::TotalItemCount(this, &hierarchal); > + if (itemCount) { > + nsAutoString itemCountStr; > + itemCountStr.AppendInt(itemCount); > + attributes->SetStringProperty(NS_LITERAL_CSTRING("child-item-count"), In an age where we're caring more about memory, should we be putting these attributes in nsgkAtoms? I'm not sure what the guidance is here.
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to James Teh [:Jamie] from comment #4) > Comment on attachment 9008489 [details] [diff] [review] > Add 'child-item-count' and 'hierarchal' attributes to collection containers. > r?Jamie > > Review of attachment 9008489 [details] [diff] [review]: > ----------------------------------------------------------------- > > While hierarchal is a valid word (relating to a hierarch), the correct word > here is hierarchical. Sorry; I know that's a *lot* of changes in this diff. > :) > > ::: accessible/base/AccGroupInfo.cpp > @@ +239,5 @@ > > + AccGroupInfo* groupInfo = childItem->GetGroupInfo(); > > + if (groupInfo) { > > + Accessible* parent = groupInfo->ConceptualParent(); > > + if (groupInfo->SetSize() && (!parent || parent == aContainer)) { > > + itemCount++; > > When we discussed these attributes, you noted it wouldn't hurt performance > because it only has to look at the first item. This appears to be looking at > all children. I guess we'll probably end up calculating group info later > anyway and we do cache it, but can you give me reasonable assurance this > isn't going to adversely affect performance on other platforms? Keep in mind > that Windows screen readers will fetch attributes for almost every > accessible in the tree. > I thought about this. I decided to limit this calculation only to certain collection roles. Not sure what the impact would be.. > Also, why not just use the SetSize reported by the first item? I'm sure > there's a reason, but I'm missing it. Perhaps this is because of containers > containing multiple groups, but as you noted, this isn't going to work so > well anyway. > Right, it would not work for multiple group containers, specifically sets that have separators in between. That seems like a common pattern. So enumerating over all the children makes sure we don't just count the first group, but all the separator delimited ones. What I meant earlier about aria-setsize is that any child can have a number arbitrarily set with aria-setsize and we wouldn't know which one is the canonical one. That should probably be considered bad markup, but it's not impossible. It gets more complicated if there are both aria-setsize children and role=separator elements. Since we would need to divine the author's intention: did they mean to use aria-setsize to unify all the items in the container, or should they be counted separately? > ::: accessible/generic/Accessible.cpp > @@ +1057,5 @@ > > + uint32_t itemCount = AccGroupInfo::TotalItemCount(this, &hierarchal); > > + if (itemCount) { > > + nsAutoString itemCountStr; > > + itemCountStr.AppendInt(itemCount); > > + attributes->SetStringProperty(NS_LITERAL_CSTRING("child-item-count"), > > In an age where we're caring more about memory, should we be putting these > attributes in nsgkAtoms? I'm not sure what the guidance is here. Probably. I didn't see any existing fitting name there, and that file scares me. I'll add something..
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to James Teh [:Jamie] from comment #4) > Comment on attachment 9008489 [details] [diff] [review] > Add 'child-item-count' and 'hierarchal' attributes to collection containers. > r?Jamie > > Review of attachment 9008489 [details] [diff] [review]: > ----------------------------------------------------------------- > > While hierarchal is a valid word (relating to a hierarch), the correct word > here is hierarchical. Sorry; I know that's a *lot* of changes in this diff. > :) > Haha. I'll fix that too.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → eitan
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #5) > (In reply to James Teh [:Jamie] from comment #4) > > In an age where we're caring more about memory, should we be putting these > > attributes in nsgkAtoms? I'm not sure what the guidance is here. > > Probably. I didn't see any existing fitting name there, and that file scares > me. I'll add something.. What do you think if I use 'count' instead of 'child-item-count'? less explicit, but we already have an atom for it. And it seems concise and to the point. And likewise, what if i use tree=true for 'hierarchical'?
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(jteh)
Comment 8•6 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #7) > What do you think if I use 'count' instead of 'child-item-count'? less > explicit, but we already have an atom for it. And it seems concise and to > the point. And likewise, what if i use tree=true for 'hierarchical'? As discussed, I'd rather not do this; it's potentially confusing for a11y client developers. If you don't get a response concerning the convention on whether or not to add atoms, just do the original literal strings for now. (In reply to Eitan Isaacson [:eeejay] from comment #5) > I thought about this. I decided to limit this calculation only to certain > collection roles. Not sure what the impact would be.. As discussed, in the separators case, the item number is not valid after the first separator anyway, so let's just use the set size of the first item, at least for now. Please file a follow up to investigate/deal with this weirdness.
Flags: needinfo?(jteh)
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #9010349 -
Flags: review?(jteh)
Assignee | ||
Updated•6 years ago
|
Attachment #9008489 -
Attachment is obsolete: true
Attachment #9008489 -
Flags: review?(jteh)
Comment 10•6 years ago
|
||
Comment on attachment 9010349 [details] [diff] [review] Add 'child-item-count' and 'hierarchical' attributes to collection containers. r?Jamie Review of attachment 9010349 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: accessible/base/AccGroupInfo.h @@ +69,5 @@ > */ > static Accessible* FirstItemOf(const Accessible* aContainer); > > + /** > + * Return next item of the same group to the given item. Fix this comment please. :)
Attachment #9010349 -
Flags: review?(jteh) → review+
Comment 11•6 years ago
|
||
Pushed by eisaacson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/df90cba28373 Add 'child-item-count' and 'hierarchical' attributes to collection containers. r=Jamie
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df90cba28373
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in
before you can comment on or make changes to this bug.
Description
•