Issue 29584714: Issue 5874 - Sync strings with the agencies translations (Closed)
DescriptionIssue 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 #
MessagesTotal messages: 15
This is ready for the review. @Tamara can you please review ".json" files and approve the strings please.
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.htm... desktop-options.html:248: <button class="i18n_options_control_remove_title delete" data-action="remove-subscription"></button> 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.
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.htm... desktop-options.html:248: <button class="i18n_options_control_remove_title delete" data-action="remove-subscription"></button> 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. 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. https://codereview.adblockplus.org/29584714/diff/29586648/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29584714/diff/29586648/locale/en_US/deskto... locale/en_US/desktop-options.json:128: "message": "You've turned off ad blocking on these websites and, therefore, will see ads on them. <a>Learn more</a>" @Tamara I would like to make "Learn more" a separate string, so we don't need to translate them separately in the end of each string, seems like they are always separate from the content. Let me know if that's fine with you.
On 2017/10/23 14:11:08, saroyanm wrote: > https://codereview.adblockplus.org/29584714/diff/29586648/locale/en_US/deskto... > File locale/en_US/desktop-options.json (right): > > https://codereview.adblockplus.org/29584714/diff/29586648/locale/en_US/deskto... > locale/en_US/desktop-options.json:128: "message": "You've turned off ad blocking > on these websites and, therefore, will see ads on them. <a>Learn more</a>" > @Tamara I would like to make "Learn more" a separate string, so we don't need to > translate them separately in the end of each string, seems like they are always > separate from the content. > > Let me know if that's fine with you. Sounds good. I have the translations already for each language, so shouldn't be a problem at all.
https://codereview.adblockplus.org/29584714/diff/29586648/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29584714/diff/29586648/locale/en_US/deskto... locale/en_US/desktop-options.json:128: "message": "You've turned off ad blocking on these websites and, therefore, will see ads on them. <a>Learn more</a>" On 2017/10/23 14:11:08, saroyanm wrote: > @Tamara I would like to make "Learn more" a separate string, so we don't need to > translate them separately in the end of each string, seems like they are always > separate from the content. > > Let me know if that's fine with you. Done.
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 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
So far this is the only one that needed a comment. https://codereview.adblockplus.org/29584714/diff/29586658/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29584714/diff/29586658/locale/en_US/deskto... locale/en_US/desktop-options.json:59: "description": "Tooltip for 'Block social media icon tracking' option in General tab", Shouldn't this be "Block social media icons tracking"?
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.htm... > 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: > > 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. Forgot to also mention that when focused on the button, screen readers will only read "button" since there is no label. Even though the context for the button is given elsewhere, it currently isn't directly related to the button. So if a user moves directly to that button they wouldn't know what it if for.
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.htm... > 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: > > 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. Forgot to also mention that when focused on the button, screen readers will only read "button" since there is no label. Even though the context for the button is given elsewhere, it currently isn't directly related to the button. So if a user moves directly to that button they wouldn't know what it if for.
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 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. https://codereview.adblockplus.org/29584714/diff/29586658/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29584714/diff/29586658/locale/en_US/deskto... locale/en_US/desktop-options.json:59: "description": "Tooltip for 'Block social media icon tracking' option in General tab", On 2017/10/23 15:48:56, tamara wrote: > Shouldn't this be "Block social media icons tracking"? Right, it's the description, but not actual text, the correct text is specified here -> https://codereview.adblockplus.org/29584714/diff/29586658/locale/en_US/common... But I'll update the description as well in order to make that clear as well.
https://codereview.adblockplus.org/29584714/diff/29586658/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29584714/diff/29586658/locale/en_US/deskto... locale/en_US/desktop-options.json:59: "description": "Tooltip for 'Block social media icon tracking' option in General tab", On 2017/10/23 16:14:33, saroyanm wrote: > On 2017/10/23 15:48:56, tamara wrote: > > Shouldn't this be "Block social media icons tracking"? > > Right, it's the description, but not actual text, the correct text is specified > here -> > https://codereview.adblockplus.org/29584714/diff/29586658/locale/en_US/common... > > But I'll update the description as well in order to make that clear as well. Done.
https://codereview.adblockplus.org/29584714/diff/29586697/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29584714/diff/29586697/locale/en_US/deskto... locale/en_US/desktop-options.json:60: "message": "The social media buttons (or icons) on the websites that you visit allow social media networks to build a profile of you based on your browsing habits - even when you don't click on them. Hide these buttons to protect your profile." Note from Tamara: This should be "The social media icons on the websites you visit allow social media networks to build a profile of you based on your browsing habits - even when you don’t click on them. Hiding these icons can protect your profile"
https://codereview.adblockplus.org/29584714/diff/29586697/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29584714/diff/29586697/locale/en_US/deskto... locale/en_US/desktop-options.json:60: "message": "The social media buttons (or icons) on the websites that you visit allow social media networks to build a profile of you based on your browsing habits - even when you don't click on them. Hide these buttons to protect your profile." On 2017/10/23 16:25:44, saroyanm wrote: > Note from Tamara: This should be "The social media icons on the websites you > visit allow social media networks to build a profile of you based on your > browsing habits - even when you don’t click on them. Hiding these icons can > protect your profile" Done.
On 2017/10/23 16:31:11, saroyanm wrote: > https://codereview.adblockplus.org/29584714/diff/29586697/locale/en_US/deskto... > File locale/en_US/desktop-options.json (right): > > https://codereview.adblockplus.org/29584714/diff/29586697/locale/en_US/deskto... > locale/en_US/desktop-options.json:60: "message": "The social media buttons (or > icons) on the websites that you visit allow social media networks to build a > profile of you based on your browsing habits - even when you don't click on > them. Hide these buttons to protect your profile." > On 2017/10/23 16:25:44, saroyanm wrote: > > Note from Tamara: This should be "The social media icons on the websites you > > visit allow social media networks to build a profile of you based on your > > browsing habits - even when you don’t click on them. Hiding these icons can > > protect your profile" > > Done. LGTM
(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). 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
