Issue 29519650: Issue 5540 - implement notification (Closed)
Patch Set 1 : #
Total comments: 45
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Patch Set 5 : Rebased #
MessagesTotal messages: 14
I didn't specify a specific reviewer yet, will do as soon I'll know how occupied are you guys, meanwhile feel free to jump in if you will done with other reviews and will have time for this one next week. https://codereview.adblockplus.org/29519650/diff/29519663/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29519650/diff/29519663/skin/new-options.cs... skin/new-options.css:1121: Notification There is an issue open, I'll followup -> https://bitbucket.org/adblockplus/spec/issues/61 https://codereview.adblockplus.org/29519650/diff/29519663/skin/new-options.cs... skin/new-options.css:1121: Notification I didn't implement the text styles, as that might be applied globally. Also will, so either I'll revisit it later, or if it will be obvious that it's separate will update here.
This is ready for the review, @Ire can you please have a look, but prioritize https://codereview.adblockplus.org/29519636/ first. Thanks in advance
Thanks Manvel! Here are my first comments. https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-op... locale/en-US/new-options.json:419: "description": "Notification that is being shown after a new website is whitelisted", NIT: "Notification that is shown after a new website is whitelisted" is more correct (remove "being") https://codereview.adblockplus.org/29519650/diff/29525623/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29525623/new-options.html#ne... new-options.html:380: <button data-action="hide-notification" id="hide-notification"></button> It's part of our coding style to not omit optional tags, so there should be a `type="button"` on this https://codereview.adblockplus.org/29519650/diff/29525623/new-options.html#ne... new-options.html:380: <button data-action="hide-notification" id="hide-notification"></button> This button doesn't have an accessible name https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js#newc... new-options.js:1006: E("notification").setAttribute("aria-hidden", false); The aria-live attribute could also be relevant here (see https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Live_Reg...) https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js#newc... new-options.js:1008: setTimeout(hideNotification, 3000); (Personal Opinion) I think 3 seconds is too short https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js#newc... new-options.js:1013: E("notification").setAttribute("aria-hidden", true); NIT: Clear the textContent as well? https://codereview.adblockplus.org/29519650/diff/29525623/skin/delete.svg File skin/delete.svg (right): https://codereview.adblockplus.org/29519650/diff/29525623/skin/delete.svg#new... skin/delete.svg:1: <svg id="5dbfc7a9-72c4-4898-881d-75bf73a9a059" data-name="artwork" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"><title>delete</title><polygon points="42.71 6.71 41.29 5.29 24 22.59 6.71 5.29 5.29 6.71 22.59 24 5.29 41.29 6.71 42.71 24 25.41 41.29 42.71 42.71 41.29 25.41 24 42.71 6.71"/></svg> For some reason the weight/thickness of the icon seems much lighter than in the spec. This makes it quite difficult to see. https://codereview.adblockplus.org/29519650/diff/29525623/skin/delete.svg#new... skin/delete.svg:1: <svg id="5dbfc7a9-72c4-4898-881d-75bf73a9a059" data-name="artwork" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"><title>delete</title><polygon points="42.71 6.71 41.29 5.29 24 22.59 6.71 5.29 5.29 6.71 22.59 24 5.29 41.29 6.71 42.71 24 25.41 41.29 42.71 42.71 41.29 25.41 24 42.71 6.71"/></svg> The ID is invalid (starts with a number) https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1088: position: absolute; I think this should be `position: fixed`. Although unlikely, the user might be slightly scrolled down on the page (e.g. if they have a long list of whitelisted websites) and will not be able to see the notification. https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1094: box-sizing: border-box; Not sure if this will be addressed by implementing issue 5543, but this box-sizing style seems like it should be applied to all elements https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1097: font-weight: 600; Can this bold style be applied by using a <strong> element instead? https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1099: background-color: #E1F2FA; (I think this may have been discussed already but) this color to background-color contrast ratio is just 2.7 https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1111: padding: 0rem;e There's a stray "e" here :p https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1124: cursor: pointer; I think the `cursor: pointer` should be on the button itself, not the image only. If you hover the mouse around the area of the image, which is still the button, the cursor changes even though it's still a clickable area. https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1131: background-color: #5CBCE1; (I think this was also discussed before but) the change in colour here is too insignificant, it's unnoticeable. Plus this colour's contrast ratio is too low.
Awesome, I think I also have everything here for the next patch. https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-op... locale/en-US/new-options.json:419: "description": "Notification that is being shown after a new website is whitelisted", On 2017/08/24 10:08:43, ire wrote: > NIT: "Notification that is shown after a new website is whitelisted" is more > correct (remove "being") agree https://codereview.adblockplus.org/29519650/diff/29525623/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29525623/new-options.html#ne... new-options.html:380: <button data-action="hide-notification" id="hide-notification"></button> On 2017/08/24 10:08:43, ire wrote: > It's part of our coding style to not omit optional tags, so there should be a > `type="button"` on this I think we were overriding current -> https://google.github.io/styleguide/htmlcssguide.html#Optional_Tags Google style, which was about the HTML tags, but not attributes. In general I agree that buttons here should specify types, currently all of them are missing type attribute, so I think it's better to be consistent in that case, things probably will change as soon we will start implementing -> https://issues.adblockplus.org/ticket/5251 so probably that will make sense to cover that there as well. https://codereview.adblockplus.org/29519650/diff/29525623/new-options.html#ne... new-options.html:380: <button data-action="hide-notification" id="hide-notification"></button> On 2017/08/24 10:08:43, ire wrote: > This button doesn't have an accessible name Good point, I'll use aria-label="Close" https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js#newc... new-options.js:1006: E("notification").setAttribute("aria-hidden", false); On 2017/08/24 10:08:44, ire wrote: > The aria-live attribute could also be relevant here (see > https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Live_Reg...) Agree https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js#newc... new-options.js:1008: setTimeout(hideNotification, 3000); On 2017/08/24 10:08:43, ire wrote: > (Personal Opinion) I think 3 seconds is too short That's specified in the specs, let's be consistent for now with specs to be able to finish the page until it's not critical, but I agree that this is something that make sense to bring to the discussion during the next meeting. https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js#newc... new-options.js:1013: E("notification").setAttribute("aria-hidden", true); On 2017/08/24 10:08:44, ire wrote: > NIT: Clear the textContent as well? Yes, I'll update that. https://codereview.adblockplus.org/29519650/diff/29525623/skin/delete.svg File skin/delete.svg (right): https://codereview.adblockplus.org/29519650/diff/29525623/skin/delete.svg#new... skin/delete.svg:1: <svg id="5dbfc7a9-72c4-4898-881d-75bf73a9a059" data-name="artwork" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"><title>delete</title><polygon points="42.71 6.71 41.29 5.29 24 22.59 6.71 5.29 5.29 6.71 22.59 24 5.29 41.29 6.71 42.71 24 25.41 41.29 42.71 42.71 41.29 25.41 24 42.71 6.71"/></svg> On 2017/08/24 10:08:44, ire wrote: > The ID is invalid (starts with a number) I think the ID is redundant, I think this image wasn't optimised. https://codereview.adblockplus.org/29519650/diff/29525623/skin/delete.svg#new... skin/delete.svg:1: <svg id="5dbfc7a9-72c4-4898-881d-75bf73a9a059" data-name="artwork" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"><title>delete</title><polygon points="42.71 6.71 41.29 5.29 24 22.59 6.71 5.29 5.29 6.71 22.59 24 5.29 41.29 6.71 42.71 24 25.41 41.29 42.71 42.71 41.29 25.41 24 42.71 6.71"/></svg> On 2017/08/24 10:08:44, ire wrote: > For some reason the weight/thickness of the icon seems much lighter than in the > spec. This makes it quite difficult to see. I've created a bitbucket issue for that -> https://bitbucket.org/adblockplus/spec/issues/68 This shouldn't be blocker for this review though, we will fix it later as soon new SVG is provided. https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1088: position: absolute; On 2017/08/24 10:08:44, ire wrote: > I think this should be `position: fixed`. > > Although unlikely, the user might be slightly scrolled down on the page (e.g. if > they have a long list of whitelisted websites) and will not be able to see the > notification. Good point! https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1094: box-sizing: border-box; On 2017/08/24 10:08:44, ire wrote: > Not sure if this will be addressed by implementing issue 5543, but this > box-sizing style seems like it should be applied to all elements Not sure if we should apply this to all elements, I know that the practice exists. If it appears that we need this on all elements will apply globally in separate review. But the reason why I did it so is because the scrollbar that was appearing when I was using padding as this is 100% width absolute item. https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1097: font-weight: 600; On 2017/08/24 10:08:44, ire wrote: > Can this bold style be applied by using a <strong> element instead? Yes, I'll use <strong> instead https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1099: background-color: #E1F2FA; On 2017/08/24 10:08:44, ire wrote: > (I think this may have been discussed already but) this color to > background-color contrast ratio is just 2.7 I agree, that the contrast needs to be adjusted. but that shouldn't be blocker for the review. So either I can bring that up during the next meeting, as I collecting similar questions. So we will have something ready to show as well. Or maybe you can create a bitbucket issue explaining the contrast. https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1111: padding: 0rem;e On 2017/08/24 10:08:45, ire wrote: > There's a stray "e" here :p :D https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1124: cursor: pointer; On 2017/08/24 10:08:44, ire wrote: > I think the `cursor: pointer` should be on the button itself, not the image > only. If you hover the mouse around the area of the image, which is still the > button, the cursor changes even though it's still a clickable area. Right. I'll change that. https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1131: background-color: #5CBCE1; On 2017/08/24 10:08:45, ire wrote: > (I think this was also discussed before but) the change in colour here is too > insignificant, it's unnoticeable. Plus this colour's contrast ratio is too low. Yes, I agree, same as above question. I'll collect all similar cases. Adjusting colors is easy and probably question of minutes, but discussions and agreements usually takes time. So we probably will have separate issue addressing all contrast issues, but currently let's be consistent with style guide.
https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-op... locale/en-US/new-options.json:168: "options_whitelist_description": { This ID is duplicated. as noticed here -> https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op...
Thanks for the review Ire. I've uploaded the new patch. https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-op... locale/en-US/new-options.json:168: "options_whitelist_description": { On 2017/08/24 17:53:59, saroyanm wrote: > This ID is duplicated. > as noticed here -> > https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... Done. https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-op... locale/en-US/new-options.json:419: "description": "Notification that is being shown after a new website is whitelisted", On 2017/08/24 14:18:25, saroyanm wrote: > On 2017/08/24 10:08:43, ire wrote: > > NIT: "Notification that is shown after a new website is whitelisted" is more > > correct (remove "being") > > agree Done. https://codereview.adblockplus.org/29519650/diff/29525623/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29525623/new-options.html#ne... new-options.html:380: <button data-action="hide-notification" id="hide-notification"></button> On 2017/08/24 14:18:25, saroyanm wrote: > On 2017/08/24 10:08:43, ire wrote: > > This button doesn't have an accessible name > > Good point, I'll use aria-label="Close" Done. https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js#newc... new-options.js:1006: E("notification").setAttribute("aria-hidden", false); On 2017/08/24 14:18:25, saroyanm wrote: > On 2017/08/24 10:08:44, ire wrote: > > The aria-live attribute could also be relevant here (see > > > https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Live_Reg...) > > Agree Done. https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js#newc... new-options.js:1013: E("notification").setAttribute("aria-hidden", true); On 2017/08/24 14:18:26, saroyanm wrote: > On 2017/08/24 10:08:44, ire wrote: > > NIT: Clear the textContent as well? > > Yes, I'll update that. Done. https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1088: position: absolute; On 2017/08/24 14:18:26, saroyanm wrote: > On 2017/08/24 10:08:44, ire wrote: > > I think this should be `position: fixed`. > > > > Although unlikely, the user might be slightly scrolled down on the page (e.g. > if > > they have a long list of whitelisted websites) and will not be able to see the > > notification. > > Good point! Done. https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1097: font-weight: 600; On 2017/08/24 14:18:26, saroyanm wrote: > On 2017/08/24 10:08:44, ire wrote: > > Can this bold style be applied by using a <strong> element instead? > > Yes, I'll use <strong> instead Done. https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1111: padding: 0rem;e On 2017/08/24 14:18:26, saroyanm wrote: > On 2017/08/24 10:08:45, ire wrote: > > There's a stray "e" here :p > > :D Done. https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1124: cursor: pointer; On 2017/08/24 14:18:26, saroyanm wrote: > On 2017/08/24 10:08:44, ire wrote: > > I think the `cursor: pointer` should be on the button itself, not the image > > only. If you hover the mouse around the area of the image, which is still the > > button, the cursor changes even though it's still a clickable area. > > Right. I'll change that. Done.
Thanks Manvel. Notes: https://codereview.adblockplus.org/29519650/diff/29525623/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29525623/new-options.html#ne... new-options.html:380: <button data-action="hide-notification" id="hide-notification"></button> On 2017/08/24 14:18:25, saroyanm wrote: > On 2017/08/24 10:08:43, ire wrote: > > It's part of our coding style to not omit optional tags, so there should be a > > `type="button"` on this > > I think we were overriding current -> > https://google.github.io/styleguide/htmlcssguide.html#Optional_Tags Google > style, which was about the HTML tags, but not attributes. > > In general I agree that buttons here should specify types, currently all of them > are missing type attribute, so I think it's better to be consistent in that > case, things probably will change as soon we will start implementing -> > https://issues.adblockplus.org/ticket/5251 so probably that will make sense to > cover that there as well. Okay, that makes sense. https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519650/diff/29525623/new-options.js#newc... new-options.js:1008: setTimeout(hideNotification, 3000); On 2017/08/24 14:18:26, saroyanm wrote: > On 2017/08/24 10:08:43, ire wrote: > > (Personal Opinion) I think 3 seconds is too short > > That's specified in the specs, let's be consistent for now with specs to be able > to finish the page until it's not critical, but I agree that this is something > that make sense to bring to the discussion during the next meeting. Acknowledged. https://codereview.adblockplus.org/29519650/diff/29525623/skin/delete.svg File skin/delete.svg (right): https://codereview.adblockplus.org/29519650/diff/29525623/skin/delete.svg#new... skin/delete.svg:1: <svg id="5dbfc7a9-72c4-4898-881d-75bf73a9a059" data-name="artwork" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"><title>delete</title><polygon points="42.71 6.71 41.29 5.29 24 22.59 6.71 5.29 5.29 6.71 22.59 24 5.29 41.29 6.71 42.71 24 25.41 41.29 42.71 42.71 41.29 25.41 24 42.71 6.71"/></svg> On 2017/08/24 14:18:26, saroyanm wrote: > On 2017/08/24 10:08:44, ire wrote: > > For some reason the weight/thickness of the icon seems much lighter than in > the > > spec. This makes it quite difficult to see. > > I've created a bitbucket issue for that -> > https://bitbucket.org/adblockplus/spec/issues/68 > This shouldn't be blocker for this review though, we will fix it later as soon > new SVG is provided. Acknowledged. https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1094: box-sizing: border-box; On 2017/08/24 14:18:26, saroyanm wrote: > On 2017/08/24 10:08:44, ire wrote: > > Not sure if this will be addressed by implementing issue 5543, but this > > box-sizing style seems like it should be applied to all elements > > Not sure if we should apply this to all elements, I know that the practice > exists. > If it appears that we need this on all elements will apply globally in separate > review. > But the reason why I did it so is because the scrollbar that was appearing when > I was using padding as this is 100% width absolute item. Acknowledged. https://codereview.adblockplus.org/29519650/diff/29525623/skin/new-options.cs... skin/new-options.css:1099: background-color: #E1F2FA; On 2017/08/24 14:18:26, saroyanm wrote: > On 2017/08/24 10:08:44, ire wrote: > > (I think this may have been discussed already but) this color to > > background-color contrast ratio is just 2.7 > > I agree, that the contrast needs to be adjusted. > > but that shouldn't be blocker for the review. > > So either I can bring that up during the next meeting, as I collecting similar > questions. So we will have something ready to show as well. Or maybe you can > create a bitbucket issue explaining the contrast. It's fine to bring up during the next meeting since it'll likely affect other parts of the site as well https://codereview.adblockplus.org/29519650/diff/29526765/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29526765/new-options.html#ne... new-options.html:380: <button id="hide-notification" data-action="hide-notification" aria-label="Close"></button> I think "Close Notification" is more clear https://codereview.adblockplus.org/29519650/diff/29526765/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519650/diff/29526765/new-options.js#newc... new-options.js:1004: function showNotification(text) This doesn't need to hold up this review, but we could manage focus a bit better here. For a keyboard user, when they "click" on the "Add website" button, focus disappears. I have 3 suggestions for how to handle this, in order of preference: 1. Move focus to the close notification button. This moves the attention of the keyboard user to the notification, which is sort of the purpose of it. When the user clicks to hide (or the notification times out), move their focus back to the input field 2. Move focus back to the input field immediately. This makes sense since the field was just cleared. 3. Move focus to the delete button for that website. This has the potential for the user to mistakenly delete the website they just added, which is why it's my last choice. It's up to you if you want to handle this in this review, or later on.
Thanks Ire, I've addressed your comments and a new patch is uploaded. https://codereview.adblockplus.org/29519650/diff/29526765/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29526765/new-options.html#ne... new-options.html:380: <button id="hide-notification" data-action="hide-notification" aria-label="Close"></button> On 2017/08/25 09:59:45, ire wrote: > I think "Close Notification" is more clear Done. https://codereview.adblockplus.org/29519650/diff/29526765/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519650/diff/29526765/new-options.js#newc... new-options.js:1004: function showNotification(text) On 2017/08/25 09:59:45, ire wrote: > This doesn't need to hold up this review, but we could manage focus a bit better > here. For a keyboard user, when they "click" on the "Add website" button, focus > disappears. > > I have 3 suggestions for how to handle this, in order of preference: > > 1. Move focus to the close notification button. This moves the attention of the > keyboard user to the notification, which is sort of the purpose of it. When the > user clicks to hide (or the notification times out), move their focus back to > the input field > > 2. Move focus back to the input field immediately. This makes sense since the > field was just cleared. > > 3. Move focus to the delete button for that website. This has the potential for > the user to mistakenly delete the website they just added, which is why it's my > last choice. > > It's up to you if you want to handle this in this review, or later on. It's bit difficult because we don't know where is user located when the notification is being triggered, please note that the notification is not being triggered when user click on Add button, but when a filter is being added, this might also happen in the Advanced tab if user adds a filter that is a whitlisting filter. I do agree that we should improve the focus handling, but it's bit more complex as it looks to be, so considering the fact that notification is being hidden automatically I think it's fine keeping focus where it is. Also user don't need to navigate to the Add Website button, but trigger submit by hitting enter.
https://codereview.adblockplus.org/29519650/diff/29526765/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519650/diff/29526765/new-options.js#newc... new-options.js:1004: function showNotification(text) On 2017/08/25 11:04:19, saroyanm wrote: > On 2017/08/25 09:59:45, ire wrote: > > This doesn't need to hold up this review, but we could manage focus a bit > better > > here. For a keyboard user, when they "click" on the "Add website" button, > focus > > disappears. > > > > I have 3 suggestions for how to handle this, in order of preference: > > > > 1. Move focus to the close notification button. This moves the attention of > the > > keyboard user to the notification, which is sort of the purpose of it. When > the > > user clicks to hide (or the notification times out), move their focus back to > > the input field > > > > 2. Move focus back to the input field immediately. This makes sense since the > > field was just cleared. > > > > 3. Move focus to the delete button for that website. This has the potential > for > > the user to mistakenly delete the website they just added, which is why it's > my > > last choice. > > > > It's up to you if you want to handle this in this review, or later on. > > It's bit difficult because we don't know where is user located when the > notification is being triggered, please note that the notification is not being > triggered when user click on Add button, but when a filter is being added, this > might also happen in the Advanced tab if user adds a filter that is a > whitlisting filter. > > I do agree that we should improve the focus handling, but it's bit more complex > as it looks to be, so considering the fact that notification is being hidden > automatically I think it's fine keeping focus where it is. > > Also user don't need to navigate to the Add Website button, but trigger submit > by hitting enter. Okay good point. In light of what you said, I think my second option is now my preference (moving focus back to the input field after it's cleared). But perhaps this is something we should discuss in the next meeting with Jeen to help us decide what the best implementation would be https://codereview.adblockplus.org/29519650/diff/29527656/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29527656/new-options.html#ne... new-options.html:380: <button id="hide-notification" data-action="hide-notification" aria-label="Close Notification"></button> Sorry I didn't notice this before, but shouldn't the aria-label be translated as well?
https://codereview.adblockplus.org/29519650/diff/29526765/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519650/diff/29526765/new-options.js#newc... new-options.js:1004: function showNotification(text) On 2017/08/25 12:10:07, ire wrote: > On 2017/08/25 11:04:19, saroyanm wrote: > > On 2017/08/25 09:59:45, ire wrote: > > > This doesn't need to hold up this review, but we could manage focus a bit > > better > > > here. For a keyboard user, when they "click" on the "Add website" button, > > focus > > > disappears. > > > > > > I have 3 suggestions for how to handle this, in order of preference: > > > > > > 1. Move focus to the close notification button. This moves the attention of > > the > > > keyboard user to the notification, which is sort of the purpose of it. When > > the > > > user clicks to hide (or the notification times out), move their focus back > to > > > the input field > > > > > > 2. Move focus back to the input field immediately. This makes sense since > the > > > field was just cleared. > > > > > > 3. Move focus to the delete button for that website. This has the potential > > for > > > the user to mistakenly delete the website they just added, which is why it's > > my > > > last choice. > > > > > > It's up to you if you want to handle this in this review, or later on. > > > > It's bit difficult because we don't know where is user located when the > > notification is being triggered, please note that the notification is not > being > > triggered when user click on Add button, but when a filter is being added, > this > > might also happen in the Advanced tab if user adds a filter that is a > > whitlisting filter. > > > > I do agree that we should improve the focus handling, but it's bit more > complex > > as it looks to be, so considering the fact that notification is being hidden > > automatically I think it's fine keeping focus where it is. > > > > Also user don't need to navigate to the Add Website button, but trigger submit > > by hitting enter. > > Okay good point. In light of what you said, I think my second option is now my > preference (moving focus back to the input field after it's cleared). But > perhaps this is something we should discuss in the next meeting with Jeen to > help us decide what the best implementation would be I agree https://codereview.adblockplus.org/29519650/diff/29527656/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29527656/new-options.html#ne... new-options.html:380: <button id="hide-notification" data-action="hide-notification" aria-label="Close Notification"></button> On 2017/08/25 12:10:07, ire wrote: > Sorry I didn't notice this before, but shouldn't the aria-label be translated as > well? Thanks for bringing this up. There is no easy way to translate attribute values currently unfortunately. So to translate the values will require to do that using JavaScript which will become a headache on current stage, but we should fix that in future. Well we probably will need to fix whole translation, currently it's done using CLASSes which was previously done because of browsers were not supporting data attributes.
https://codereview.adblockplus.org/29519650/diff/29527656/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29527656/new-options.html#ne... new-options.html:380: <button id="hide-notification" data-action="hide-notification" aria-label="Close Notification"></button> On 2017/08/25 12:22:56, saroyanm wrote: > On 2017/08/25 12:10:07, ire wrote: > > Sorry I didn't notice this before, but shouldn't the aria-label be translated > as > > well? > > Thanks for bringing this up. > There is no easy way to translate attribute values currently unfortunately. So > to translate the values will require to do that using JavaScript which will > become a headache on current stage, but we should fix that in future. Well we > probably will need to fix whole translation, currently it's done using CLASSes > which was previously done because of browsers were not supporting data > attributes. You're welcome :) A suggestion for now could be to add a <span> element within the button that would only be visible to screen readers that would have the content. e.g. <button id="hide-notification" data-action="hide-notification"> <span class="sr-only i18n_hide_notification"></span> </button> The .sr-only class would be hidden from visual view, e.g. https://hg.adblockplus.org/web.acceptableads.com/file/6e89cf6909de/static/scs... This would solve the problem for now until all translations are refactored.
https://codereview.adblockplus.org/29519650/diff/29527656/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29527656/new-options.html#ne... new-options.html:380: <button id="hide-notification" data-action="hide-notification" aria-label="Close Notification"></button> On 2017/08/25 12:43:44, ire wrote: > On 2017/08/25 12:22:56, saroyanm wrote: > > On 2017/08/25 12:10:07, ire wrote: > > > Sorry I didn't notice this before, but shouldn't the aria-label be > translated > > as > > > well? > > > > Thanks for bringing this up. > > There is no easy way to translate attribute values currently unfortunately. So > > to translate the values will require to do that using JavaScript which will > > become a headache on current stage, but we should fix that in future. Well we > > probably will need to fix whole translation, currently it's done using CLASSes > > which was previously done because of browsers were not supporting data > > attributes. > > You're welcome :) A suggestion for now could be to add a <span> element within > the button that would only be visible to screen readers that would have the > content. e.g. > > <button id="hide-notification" data-action="hide-notification"> > <span class="sr-only i18n_hide_notification"></span> > </button> > > The .sr-only class would be hidden from visual view, e.g. > https://hg.adblockplus.org/web.acceptableads.com/file/6e89cf6909de/static/scs... > > This would solve the problem for now until all translations are refactored. Great idea :) Done.
Rebased
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
