Make ToggleImageButton into an accessible toggle button.

Make ToggleImageButton into an accessible toggle button.

BUG=628965
TEST=on Chrome OS, with ChromeVox on, alt-shift-n, tab to do not distrub and toggle. Verify "Do not disturb" and "Do not disturb, pressed" are announced by ChromeVox.

Committed: https://crrev.com/83cc54ce489016614d04f9148a884d20e3a67c0b
Cr-Commit-Position: refs/heads/master@{#407509}

[David Tseng, 2016-07-19 18:37:56]

Description was changed from

==========
Make ImageToggleButton into an accessible toggle button.
==========

to

==========
Make ImageToggleButton into an accessible toggle button.

BUG=628965
TEST=on Chrome OS, with ChromeVox on, alt-shift-n, tab to do not distrub and
toggle. Verify "Do not disturb" and "Do not disturb, pressed" are announced by
ChromeVox.
==========

[David Tseng, 2016-07-19 18:37:56]

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

[David Tseng, 2016-07-19 18:41:11]

Description was changed from

==========
Make ImageToggleButton into an accessible toggle button.

BUG=628965
TEST=on Chrome OS, with ChromeVox on, alt-shift-n, tab to do not distrub and
toggle. Verify "Do not disturb" and "Do not disturb, pressed" are announced by
ChromeVox.
==========

to

==========
Make ToggleImageButton into an accessible toggle button.

BUG=628965
TEST=on Chrome OS, with ChromeVox on, alt-shift-n, tab to do not distrub and
toggle. Verify "Do not disturb" and "Do not disturb, pressed" are announced by
ChromeVox.
==========

[sadrul, 2016-07-19 23:52:33]

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

[sadrul, 2016-07-19 23:52:34]

+dmazzoni@ for review.

Is this testable?

[David Tseng, 2016-07-20 19:47:10]

On 2016/07/19 23:52:34, sadrul wrote:
> +dmazzoni@ for review.
> 
> Is this testable?
Added test.

[dmazzoni, 2016-07-21 17:05:57]

Would it be possible to fix various subclasses?

In cases where it really is a toggle button it seems weird to
not have the right role when un-toggled.

https://codereview.chromium.org/2162083004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc
(right):

https://codereview.chromium.org/2162083004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:681: do {
This looks incomplete?

[David Tseng, 2016-07-21 21:22:50]

On Thu, Jul 21, 2016 at 10:05 AM, <dmazzoni@chromium.org> wrote:

> Would it be possible to fix various subclasses?
>
> Not really. See below.




> In cases where it really is a toggle button it seems weird to
> not have the right role when un-toggled.
>
> The subclasses (mostly Notification CenterButton) mix up the notion of the
toggle button because it provides different images for different button
states.

The fact that the ToggleImageButton is "toggleable" is visually centric it
seems so I went ahead and check for the caller that uses setToggledImage
which sets either images_ or alternate_images_ with a ImageSkia.




>
> https://codereview.chromium.org/2162083004/diff/20001/chrome
> /browser/chromeos/accessibility/spoken_feedback_browsertest.cc
> File
> chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc
> (right):
>
> https://codereview.chromium.org/2162083004/diff/20001/chrome
> /browser/chromeos/accessibility/spoken_feedback_browsertest.cc#newcode681
> chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:681:
> do {
> This looks incomplete?
> Oops. Yes, should now be in the latest patchset.
>


> https://codereview.chromium.org/2162083004/
>
> --
> 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.
>

-- 
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.

[dmazzoni, 2016-07-22 18:06:29]

lgtm

Please run git cl format on the test, but otherwise lg

https://codereview.chromium.org/2162083004/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc
(right):

https://codereview.chromium.org/2162083004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:230:
IN_PROC_BROWSER_TEST_F(LoggedInSpokenFeedbackTest, NavigateNotificationCenter) {
wrap

https://codereview.chromium.org/2162083004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:233:
EXPECT_TRUE(PerformAcceleratorAction(ash::SHOW_MESSAGE_CENTER_BUBBLE));
fix indentation

https://codereview.chromium.org/2162083004/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/image_button.cc (right):

https://codereview.chromium.org/2162083004/diff/60001/ui/views/controls/butto...
ui/views/controls/button/image_button.cc:274: // Use the visual pressed image as
a cue for making this control into an
wrap? looks like more than 80 chars in Rietveld

[David Tseng, 2016-07-22 22:49:26]

Re: the formatting issues. I checked manually and also re-ran git cl format on
the offending files, and I could see nothing wrong.

https://codereview.chromium.org/2162083004/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc
(right):

https://codereview.chromium.org/2162083004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:230:
IN_PROC_BROWSER_TEST_F(LoggedInSpokenFeedbackTest, NavigateNotificationCenter) {
On 2016/07/22 18:06:29, dmazzoni wrote:
> wrap

This is 80 characters.

https://codereview.chromium.org/2162083004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:233:
EXPECT_TRUE(PerformAcceleratorAction(ash::SHOW_MESSAGE_CENTER_BUBBLE));
On 2016/07/22 18:06:29, dmazzoni wrote:
> fix indentation

Something strange is going on...this is a two character indent like everything
else.

https://codereview.chromium.org/2162083004/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/image_button.cc (right):

https://codereview.chromium.org/2162083004/diff/60001/ui/views/controls/butto...
ui/views/controls/button/image_button.cc:274: // Use the visual pressed image as
a cue for making this control into an
On 2016/07/22 18:06:29, dmazzoni wrote:
> wrap? looks like more than 80 chars in Rietveld

Hmm...strange, it's 73 characters.

[sadrul, 2016-07-23 01:10:55]

lgtm

[dmazzoni, 2016-07-23 01:20:38]

Sorry about that, Rietveld has a new UI and it seems to be wrapping wrong

On Fri, Jul 22, 2016, 6:10 PM <sadrul@chromium.org> wrote:

> lgtm
>
>
>
> https://codereview.chromium.org/2162083004/
>
> --
> 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.
>

-- 
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, 2016-07-25 16:18:13]

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

[commit-bot: I haz the power, 2016-07-25 16:18:30]

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, 2016-07-25 17:32:17]

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

[commit-bot: I haz the power, 2016-07-25 17:32:18]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2016-07-25 17:46:00]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2016-07-25 17:46:21]

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, 2016-07-25 17:52:48]

