Issue 6510 - Configure notification opens options page incorrectly

Issue 6510 - Configure notification opens options page incorrectly

[saroyanm, 2018-03-22 16:12:45]

Andrea can you please have a look

[saroyanm, 2018-03-22 16:15:53]

https://codereview.adblockplus.org/29730617/diff/29730627/js/desktop-options.js
File js/desktop-options.js (right):

https://codereview.adblockplus.org/29730617/diff/29730627/js/desktop-options....
js/desktop-options.js:1459: case "focusSection":
We used to set the tab view to the notifications ->
https://hg.adblockplus.org/adblockplusui/file/tip/js/desktop-options.js#l1454
The actual argument is sent from here ->
https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/notificationHelper....

[a.giammarchi, 2018-03-22 18:43:28]

nothing crucial, nothing a real blocker, but I have few questions before moving
forward.

https://codereview.adblockplus.org/29730617/diff/29730627/css/desktop-options...
File css/desktop-options.scss (right):

https://codereview.adblockplus.org/29730617/diff/29730627/css/desktop-options...
css/desktop-options.scss:551: 0% { background: none }
I think the neutral value for background-color is transparent.

`none` IIRC is only for background-image, to nullify any data/url.

https://codereview.adblockplus.org/29730617/diff/29730627/css/desktop-options...
css/desktop-options.scss:552: 30% { background: #ffd7a3 }
whenever you're changing only one part of a complex CSS property, is usually
better to address it directly. background is a mess of overloads and it's able
to overwrite many other things if set directly. background-color though, is what
you are after here, so I'd go with background-color.

https://codereview.adblockplus.org/29730617/diff/29730627/js/desktop-options.js
File js/desktop-options.js (right):

https://codereview.adblockplus.org/29730617/diff/29730627/js/desktop-options....
js/desktop-options.js:1356: let element = getPrefElement(key);
let is OK, but why not `const` here ?

https://codereview.adblockplus.org/29730617/diff/29730627/js/desktop-options....
js/desktop-options.js:1363: return document.querySelector("[data-pref='" + key +
"']");
out of curiosity: are keys always sanitized or some key might contain a double
quote char, as example?

https://codereview.adblockplus.org/29730617/diff/29730627/js/desktop-options....
js/desktop-options.js:1465: elem.classList.add("highlight-animate");
so, in hidePref you check if elem is there before using it. Is it needed to be
checked in here too?

[saroyanm, 2018-03-22 20:43:22]

Thanks for review Andrea,  otherwise wouldn't be easy to move this forward as
Thomas is on vacation.

New patch is uploaded, ready  for the next round.

https://codereview.adblockplus.org/29730617/diff/29730627/css/desktop-options...
File css/desktop-options.scss (right):

https://codereview.adblockplus.org/29730617/diff/29730627/css/desktop-options...
css/desktop-options.scss:551: 0% { background: none }
On 2018/03/22 18:43:28, a.giammarchi wrote:
> I think the neutral value for background-color is transparent.
> 
> `none` IIRC is only for background-image, to nullify any data/url.
> 

Done.

https://codereview.adblockplus.org/29730617/diff/29730627/css/desktop-options...
css/desktop-options.scss:552: 30% { background: #ffd7a3 }
On 2018/03/22 18:43:28, a.giammarchi wrote:
> whenever you're changing only one part of a complex CSS property, is usually
> better to address it directly. background is a mess of overloads and it's able
> to overwrite many other things if set directly. background-color though, is
what
> you are after here, so I'd go with background-color.

Agree, done.

https://codereview.adblockplus.org/29730617/diff/29730627/js/desktop-options.js
File js/desktop-options.js (right):

https://codereview.adblockplus.org/29730617/diff/29730627/js/desktop-options....
js/desktop-options.js:1356: let element = getPrefElement(key);
On 2018/03/22 18:43:28, a.giammarchi wrote:
> let is OK, but why not `const` here ?

Done.

https://codereview.adblockplus.org/29730617/diff/29730627/js/desktop-options....
js/desktop-options.js:1363: return document.querySelector("[data-pref='" + key +
"']");
On 2018/03/22 18:43:28, a.giammarchi wrote:
> out of curiosity: are keys always sanitized or some key might contain a double
> quote char, as example?

They can contain, I don't think we are  sanitizing keys.

https://codereview.adblockplus.org/29730617/diff/29730627/js/desktop-options....
js/desktop-options.js:1465: elem.classList.add("highlight-animate");
On 2018/03/22 18:43:28, a.giammarchi wrote:
> so, in hidePref you check if elem is there before using it. Is it needed to be
> checked in here too?

Not necessarily, because "notifications_ignoredcategories" is hardcoded in
desktop-options.html, we know that it always exist.

[a.giammarchi, 2018-03-22 22:51:29]

LGTM