Issue 29578574: Issue 5632 - Use checkboxes for toggling acceptable ads (Closed)
|
Created:
Oct. 16, 2017, 11:02 a.m. by saroyanm Modified:
Oct. 23, 2017, 11:16 a.m. Visibility:
Public. |
DescriptionIssue 5632 - Use checkboxes for toggling acceptable ads
Patch Set 1 : #
Total comments: 26
Patch Set 2 : Rebased #Patch Set 3 : Addressed Ire's comments #Patch Set 4 : Keep the focus of disabled elements #
Total comments: 4
Patch Set 5 : Removed strings #
Total comments: 4
Patch Set 6 : Addressed Ire's comments #
Total comments: 10
Patch Set 7 : Addressed Thomas Comments #Patch Set 8 : Fixed the duplication #
MessagesTotal messages: 14
@Ire can you please have a look, I'd like to ask you @Thomas to do a high level review after I'll get a go from Ire, because this review contain also Acceptable Ads logic change which will require module owner attention before being pushed.
https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:898: getDocLink("privacy_friendly_ads", (link) => We still missing this redirection link, I'll reach Winsley regarding this and will be sure not to push this before we will have one. https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:1063: if (navigator.doNotTrack != 1) See -> https://issues.adblockplus.org/ticket/5866 I think we can handle this here.
Thanks Manvel. Comments below https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:658: let isAcceptableAds = value == "ads"; I'm getting the eslint error: isAcceptableAds is already declared in the upper scope (no-shadow) I think this is to do with the isAcceptableAds function that's already defined. Since you're checking if it's the #acceptable-ads-allow element, perhaps you can name this something like `isAcceptableAdsAllow` https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:898: getDocLink("privacy_friendly_ads", (link) => On 2017/10/16 17:45:51, saroyanm wrote: > We still missing this redirection link, I'll reach Winsley regarding this and > will be sure not to push this before we will have one. Acknowledged. https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:1051: acceptableAdsPrivacy.disabled = true; When you toggle the acceptableAdsPrivacy checkbox on and off via keyboard, focus is moved away from it. I narrowed it down to this line. I guess because it's being set as disabled temporarily, focus is removed for it. https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:1063: if (navigator.doNotTrack != 1) On 2017/10/16 17:45:52, saroyanm wrote: > See -> https://issues.adblockplus.org/ticket/5866 > > I think we can handle this here. Seen. I agree with you. https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:1063: if (navigator.doNotTrack != 1) I don't know if support for Edge/Safari is relevant here, but they use `window.doNotTrack` instead (see https://developer.mozilla.org/en-US/docs/Web/API/Navigator/doNotTrack#Browser...) https://codereview.adblockplus.org/29578574/diff/29580578/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29578574/diff/29580578/skin/desktop-option... skin/desktop-options.css:632: #acceptable-ads ul li button NIT: This may not be to be so specific. `#acceptable-ads button` also works https://codereview.adblockplus.org/29578574/diff/29580578/skin/desktop-option... skin/desktop-options.css:636: left: 0rem; The checkbox is still on the left when direction is rtl https://codereview.adblockplus.org/29578574/diff/29580578/skin/desktop-option... skin/desktop-options.css:639: #acceptable-ads ul li label NIT: Same here. Suggest: `#acceptable-ads label` https://codereview.adblockplus.org/29578574/diff/29580578/skin/desktop-option... skin/desktop-options.css:651: .new In the spec, this text is uppercase. Also, it seems like there should be more padding on the right and left
New patch uploaded. I'll try to think about solution for the focus issue of the disable toggle. https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:658: let isAcceptableAds = value == "ads"; On 2017/10/17 08:16:34, ire wrote: > I'm getting the eslint error: isAcceptableAds is already declared in the upper > scope (no-shadow) > > I think this is to do with the isAcceptableAds function that's already defined. > > Since you're checking if it's the #acceptable-ads-allow element, perhaps you can > name this something like `isAcceptableAdsAllow` Thanks, I isolated scope with the braces. Let me know if you think renaming still make sense. https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:1051: acceptableAdsPrivacy.disabled = true; On 2017/10/17 08:16:34, ire wrote: > When you toggle the acceptableAdsPrivacy checkbox on and off via keyboard, focus > is moved away from it. I narrowed it down to this line. I guess because it's > being set as disabled temporarily, focus is removed for it. yes this is trickier than it appears. I'll try to address this in a separate patch. I moved this to the "else" case, but this is not a solution, the problem here is that we are disabling the toggle when both Acceptable Ads and Acceptable Ads that Privacy Friendly list are not installed, but while the addition and removal is async and we can't be completely sure what was the user intention when one or another list is removed or added it makes this bit tricky when browsers remove focus from focused elements. So the scenario is that when we are clicking on the toggle we do remove one list and add the another one and if one is being removed before the other one is added we have the toggle set to disabled. https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:1063: if (navigator.doNotTrack != 1) On 2017/10/17 08:16:34, ire wrote: > I don't know if support for Edge/Safari is relevant here, but they use > `window.doNotTrack` instead (see > https://developer.mozilla.org/en-US/docs/Web/API/Navigator/doNotTrack#Browser...) We don't consider Safari, but it make sense for the Edge, we have a lot of to adapt, we will cover that here -> https://issues.adblockplus.org/ticket/5813 https://codereview.adblockplus.org/29578574/diff/29580578/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29578574/diff/29580578/skin/desktop-option... skin/desktop-options.css:632: #acceptable-ads ul li button On 2017/10/17 08:16:34, ire wrote: > NIT: This may not be to be so specific. `#acceptable-ads button` also works Done. https://codereview.adblockplus.org/29578574/diff/29580578/skin/desktop-option... skin/desktop-options.css:632: #acceptable-ads ul li button On 2017/10/17 08:16:34, ire wrote: > NIT: This may not be to be so specific. `#acceptable-ads button` also works Done. https://codereview.adblockplus.org/29578574/diff/29580578/skin/desktop-option... skin/desktop-options.css:636: left: 0rem; On 2017/10/17 08:16:34, ire wrote: > The checkbox is still on the left when direction is rtl Done. https://codereview.adblockplus.org/29578574/diff/29580578/skin/desktop-option... skin/desktop-options.css:639: #acceptable-ads ul li label On 2017/10/17 08:16:34, ire wrote: > NIT: Same here. > > Suggest: `#acceptable-ads label` Done. https://codereview.adblockplus.org/29578574/diff/29580578/skin/desktop-option... skin/desktop-options.css:639: #acceptable-ads ul li label On 2017/10/17 08:16:34, ire wrote: > NIT: Same here. > > Suggest: `#acceptable-ads label` Done. https://codereview.adblockplus.org/29578574/diff/29580578/skin/desktop-option... skin/desktop-options.css:651: .new On 2017/10/17 08:16:34, ire wrote: > In the spec, this text is uppercase. Also, it seems like there should be more > padding on the right and left I used the styles that match the old design here -> https://bitbucket.org/adblockplus/spec/src/32814c2fc398a48c0b55c1323bae5efe98... Because seems like we will need to adjust a lot of other styles to make consistent.Though I increased the padding a bit more.
New patch uploaded. https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:1051: acceptableAdsPrivacy.disabled = true; On 2017/10/17 19:23:07, saroyanm wrote: > On 2017/10/17 08:16:34, ire wrote: > > When you toggle the acceptableAdsPrivacy checkbox on and off via keyboard, > focus > > is moved away from it. I narrowed it down to this line. I guess because it's > > being set as disabled temporarily, focus is removed for it. > > yes this is trickier than it appears. I'll try to address this in a separate > patch. > > I moved this to the "else" case, but this is not a solution, the problem here is > that we are disabling the toggle when both Acceptable Ads and Acceptable Ads > that Privacy Friendly list are not installed, but while the addition and removal > is async and we can't be completely sure what was the user intention when one or > another list is removed or added it makes this bit tricky when browsers remove > focus from focused elements. > So the scenario is that when we are clicking on the toggle we do remove one list > and add the another one and if one is being removed before the other one is > added we have the toggle set to disabled. Done.
https://codereview.adblockplus.org/29578574/diff/29581910/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29578574/diff/29581910/locale/en_US/deskto... locale/en_US/desktop-options.json:2: "options_page_title": { Note: we shouldn't add/change strings because of the string freeze :(
Thanks. A few more comments. https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:658: let isAcceptableAds = value == "ads"; On 2017/10/17 19:23:07, saroyanm wrote: > On 2017/10/17 08:16:34, ire wrote: > > I'm getting the eslint error: isAcceptableAds is already declared in the upper > > scope (no-shadow) > > > > I think this is to do with the isAcceptableAds function that's already > defined. > > > > Since you're checking if it's the #acceptable-ads-allow element, perhaps you > can > > name this something like `isAcceptableAdsAllow` > > Thanks, I isolated scope with the braces. Let me know if you think renaming > still make sense. I'm still getting the same error, I don't think the braces made a difference https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:1051: acceptableAdsPrivacy.disabled = true; On 2017/10/17 19:23:07, saroyanm wrote: > On 2017/10/17 08:16:34, ire wrote: > > When you toggle the acceptableAdsPrivacy checkbox on and off via keyboard, > focus > > is moved away from it. I narrowed it down to this line. I guess because it's > > being set as disabled temporarily, focus is removed for it. > > yes this is trickier than it appears. I'll try to address this in a separate > patch. > > I moved this to the "else" case, but this is not a solution, the problem here is > that we are disabling the toggle when both Acceptable Ads and Acceptable Ads > that Privacy Friendly list are not installed, but while the addition and removal > is async and we can't be completely sure what was the user intention when one or > another list is removed or added it makes this bit tricky when browsers remove > focus from focused elements. > So the scenario is that when we are clicking on the toggle we do remove one list > and add the another one and if one is being removed before the other one is > added we have the toggle set to disabled. Ah I see, very tricky! https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:1063: if (navigator.doNotTrack != 1) On 2017/10/17 19:23:07, saroyanm wrote: > On 2017/10/17 08:16:34, ire wrote: > > I don't know if support for Edge/Safari is relevant here, but they use > > `window.doNotTrack` instead (see > > > https://developer.mozilla.org/en-US/docs/Web/API/Navigator/doNotTrack#Browser...) > > We don't consider Safari, but it make sense for the Edge, we have a lot of to > adapt, we will cover that here -> https://issues.adblockplus.org/ticket/5813 Acknowledged. https://codereview.adblockplus.org/29578574/diff/29581910/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29578574/diff/29581910/locale/en_US/deskto... locale/en_US/desktop-options.json:2: "options_page_title": { On 2017/10/18 11:17:55, saroyanm wrote: > Note: we shouldn't add/change strings because of the string freeze :( String freeze? (I assume this is why the "new" label is missing?) https://codereview.adblockplus.org/29578574/diff/29582663/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29582663/desktop-options.js#... desktop-options.js:1072: acceptableAdsPrivacy.setAttribute("aria-disabled", true); This is a fine compromise. We should also set the tabindex to -1 when disabled, to stop users being able to focus on it via keyboard when it is disabled. (I tried this and it doesn't stop the element being focused when toggling it specifically) https://codereview.adblockplus.org/29578574/diff/29582663/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29578574/diff/29582663/skin/desktop-option... skin/desktop-options.css:98: [aria-disabled="true"] There's already a set of styles applied to disabled checkbox buttons below (which is slightly different to what you have here) You can extend that one by adding this selector to it. The selector is `button[role="checkbox"][disabled]` (line 183)
New patch uploaded. https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:658: let isAcceptableAds = value == "ads"; On 2017/10/18 17:48:45, ire wrote: > On 2017/10/17 19:23:07, saroyanm wrote: > > On 2017/10/17 08:16:34, ire wrote: > > > I'm getting the eslint error: isAcceptableAds is already declared in the > upper > > > scope (no-shadow) > > > > > > I think this is to do with the isAcceptableAds function that's already > > defined. > > > > > > Since you're checking if it's the #acceptable-ads-allow element, perhaps you > > can > > > name this something like `isAcceptableAdsAllow` > > > > Thanks, I isolated scope with the braces. Let me know if you think renaming > > still make sense. > > I'm still getting the same error, I don't think the braces made a difference Right, my bad, I thought it was a variable in the same scope as switch, I've missed that. I used "isRegularAcceptableAds", if you have a strong preference regarding the name I still can change it. I'd like to avoid using "allow" not to sound ambiguous in this context. But I also not really sure about this name, I would like to differentiate here between Acceptable Ads and privacy-friendly Acceptable Ads. https://codereview.adblockplus.org/29578574/diff/29581910/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29578574/diff/29581910/locale/en_US/deskto... locale/en_US/desktop-options.json:2: "options_page_title": { On 2017/10/18 17:48:45, ire wrote: > On 2017/10/18 11:17:55, saroyanm wrote: > > Note: we shouldn't add/change strings because of the string freeze :( > > String freeze? > > (I assume this is why the "new" label is missing?) Yes I removed all Strings from here and implemented solution that does not require string changes, I'm planing to pitch all missing strings here -> https://issues.adblockplus.org/ticket/5874 https://codereview.adblockplus.org/29578574/diff/29582663/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29582663/desktop-options.js#... desktop-options.js:1072: acceptableAdsPrivacy.setAttribute("aria-disabled", true); On 2017/10/18 17:48:45, ire wrote: > This is a fine compromise. We should also set the tabindex to -1 when disabled, > to stop users being able to focus on it via keyboard when it is disabled. (I > tried this and it doesn't stop the element being focused when toggling it > specifically) Good point, done. https://codereview.adblockplus.org/29578574/diff/29582663/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29578574/diff/29582663/skin/desktop-option... skin/desktop-options.css:98: [aria-disabled="true"] On 2017/10/18 17:48:45, ire wrote: > There's already a set of styles applied to disabled checkbox buttons below > (which is slightly different to what you have here) > > You can extend that one by adding this selector to it. > > The selector is `button[role="checkbox"][disabled]` (line 183) Great idea, done.
LGTM https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29580578/desktop-options.js#... desktop-options.js:658: let isAcceptableAds = value == "ads"; On 2017/10/19 20:41:55, saroyanm wrote: > On 2017/10/18 17:48:45, ire wrote: > > On 2017/10/17 19:23:07, saroyanm wrote: > > > On 2017/10/17 08:16:34, ire wrote: > > > > I'm getting the eslint error: isAcceptableAds is already declared in the > > upper > > > > scope (no-shadow) > > > > > > > > I think this is to do with the isAcceptableAds function that's already > > > defined. > > > > > > > > Since you're checking if it's the #acceptable-ads-allow element, perhaps > you > > > can > > > > name this something like `isAcceptableAdsAllow` > > > > > > Thanks, I isolated scope with the braces. Let me know if you think renaming > > > still make sense. > > > > I'm still getting the same error, I don't think the braces made a difference > > Right, my bad, I thought it was a variable in the same scope as switch, I've > missed that. > I used "isRegularAcceptableAds", if you have a strong preference regarding the > name I still can change it. > I'd like to avoid using "allow" not to sound ambiguous in this context. > > But I also not really sure about this name, I would like to differentiate here > between Acceptable Ads and privacy-friendly Acceptable Ads. I don't have a strong preference about the name. I can't think of a better name than what you have here already, so I think it's good enough. https://codereview.adblockplus.org/29578574/diff/29581910/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29578574/diff/29581910/locale/en_US/deskto... locale/en_US/desktop-options.json:2: "options_page_title": { On 2017/10/19 20:41:55, saroyanm wrote: > On 2017/10/18 17:48:45, ire wrote: > > On 2017/10/18 11:17:55, saroyanm wrote: > > > Note: we shouldn't add/change strings because of the string freeze :( > > > > String freeze? > > > > (I assume this is why the "new" label is missing?) > > Yes I removed all Strings from here and implemented solution that does not > require string changes, I'm planing to pitch all missing strings here -> > https://issues.adblockplus.org/ticket/5874 Oh I see. Ack.
https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.htm... desktop-options.html:113: </a> Detail: We don't want to hyperlink the entire block but only the text so let's wrap those elements the other way around. i.e. <p> <a id="..." class="i18n_..."></a> </p> https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.js#... desktop-options.js:562: if (element.getAttribute("aria-disabled") == "true") Detail: No need to check for "true" or "false" when dealing with binary attributes. They either exist or they don't. So we should be able to use `Element.hasAttribute()` instead. Same applies to "aria-checked". https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.js#... desktop-options.js:672: }); This is very hard to read so for the sake of maintainability I'd strongly recommend simplifying this a bit to avoid mistakes because it's crucial that this work correctly.
Thanks Thomas, new patch uploaded. https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.htm... desktop-options.html:113: </a> On 2017/10/20 15:35:03, Thomas Greiner wrote: > Detail: We don't want to hyperlink the entire block but only the text so let's > wrap those elements the other way around. > > i.e. > <p> > <a id="..." class="i18n_..."></a> > </p> Done, I'll fix wording and section in separate issue where we will be able to introduce/update existing strings. https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.js#... desktop-options.js:562: if (element.getAttribute("aria-disabled") == "true") On 2017/10/20 15:35:03, Thomas Greiner wrote: > Detail: No need to check for "true" or "false" when dealing with binary > attributes. They either exist or they don't. So we should be able to use > `Element.hasAttribute()` instead. > > Same applies to "aria-checked". In our code in some places we do set attribute like: Element.setAttribute("aria-checked", value); I think we should refactor whole codebase to use hasAttribute for binary attributes. That will require to create setBinaryAttribute function in general. If you agree with that I can create separate ticket, I also can fix that for introduced attributes in current review, but in that case we will have inconsistent codebase, if that worth it I'm ready to do that, not sure what is better in this case. https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.js#... desktop-options.js:672: }); On 2017/10/20 15:35:03, Thomas Greiner wrote: > This is very hard to read so for the sake of maintainability I'd strongly > recommend simplifying this a bit to avoid mistakes because it's crucial that > this work correctly. I agree with you, I made it hopefully simpler now.
https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.js#... desktop-options.js:562: if (element.getAttribute("aria-disabled") == "true") On 2017/10/20 16:42:06, saroyanm wrote: > If you agree with that I can create separate ticket, I also can fix that for > introduced attributes in current review, but in that case we will have > inconsistent codebase, if that worth it I'm ready to do that, not sure what is > better in this case. I'm fine with working on this in a separate ticket. https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.js#... desktop-options.js:672: }); On 2017/10/20 16:42:06, saroyanm wrote: > On 2017/10/20 15:35:03, Thomas Greiner wrote: > > This is very hard to read so for the sake of maintainability I'd strongly > > recommend simplifying this a bit to avoid mistakes because it's crucial that > > this work correctly. > > I agree with you, I made it hopefully simpler now. It's better. As mentioned in person, I'd avoid duplicating the `browser.runtime.sendMessage()` calls though and rename `isCheck` to something along the lines of `shouldCheck` to clarify its purpose.
New patch uploaded https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.js#... desktop-options.js:562: if (element.getAttribute("aria-disabled") == "true") On 2017/10/20 17:59:57, Thomas Greiner wrote: > On 2017/10/20 16:42:06, saroyanm wrote: > > If you agree with that I can create separate ticket, I also can fix that for > > introduced attributes in current review, but in that case we will have > > inconsistent codebase, if that worth it I'm ready to do that, not sure what is > > better in this case. > > I'm fine with working on this in a separate ticket. Ticket: https://issues.adblockplus.org/ticket/5900 https://codereview.adblockplus.org/29578574/diff/29583616/desktop-options.js#... desktop-options.js:672: }); On 2017/10/20 17:59:57, Thomas Greiner wrote: > On 2017/10/20 16:42:06, saroyanm wrote: > > On 2017/10/20 15:35:03, Thomas Greiner wrote: > > > This is very hard to read so for the sake of maintainability I'd strongly > > > recommend simplifying this a bit to avoid mistakes because it's crucial that > > > this work correctly. > > > > I agree with you, I made it hopefully simpler now. > > It's better. As mentioned in person, I'd avoid duplicating the > `browser.runtime.sendMessage()` calls though and rename `isCheck` to something > along the lines of `shouldCheck` to clarify its purpose. Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
