Use correct position when setting pos_in_set for tabs.

Use correct position when setting pos_in_set for tabs.

The aria spec says (of pos in set):
Authors MUST set the value for aria-posinset to an integer greater than or equal to 1, and less than or equal to the size of the set.

TEST=manual. Verify you don't get things like "... 0 of 5", and that you can get to "... 5 of 5".

[David Tseng, 2017-02-27 22:26:21]

Description was changed from

==========
Use correct position when setting pos_in_set for tabs.

BUG=
==========

to

==========
Use correct position when setting pos_in_set for tabs.

The aria says (of pos in set):
Authors MUST set the value for aria-posinset to an integer greater than or equal
to 1, and less than or equal to the size of the set.

TEST=manual. Verify you don't get things like "... 0 of 5", and that you can get
to "... 5 of 5".
==========

[David Tseng, 2017-02-27 22:26:21]

dtseng@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[David Tseng, 2017-02-27 22:27:37]

dtseng@chromium.org changed reviewers:
+ sky@chromium.org

[David Tseng, 2017-02-27 22:27:37]

One of those things that's not a big deal except for it sounds glaringly bad to
users.

sky for RS.

[David Tseng, 2017-02-27 22:28:19]

Description was changed from

==========
Use correct position when setting pos_in_set for tabs.

The aria says (of pos in set):
Authors MUST set the value for aria-posinset to an integer greater than or equal
to 1, and less than or equal to the size of the set.

TEST=manual. Verify you don't get things like "... 0 of 5", and that you can get
to "... 5 of 5".
==========

to

==========
Use correct position when setting pos_in_set for tabs.

The aria spec says (of pos in set):
Authors MUST set the value for aria-posinset to an integer greater than or equal
to 1, and less than or equal to the size of the set.

TEST=manual. Verify you don't get things like "... 0 of 5", and that you can get
to "... 5 of 5".
==========

[David Tseng, 2017-02-27 22:35:42]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 22:36:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 23:26:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-27 23:26:45]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-02-27 23:50:49]

lgtm

Yep, should be 1-based and not zero-based.

Should we clarify in any header files or ax_enums.h?

[sky, 2017-02-28 04:01:06]

https://codereview.chromium.org/2719223002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/2719223002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_strip.cc:1348: int pos_in_set = tab_count() ?
GetModelIndexOfTab(tab) + 1 : 0;
GetModelIndexOfTab may return -1 if tab is closing.

[sky, 2017-02-28 04:02:33]

https://codereview.chromium.org/2719223002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/2719223002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_strip.cc:1349:
node_data->AddIntAttribute(ui::AX_ATTR_POS_IN_SET, pos_in_set);
One more comment. Why do we need AX_ATTR_POS_IN_SET to be 1s based? Can't we
hide that in however the data is exported? Forcing everyone to deal with 1s
based is so easy to get wrong (as we just proved).

[David Tseng, 2017-02-28 19:26:53]

https://codereview.chromium.org/2719223002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/2719223002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_strip.cc:1348: int pos_in_set = tab_count() ?
GetModelIndexOfTab(tab) + 1 : 0;
On 2017/02/28 04:01:05, sky wrote:
> GetModelIndexOfTab may return -1 if tab is closing.

Added explicit check (which omits data if it is not satisfied).

https://codereview.chromium.org/2719223002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_strip.cc:1349:
node_data->AddIntAttribute(ui::AX_ATTR_POS_IN_SET, pos_in_set);
On 2017/02/28 04:02:33, sky wrote:
> One more comment. Why do we need AX_ATTR_POS_IN_SET to be 1s based? Can't we
> hide that in however the data is exported? Forcing everyone to deal with 1s
> based is so easy to get wrong (as we just proved).

It's spec'ed as 1-based in the relevant W3C doc (ARIA).  It's handled that way
in Blink, for example.

The -1 case is actually deeper than accessibility. The Tab becomes visible with
a tab index of -1, so I'm not really sure if I understand when a view should be
capable of providing accurate information about its state or if such a thing
exists or should exist.

I've added a note in the ax_enums.idl file and added an explicit check for the
expected values.

It's a pretty obscure attribute in the first place and I don't think your
general audience here would know to use it. The original change was made by
Dominic in the context of a larger refactor.

[dmazzoni, 2017-02-28 19:32:49]

>
> > One more comment. Why do we need AX_ATTR_POS_IN_SET to be 1s based?
> Can't we
> > hide that in however the data is exported? Forcing everyone to deal
> with 1s
> > based is so easy to get wrong (as we just proved).
>

Yeah, that's a tough call. As David said generally we try to follow a web
spec for any accessibility attributes because otherwise they're slightly
different on every platform and it's unclear what to pick. We store the
web-centric attribute internally, and then convert to what's desired on
each platform last.

Perhaps a simple rename like AX_ATTR_POS_IN_SET_1_BASED might prevent
confusion. That way we stay consistent with the most common usage of this
attribute, but someone just trying to update code in views/ gets a heads-up
that it's not the intuitive value.