Description was changed from

==========
Make ToggleImageButton into an accessible toggle button.

BUG=628965
TEST=on Chrome OS, with ChromeVox on, alt-shift-n, tab to do not distrub and
toggle. Verify "Do not disturb" and "Do not disturb, pressed" are announced by
ChromeVox.
==========

to

==========
Make ToggleImageButton into an accessible toggle button.

BUG=628965
TEST=on Chrome OS, with ChromeVox on, alt-shift-n, tab to do not distrub and
toggle. Verify "Do not disturb" and "Do not disturb, pressed" are announced by
ChromeVox.
==========

[commit-bot: I haz the power, 2016-07-25 17:52:49]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-25 17:56:13]

Description was changed from

==========
Make ToggleImageButton into an accessible toggle button.

BUG=628965
TEST=on Chrome OS, with ChromeVox on, alt-shift-n, tab to do not distrub and
toggle. Verify "Do not disturb" and "Do not disturb, pressed" are announced by
ChromeVox.
==========

to

==========
Make ToggleImageButton into an accessible toggle button.

BUG=628965
TEST=on Chrome OS, with ChromeVox on, alt-shift-n, tab to do not distrub and
toggle. Verify "Do not disturb" and "Do not disturb, pressed" are announced by
ChromeVox.

Committed: https://crrev.com/83cc54ce489016614d04f9148a884d20e3a67c0b
Cr-Commit-Position: refs/heads/master@{#407509}
==========

[commit-bot: I haz the power, 2016-07-25 17:56:14]

Patchset 4 (id:??) landed as
https://crrev.com/83cc54ce489016614d04f9148a884d20e3a67c0b
Cr-Commit-Position: refs/heads/master@{#407509}