Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(30)

Issue 29584714: Issue 5874 - Sync strings with the agencies translations (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 7 months ago by saroyanm
Modified:
2 years, 7 months ago
Reviewers:
ire, tamara
Visibility:
Public.

Description

Issue 5874 - Sync strings with the agencies translations

Patch Set 1 : #

Total comments: 5

Patch Set 2 : Rebased #

Patch Set 3 : Updated strings to match translations from the agencies #

Total comments: 2

Patch Set 4 : Separate Learn more from strings #

Total comments: 3

Patch Set 5 : Fixed the description #

Total comments: 2

Patch Set 6 : Updated Fanboy's list tooltip description #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -59 lines) Patch
M desktop-options.html View 1 2 3 3 chunks +9 lines, -3 lines 0 comments Download
M desktop-options.js View 1 2 3 4 chunks +9 lines, -1 line 0 comments Download
M locale/en_US/common.json View 1 chunk +2 lines, -2 lines 0 comments Download
M locale/en_US/desktop-options.json View 1 2 3 4 5 16 chunks +39 lines, -53 lines 0 comments Download

Messages

Total messages: 15
saroyanm
This is ready for the review. @Tamara can you please review ".json" files and approve ...
2 years, 7 months ago (2017-10-20 21:15:43 UTC) #1
ire
Just one question below https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html#newcode248 desktop-options.html:248: <button class="i18n_options_control_remove_title delete" data-action="remove-subscription"></button> Should ...
2 years, 7 months ago (2017-10-23 12:21:39 UTC) #2
saroyanm
New patch uploaded. https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html#newcode248 desktop-options.html:248: <button class="i18n_options_control_remove_title delete" data-action="remove-subscription"></button> On 2017/10/23 ...
2 years, 7 months ago (2017-10-23 14:11:08 UTC) #3
tamara
On 2017/10/23 14:11:08, saroyanm wrote: > https://codereview.adblockplus.org/29584714/diff/29586648/locale/en_US/desktop-options.json > File locale/en_US/desktop-options.json (right): > > https://codereview.adblockplus.org/29584714/diff/29586648/locale/en_US/desktop-options.json#newcode128 > ...
2 years, 7 months ago (2017-10-23 14:18:49 UTC) #4
saroyanm
https://codereview.adblockplus.org/29584714/diff/29586648/locale/en_US/desktop-options.json File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29584714/diff/29586648/locale/en_US/desktop-options.json#newcode128 locale/en_US/desktop-options.json:128: "message": "You've turned off ad blocking on these websites ...
2 years, 7 months ago (2017-10-23 14:39:38 UTC) #5
ire
https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html#newcode248 desktop-options.html:248: <button class="i18n_options_control_remove_title delete" data-action="remove-subscription"></button> On 2017/10/23 14:11:08, saroyanm wrote: ...
2 years, 7 months ago (2017-10-23 15:45:35 UTC) #6
tamara
So far this is the only one that needed a comment. https://codereview.adblockplus.org/29584714/diff/29586658/locale/en_US/desktop-options.json File locale/en_US/desktop-options.json (right): ...
2 years, 7 months ago (2017-10-23 15:48:57 UTC) #7
ire
On 2017/10/23 15:45:35, ire wrote: > https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html > File desktop-options.html (right): > > https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html#newcode248 > ...
2 years, 7 months ago (2017-10-23 15:49:53 UTC) #8
ire
On 2017/10/23 15:45:35, ire wrote: > https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html > File desktop-options.html (right): > > https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html#newcode248 > ...
2 years, 7 months ago (2017-10-23 15:49:54 UTC) #9
saroyanm
https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html#newcode248 desktop-options.html:248: <button class="i18n_options_control_remove_title delete" data-action="remove-subscription"></button> On 2017/10/23 15:45:34, ire wrote: ...
2 years, 7 months ago (2017-10-23 16:14:33 UTC) #10
saroyanm
https://codereview.adblockplus.org/29584714/diff/29586658/locale/en_US/desktop-options.json File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29584714/diff/29586658/locale/en_US/desktop-options.json#newcode59 locale/en_US/desktop-options.json:59: "description": "Tooltip for 'Block social media icon tracking' option ...
2 years, 7 months ago (2017-10-23 16:16:45 UTC) #11
saroyanm
https://codereview.adblockplus.org/29584714/diff/29586697/locale/en_US/desktop-options.json File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29584714/diff/29586697/locale/en_US/desktop-options.json#newcode60 locale/en_US/desktop-options.json:60: "message": "The social media buttons (or icons) on the ...
2 years, 7 months ago (2017-10-23 16:25:45 UTC) #12
saroyanm
https://codereview.adblockplus.org/29584714/diff/29586697/locale/en_US/desktop-options.json File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29584714/diff/29586697/locale/en_US/desktop-options.json#newcode60 locale/en_US/desktop-options.json:60: "message": "The social media buttons (or icons) on the ...
2 years, 7 months ago (2017-10-23 16:31:11 UTC) #13
tamara
On 2017/10/23 16:31:11, saroyanm wrote: > https://codereview.adblockplus.org/29584714/diff/29586697/locale/en_US/desktop-options.json > File locale/en_US/desktop-options.json (right): > > https://codereview.adblockplus.org/29584714/diff/29586697/locale/en_US/desktop-options.json#newcode60 > ...
2 years, 7 months ago (2017-10-23 16:42:31 UTC) #14
ire
2 years, 7 months ago (2017-10-24 07:36:08 UTC) #15
(The discussion about labels is not necessarily relevant to this issue, so for
this issue LGTM)

https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.html
File desktop-options.html (right):

https://codereview.adblockplus.org/29584714/diff/29584720/desktop-options.htm...
desktop-options.html:248: <button class="i18n_options_control_remove_title
delete" data-action="remove-subscription"></button>
On 2017/10/23 16:14:32, saroyanm wrote:
> On 2017/10/23 15:45:34, ire wrote:
> > On 2017/10/23 14:11:08, saroyanm wrote:
> > > On 2017/10/23 12:21:39, ire wrote:
> > > > Should this also have the "title" attribute?
> > > > 
> > > > Also (I assume you will handle this in another issue, but I'm not
> currently
> > > > aware if you have created an issue for this yet), most of these buttons
> > don't
> > > > have an accessible label.
> > > 
> > > Not sure if it's required, we clearly communicate what the button suppose
to
> > do
> > > in the text which is "remove".
> > > So when user with screen reader focus the item the person will hear
> something
> > > along the lines 'Menu item with 4 items selected "remove"' providing
titles
> > will
> > > require translating them as well, I do agree that we need to communicate
on
> > > icons, but I don't think we need to make explicit menu items having title
> > > attribute.
> > 
> > An accessible label is required, even if it does have the title attribute
> > (that's a different kind of label). You can provide it via an an aria-label
if
> > you don't want the textcontent in the actual button, but it's definitely a
> > (really important) requirement. 
> > 
> > > We do use "title" already for the "trach" icons, but the problem is that
> they
> > > are not yet translated AFAIK, which still needs to be fixed.
> > 
> > Ack
> > 
> 
> Sorry I think I miss communicated here. 
> I mean it's already accessible and screen readers recognize the button and the
> text inside (according to my tests).
> I think aria-label needs to be used when the text is not available
>
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniqu...
> 
> So no need to use additional accessible Technics when using
> <button>Remove</button> IMO.
> 
> Regarding usage of title vs aria-label on other elements that are lacking
text,
> I think we might need to test the browsers/user-agents that don't make title
> accessible yet, during the implementation I think I tested with Chrome and
> screen readers were recognizing the title fine, but didn't tested with other
> browsers though.
> 
> It feels more like a fault of the browsers rather than "title" attribute being
> non accessible, I think this is something that might change in future, but let
> me know if I'm wrong and there is another reason of using aria-label in
addition
> to title instead.

No worries, I think I misunderstood what you were saying :)

You're absolutely correct that an aria-label is not needed if the text is
actually within the button (<button>Remove</button>)

What I was saying was that, for some buttons on this page, there is no text
content within the actual button. Instead, there is *only* a title attribute.

The accessible name for a control is not supposed to be the title attribute.
That attribute is for something totally different. The accessible name of a
control element like a button has to either come from a <label>, an
`aria-label`, or the actual text content of the element (see
https://developer.paciellogroup.com/blog/2017/04/what-is-an-accessible-name/)

So the fact that browsers don't use the title attribute is not actually a fault
of them. They aren't supposed to use it as the accessible name for the control.

If you run an accessibility test on the page (you can use Lighthouse under the
Audits tab in Chrome), you'll see that the controls raise an error because the
label is missing (even for those with a title attribute).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5