If we wanted to switch to zero-based we should name it something totally
unrelated to "pos in set" because otherwise it'd be very confusing when
mapping to/from anything named "pos in set" that follows the 1-based spec,
like maybe INDEX_IN_CONTAINER or something like that.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2017-02-28 19:36:09]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 19:36:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-02-28 20:51:56]

+1 to renaming.

On Tue, Feb 28, 2017 at 11:32 AM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:
>> > One more comment. Why do we need AX_ATTR_POS_IN_SET to be 1s based?
>> Can't we
>> > hide that in however the data is exported? Forcing everyone to deal
>> with 1s
>> > based is so easy to get wrong (as we just proved).
>
>
> Yeah, that's a tough call. As David said generally we try to follow a web
> spec for any accessibility attributes because otherwise they're slightly
> different on every platform and it's unclear what to pick. We store the
> web-centric attribute internally, and then convert to what's desired on each
> platform last.
>
> Perhaps a simple rename like AX_ATTR_POS_IN_SET_1_BASED might prevent
> confusion. That way we stay consistent with the most common usage of this
> attribute, but someone just trying to update code in views/ gets a heads-up
> that it's not the intuitive value.
>
> If we wanted to switch to zero-based we should name it something totally
> unrelated to "pos in set" because otherwise it'd be very confusing when
> mapping to/from anything named "pos in set" that follows the 1-based spec,
> like maybe INDEX_IN_CONTAINER or something like that.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-02-28 20:52:30]

https://codereview.chromium.org/2719223002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/2719223002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1346: if (tab_count() <= 0 ||
GetModelIndexOfTab(tab) < 0)
Why the tab_count() <= 0? Should GetModelIndexOfTab(tab) cover what you want?

[commit-bot: I haz the power, 2017-02-28 21:03:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 21:03:42]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2017-02-28 22:12:01]

I'm willing to acknowledge the confusing naming of
|pos_in_set|. If we assume that's going to be a common issue for lots of
people, then a simple rename of the ax enum won't suffice. It is equally
confusing in Blink, extension (automation) api, and in ChromeVox.

I don't think renaming everything is within the scope of this cl; if you
really insist, then I'll follow up with a big rename cl. As a
middle ground, I've added a validated add to AXNodeData and a new index
in set attribute for the three instances where it's being used. They
were all wrong and missed on the initial review, so it's a good point to
make this clearer.

PTAL.


sky@chromium.org writes:

>
https://codereview.chromium.org/2719223002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/tabs/tab_strip.cc (right):
>
>
https://codereview.chromium.org/2719223002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/tabs/tab_strip.cc:1346: if (tab_count() <= 0 ||
> GetModelIndexOfTab(tab) < 0)
> Why the tab_count() <= 0? Should GetModelIndexOfTab(tab) cover what you
> want?
>

Ok, done.
> https://codereview.chromium.org/2719223002/


-- 

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2017-02-28 22:46:29]

On Tue, Feb 28, 2017 at 2:11 PM, <dtseng@chromium.org> wrote:

>
> I'm willing to acknowledge the confusing naming of
> |pos_in_set|. If we assume that's going to be a common issue for lots of
> people, then a simple rename of the ax enum won't suffice. It is equally
> confusing in Blink, extension (automation) api, and in ChromeVox.
>
> I don't think renaming everything is within the scope of this cl; if you
> really insist, then I'll follow up with a big rename cl. As a
> middle ground, I've added a validated add to AXNodeData and a new index
> in set attribute for the three instances where it's being used. They
> were all wrong and missed on the initial review, so it's a good point to
> make this clearer.
>
> PTAL.
>
>
> sky@chromium.org writes:
>
> > https://codereview.chromium.org/2719223002/diff/20001/
> chrome/browser/ui/views/tabs/tab_strip.cc
> > File chrome/browser/ui/views/tabs/tab_strip.cc (right):
> >
> > https://codereview.chromium.org/2719223002/diff/20001/
> chrome/browser/ui/views/tabs/tab_strip.cc#newcode1346
> > chrome/browser/ui/views/tabs/tab_strip.cc:1346: if (tab_count() <= 0 ||
> > GetModelIndexOfTab(tab) < 0)
> > Why the tab_count() <= 0? Should GetModelIndexOfTab(tab) cover what you
> > want?
> >
>
> Ok, done.
> > https://codereview.chromium.org/2719223002/
>
>
> --
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-03-01 00:31:18]

In my opinion adding another member makes this code more confusing. Ideally the
index should be 0 based as anything else is just confusing. If you guys feel
strongly that is wrong, then rename the member to reflect that.

I do agree that both of these are out of scope for this fix. I would rather you
fix the bug in this patch without any renames. Save the rename for a follow on
patch that cleans things up.

https://codereview.chromium.org/2719223002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/2719223002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1346: if (GetModelIndexOfTab(tab) < 0)
Assign to local variable to avoid multiple calls.