Adding accessibility to the close button in sign-in promo

Adding accessibility to the close button in sign-in promo

Adding the close button as custom action of the sign-in promo element.

Context: https://drive.google.com/open?id=0ByXziH_JVCGJbTVZajl0QnVabVU

BUG=661794
Review-Url: https://codereview.chromium.org/2901783002
Cr-Commit-Position: refs/heads/master@{#474263}
Committed: https://chromium.googlesource.com/chromium/src/+/d5cf07ad07b7cbe1784df0097ee090507703395f

[jlebel, 2017-05-23 11:54:46]

Patchset #1 (id:1) has been deleted

[jlebel, 2017-05-23 11:56:02]

jlebel@chromium.org changed reviewers:
+ lpromero@chromium.org, msarda@chromium.org

[jlebel, 2017-05-23 11:56:03]

Hello Louis and Mihai,

Can you review this patch about accessibility?

Thanks,

[jlebel, 2017-05-23 12:04:40]

Description was changed from

==========
Adding accessibility to the close button in sign-in promo

Adding the close button as custom action of the sign-in promo element.

BUG=661794
==========

to

==========
Adding accessibility to the close button in sign-in promo

Adding the close button as custom action of the sign-in promo element.

Context: https://drive.google.com/open?id=0ByXziH_JVCGJbTVZajl0QnVabVU

BUG=661794
==========

[lpromero, 2017-05-23 12:06:17]

lgtm with clean up.

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/app/strings/...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/app/strings/...
ios/chrome/app/strings/ios_strings.grd:1516: <message
name="IDS_IOS_SIGNIN_PROMO_CLOSE_ACCESSIBILITY" desc="Accessibility label to
describ the close button in the sign-in promo element">
*describe

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/app/strings/...
ios/chrome/app/strings/ios_strings.grd:1516: <message
name="IDS_IOS_SIGNIN_PROMO_CLOSE_ACCESSIBILITY" desc="Accessibility label to
describ the close button in the sign-in promo element">
Nit: add a period at the end of the description sentence.

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right):

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/authentication/signin_promo_view.mm:105: //   
_closeButton.hidden = YES;
Still needed? Otherwise remove.

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right):

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:7: #import
"MDCFlatButton.h"
This works? Weird.
#import "ios/third_party/.../MaterialButtons.h"

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:46: //   
[_signinPromoView addSubview:_closeButton];
Please clean up.

[jlebel, 2017-05-23 12:43:11]

Patchset #2 (id:40001) has been deleted

[jlebel, 2017-05-23 12:43:26]

Sorry, I pulled the trigger too quickly... I didn't review my patch before
sending it.

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/app/strings/...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/app/strings/...
ios/chrome/app/strings/ios_strings.grd:1516: <message
name="IDS_IOS_SIGNIN_PROMO_CLOSE_ACCESSIBILITY" desc="Accessibility label to
describ the close button in the sign-in promo element">
On 2017/05/23 12:06:16, lpromero wrote:
> *describe

Done.

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right):

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/authentication/signin_promo_view.mm:105: //   
_closeButton.hidden = YES;
On 2017/05/23 12:06:16, lpromero wrote:
> Still needed? Otherwise remove.

Done.

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right):

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:7: #import
"MDCFlatButton.h"
On 2017/05/23 12:06:16, lpromero wrote:
> This works? Weird.
> #import "ios/third_party/.../MaterialButtons.h"

Done.

https://codereview.chromium.org/2901783002/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:46: //   
[_signinPromoView addSubview:_closeButton];
On 2017/05/23 12:06:16, lpromero wrote:
> Please clean up. 

Done.

[msarda, 2017-05-24 08:47:58]

LGTM with some suggestions.

https://codereview.chromium.org/2901783002/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right):

https://codereview.chromium.org/2901783002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:41:
[_signinPromoView.closeButton addTarget:self
I think we should also make the close button visible here:
_signinPromoView.closeButton.hidden = NO;

https://codereview.chromium.org/2901783002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:64: -
(void)prepareForReuse {
Just call [self setCloseButtonAction:nil] here.

https://codereview.chromium.org/2901783002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:66:
_signinPromoView.closeButton.hidden = NO;
This should probably be YES (in the header it says that the close button is
hidden when the closeButtonAction is nil).

However I think that for bookmarks the close button should always be visible,
right? So, I would just move _signinPromoView.closeButton.hidden = NO to the
initializer.

https://codereview.chromium.org/2901783002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:72:
_signinPromoView.closeButton.hidden = _closeButtonAction == nil;
I would remove this setter and  _signinPromoView.closeButton.hidden = NO to the
initializer.

[jlebel, 2017-05-24 11:20:17]

https://codereview.chromium.org/2901783002/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right):

https://codereview.chromium.org/2901783002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:41:
[_signinPromoView.closeButton addTarget:self
On 2017/05/24 08:47:58, msarda wrote:
> I think we should also make the close button visible here:
> _signinPromoView.closeButton.hidden = NO;

Done.

https://codereview.chromium.org/2901783002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:64: -
(void)prepareForReuse {
On 2017/05/24 08:47:58, msarda wrote:
> Just call [self setCloseButtonAction:nil] here.

I guess there is no point for that, |_closeButtonAction = nil| is fine.

https://codereview.chromium.org/2901783002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:66:
_signinPromoView.closeButton.hidden = NO;
On 2017/05/24 08:47:58, msarda wrote:
> This should probably be YES (in the header it says that the close button is
> hidden when the closeButtonAction is nil).
> 
> However I think that for bookmarks the close button should always be visible,
> right? So, I would just move _signinPromoView.closeButton.hidden = NO to the
> initializer.

Done.

https://codereview.chromium.org/2901783002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:72:
_signinPromoView.closeButton.hidden = _closeButtonAction == nil;
On 2017/05/24 08:47:58, msarda wrote:
> I would remove this setter and  _signinPromoView.closeButton.hidden = NO to
the
> initializer.

Done.

[jlebel, 2017-05-24 11:30:53]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2017-05-24 11:30:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
lpromero@chromium.org, msarda@chromium.org
Link to the patchset: https://codereview.chromium.org/2901783002/#ps80001
(title: ".")

[commit-bot: I haz the power, 2017-05-24 11:31:16]

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-05-24 12:42:52]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495625453952750,
"parent_rev": "f7ba3ebc82c44224baf56adda18795a0578a47c9", "commit_rev":
"d5cf07ad07b7cbe1784df0097ee090507703395f"}

[commit-bot: I haz the power, 2017-05-24 12:43:04]

Description was changed from

==========
Adding accessibility to the close button in sign-in promo

Adding the close button as custom action of the sign-in promo element.

Context: https://drive.google.com/open?id=0ByXziH_JVCGJbTVZajl0QnVabVU

BUG=661794
==========

to

==========
Adding accessibility to the close button in sign-in promo

Adding the close button as custom action of the sign-in promo element.

Context: https://drive.google.com/open?id=0ByXziH_JVCGJbTVZajl0QnVabVU

BUG=661794

Review-Url: https://codereview.chromium.org/2901783002
Cr-Commit-Position: refs/heads/master@{#474263}
Committed:
https://chromium.googlesource.com/chromium/src/+/d5cf07ad07b7cbe1784df0097ee0...
==========

[commit-bot: I haz the power, 2017-05-24 12:43:12]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/d5cf07ad07b7cbe1784df0097ee0...