[ios] Fix settings switch accessibility value and hint

[ios] Fix settings switch accessibility value and hint.

BUG=671559
Review-Url: https://codereview.chromium.org/2625423003
Cr-Commit-Position: refs/heads/master@{#443524}
Committed: https://chromium.googlesource.com/chromium/src/+/29ab9e2ce5544edc156a4d7450c172fb517cb5f3

[sczs, 2017-01-12 21:44:55]

Description was changed from

==========
[ios] Fix settings switch accessibility value and hint

BUG=671559
==========

to

==========
[ios] Fix settings switch accessibility value and hint.

BUG=671559
==========

[sczs, 2017-01-12 21:44:55]

sczs@chromium.org changed reviewers:
+ edchin@chromium.org, lpromero@chromium.org

[sczs, 2017-01-12 21:45:18]

PTAL
I think these are the 2 Items that contain switches. If you know of an item I
forgot please let me know.

[edchin, 2017-01-12 22:13:45]

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/app/strings/ios_...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_strings.grd:1201: Double tap to toggle setting
The guidelines suggest not to mention 'tap': "Don’t repeat the action type in
the hint. For example, do not create hints such as “Tap to play the song” or
“Tapping plays the song.”
https://developer.apple.com/reference/objectivec/nsobject/1615093-accessibili...

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/cells/sync_switch_item.mm (right):

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/sync_switch_item.mm:186: return
_switchView.accessibilityHint;
Should both the row and the switch have the same hint?

[sczs, 2017-01-12 22:26:18]

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/app/strings/ios_...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_strings.grd:1201: Double tap to toggle setting
On 2017/01/12 22:13:45, edchin wrote:
> The guidelines suggest not to mention 'tap': "Don’t repeat the action type in
> the hint. For example, do not create hints such as “Tap to play the song” or
> “Tapping plays the song.”
>
https://developer.apple.com/reference/objectivec/nsobject/1615093-accessibili...

I think the difference is that this is a switch, so its not something that you
would tap intuitively. On the other hand no action type is being repeated, Voice
Over will read: "Cell Text, ON/OFF.... Double tap to toggle setting" which is
the exact same hint that iOS switch settings use.

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/cells/sync_switch_item.mm (right):

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/sync_switch_item.mm:186: return
_switchView.accessibilityHint;
On 2017/01/12 22:13:45, edchin wrote:
> Should both the row and the switch have the same hint? 

Yes, we are aiming to imitate iOS settings switches. In this case only the row
is an accessible element, so only one hint is read.

[edchin, 2017-01-12 22:37:54]

lgtm. Also, I don't know of any other switches in settings. Thanks for fixing
this.

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/app/strings/ios_...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_strings.grd:1201: Double tap to toggle setting
On 2017/01/12 22:26:17, sczs wrote:
> On 2017/01/12 22:13:45, edchin wrote:
> > The guidelines suggest not to mention 'tap': "Don’t repeat the action type
in
> > the hint. For example, do not create hints such as “Tap to play the song” or
> > “Tapping plays the song.”
> >
>
https://developer.apple.com/reference/objectivec/nsobject/1615093-accessibili...
> 
> I think the difference is that this is a switch, so its not something that you
> would tap intuitively. On the other hand no action type is being repeated,
Voice
> Over will read: "Cell Text, ON/OFF.... Double tap to toggle setting" which is
> the exact same hint that iOS switch settings use. 

Acknowledged.

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/cells/sync_switch_item.mm (right):

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/sync_switch_item.mm:186: return
_switchView.accessibilityHint;
On 2017/01/12 22:26:17, sczs wrote:
> On 2017/01/12 22:13:45, edchin wrote:
> > Should both the row and the switch have the same hint? 
> 
> Yes, we are aiming to imitate iOS settings switches. In this case only the row
> is an accessible element, so only one hint is read.

Why not set the hint only on the row if it is the only accessible element?

[lpromero, 2017-01-13 10:01:33]

The CQ bit was checked by lpromero@chromium.org

[lpromero, 2017-01-13 10:01:34]

lgtm

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/app/strings/ios_...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_strings.grd:1201: Double tap to toggle setting
On 2017/01/12 22:37:54, edchin wrote:
> On 2017/01/12 22:26:17, sczs wrote:
> > On 2017/01/12 22:13:45, edchin wrote:
> > > The guidelines suggest not to mention 'tap': "Don’t repeat the action type
> in
> > > the hint. For example, do not create hints such as “Tap to play the song”
or
> > > “Tapping plays the song.”
> > >
> >
>
https://developer.apple.com/reference/objectivec/nsobject/1615093-accessibili...
> > 
> > I think the difference is that this is a switch, so its not something that
you
> > would tap intuitively. On the other hand no action type is being repeated,
> Voice
> > Over will read: "Cell Text, ON/OFF.... Double tap to toggle setting" which
is
> > the exact same hint that iOS switch settings use. 
> 
> Acknowledged.

No action needed: I wanted to follow Apple's example in the Settings app, where
they use "Double tap to toggle setting". Note that they don't add the Button
trait to the cell, so it might be less redundant.

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/browser/ui/colle...
File ios/chrome/browser/ui/collection_view/cells/collection_view_switch_item.mm
(left):

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/collection_view_switch_item.mm:34:
self.accessibilityTraits |= UIAccessibilityTraitButton;
No action needed: Agreed. Being as similar to iOS Settings app as possible is a
good thing.

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/cells/sync_switch_item.mm (right):

https://codereview.chromium.org/2625423003/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/sync_switch_item.mm:186: return
_switchView.accessibilityHint;
On 2017/01/12 22:37:54, edchin wrote:
> On 2017/01/12 22:26:17, sczs wrote:
> > On 2017/01/12 22:13:45, edchin wrote:
> > > Should both the row and the switch have the same hint? 
> > 
> > Yes, we are aiming to imitate iOS settings switches. In this case only the
row
> > is an accessible element, so only one hint is read.
> 
> Why not set the hint only on the row if it is the only accessible element?

No action needed: If in the future we need to have a secondary element within
the cell, the row will stop being an accessibility element and will list its
children as elements. Sergio's approach seems more maintainable this way, where
the switch holds the info no matter what, and the row makes the connection,
based on the behavior it decides it wants.

[commit-bot: I haz the power, 2017-01-13 10:02:00]

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-01-13 10:11:26]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1484301693289410, "parent_rev":
"90c70c7ac3a6f823ec083829fad97016ef4d24b6", "commit_rev":
"29ab9e2ce5544edc156a4d7450c172fb517cb5f3"}

[commit-bot: I haz the power, 2017-01-13 10:12:00]

Description was changed from

==========
[ios] Fix settings switch accessibility value and hint.

BUG=671559
==========

to

==========
[ios] Fix settings switch accessibility value and hint.

BUG=671559

Review-Url: https://codereview.chromium.org/2625423003
Cr-Commit-Position: refs/heads/master@{#443524}
Committed:
https://chromium.googlesource.com/chromium/src/+/29ab9e2ce5544edc156a4d7450c1...
==========

[commit-bot: I haz the power, 2017-01-13 10:12:01]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/29ab9e2ce5544edc156a4d7450c1...