Issue 29674584: Issue 5549 - Implement missing error handlings for custom filter lists (Closed)
|
Created:
Jan. 19, 2018, 11:46 a.m. by a.giammarchi Modified:
Nov. 7, 2018, 3 p.m. Visibility:
Public. |
DescriptionIrrelevant at this point.
Patch Set 1 #Patch Set 2 : fixed linting typos #
Total comments: 18
Patch Set 3 : applied required changes #Patch Set 4 : fixed extra typo in css #Patch Set 5 : enabled testing via ?filterError=true #
Total comments: 20
Patch Set 6 : changes as requested #
Total comments: 1
Patch Set 7 : removed filterError flag + added better way to check invalid filters #Patch Set 8 : using convertObject #
Total comments: 7
Patch Set 9 : applied almost all changes #Patch Set 10 : put loadCustomFilters back #
Total comments: 15
Patch Set 11 : applied changes #Patch Set 12 : applied changes #Patch Set 13 : changed custom-filters-edit-area to custom-filters-control as suggested #
MessagesTotal messages: 26
I am sure there will be something to adjust and I am going to investigate a bit about tests to see how to reliably/automatically check these changes. Meanwhile I'd like some eyes on this PR to be sure it's going to the right direction, thanks.
Thanks for giving a go to this issue Andrea, it was laying around for a long time. I'm happy that we finally are tackling it. My comments are ready, I only didn't review CSS file yet (See the comment in the file). https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.htm... desktop-options.html:32: <body data-tab="general" dir="rtl"> Why direction RTL ? BTW we do specify the directions on <html> element here -> https://hg.adblockplus.org/adblockplusui/file/tip/i18n.js#l32 If you would like to test RTL direction in the test environment you can also specify locale parameter in the URL. ex.: http://127.0.0.1:5000/desktop-options.html?locale=ar It's also handy for testing the translations. https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.htm... desktop-options.html:293: <a class="i18n_options_customFilters_learn" id="link-filters-on-edit-error" target="_blank"></a> In general we use text specified in the specification text, not in the screenshot (sometimes screenshots are being outdated and for accessibility reasons), but I agree with you that we should have this link here, I only think that it should be persistent rather than only when the filter is invalid, I did created an issue to update the specs -> https://gitlab.com/eyeo/spec/issues/133 in order to have the reference link below the section description. https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js#... desktop-options.js:550: function sendMessageHandleErrors(message, onMessage) Suggestion/detail: seems like onMessage is used for the callback, what about renaming it so "callback" instead of "onMessage" ? https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js#... desktop-options.js:665: E("custom-filters-edit-error").classList.add("warning"); Suggestion: I think we can use "custom-filters" in order to specify the "warning" message, rather than on each element separately. https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js#... desktop-options.js:666: E("link-filters-on-edit-error").classList.add("visible"); Note: If we agree on making this message persistent, we probably won't need this. https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js#... desktop-options.js:668: const info = errors.map(error => lines[error.lineno - 1]); I think would be great if we could return the filter within the error object in the messageResponder.js(filters.importRaw), so we will be sure that the "lines" are the one we have initially passed. https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js#... desktop-options.js:668: const info = errors.map(error => lines[error.lineno - 1]); Note: As soon we introduce a new functionality which is not yet testable in the test environment we also update background.js. background.js is only for test, it imitates production background page/script, it doesn't have to match 100%, but at least return similar error object, so we can easily test locally. https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js#... desktop-options.js:670: editFilters.textContent = info.join("\n"); Adding all the elements as a textContent to div makes it less accessible. I'd suggest to use a textarea or list with overflow, that should also help us not to specify "many" class. https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js#... desktop-options.js:777: E("custom-filters-raw").classList.remove("warning"); Note: As mentioned above we don't need to remove the warning twice if we use parent element. https://codereview.adblockplus.org/29674584/diff/29677632/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29674584/diff/29677632/skin/desktop-option... skin/desktop-options.css:18: @font-face Any Idea what causes this diff showing change on each line ? I don't think that the UNIX line ending was changed by Windows one, or smth similar, right ? At least I can't see same output in the initial patch.
answered here and there. https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.htm... desktop-options.html:32: <body data-tab="general" dir="rtl"> On 2018/01/23 15:26:05, saroyanm wrote: > Why direction RTL ? > > BTW we do specify the directions on <html> element here -> > https://hg.adblockplus.org/adblockplusui/file/tip/i18n.js#l32 > > If you would like to test RTL direction in the test environment you can also > specify locale parameter in the URL. ex.: > http://127.0.0.1:5000/desktop-options.html?locale=ar It's also handy for testing > the translations. oh gosh that's a "me trying things and not cleaning up" typo. sorry for that https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.htm... desktop-options.html:293: <a class="i18n_options_customFilters_learn" id="link-filters-on-edit-error" target="_blank"></a> On 2018/01/23 15:26:05, saroyanm wrote: > In general we use text specified in the specification text, not in the > screenshot (sometimes screenshots are being outdated and for accessibility > reasons), but I agree with you that we should have this link here, I only think > that it should be persistent rather than only when the filter is invalid, I did > created an issue to update the specs -> https://gitlab.com/eyeo/spec/issues/133 > in order to have the reference link below the section description. I've just followed visual specs here, I probably should've asked around first. https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js#... desktop-options.js:668: const info = errors.map(error => lines[error.lineno - 1]); On 2018/01/23 15:26:05, saroyanm wrote: > Note: As soon we introduce a new functionality which is not yet testable in the > test environment we also update background.js. > background.js is only for test, it imitates production background page/script, > it doesn't have to match 100%, but at least return similar error object, so we > can easily test locally. yup, that's the "there's surely still stuff to do here" part of the ticket ;-) https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js#... desktop-options.js:670: editFilters.textContent = info.join("\n"); On 2018/01/23 15:26:05, saroyanm wrote: > Adding all the elements as a textContent to div makes it less accessible. I'd > suggest to use a textarea or list with overflow, that should also help us not to > specify "many" class. I either follow UI or I don't ... this comment is misleading to me. Have you checked the image? The many class is also accordingly to the UI. There is a border when there are many items, your suggestion won't make that happen, unless I misunderstand. https://codereview.adblockplus.org/29674584/diff/29677632/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29674584/diff/29677632/skin/desktop-option... skin/desktop-options.css:18: @font-face On 2018/01/23 15:26:06, saroyanm wrote: > Any Idea what causes this diff showing change on each line ? > > I don't think that the UNIX line ending was changed by Windows one, or smth > similar, right ? > > At least I can't see same output in the initial patch. Visual Studio Code sometimes is very dumb and causes these issues aligning everything under the closing multi line comment. I'll fix that.
https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js#... desktop-options.js:668: const info = errors.map(error => lines[error.lineno - 1]); On 2018/01/23 15:26:06, saroyanm wrote: > I think would be great if we could return the filter within the error object in > the messageResponder.js(filters.importRaw), so we will be sure that the "lines" > are the one we have initially passed. about this ... the error object doesn't offer much. When it validates, it strips out empty lines. We can either wait for another ticket to have the raw input/processed data or I can just ignore with that split multiple new lines and be done with it. I chose the fast way but I also would prefer the former one.
https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js#... desktop-options.js:670: editFilters.textContent = info.join("\n"); On 2018/01/23 15:43:18, a.giammarchi wrote: > On 2018/01/23 15:26:05, saroyanm wrote: > > Adding all the elements as a textContent to div makes it less accessible. I'd > > suggest to use a textarea or list with overflow, that should also help us not > to > > specify "many" class. > > I either follow UI or I don't ... this comment is misleading to me. Have you > checked the image? > > The many class is also accordingly to the UI. There is a border when there are > many items, your suggestion won't make that happen, unless I misunderstand. I missed that, I didn't review the CSS, my fault. But I think we still can change the element to make it more accessible. I don't have good solution either for specifying the border depending on height or child count.
https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29677632/desktop-options.js#... desktop-options.js:668: const info = errors.map(error => lines[error.lineno - 1]); On 2018/01/23 15:59:23, a.giammarchi wrote: > On 2018/01/23 15:26:06, saroyanm wrote: > > I think would be great if we could return the filter within the error object > in > > the messageResponder.js(filters.importRaw), so we will be sure that the > "lines" > > are the one we have initially passed. > > about this ... the error object doesn't offer much. When it validates, it strips > out empty lines. We can either wait for another ticket to have the raw > input/processed data or I can just ignore with that split multiple new lines and > be done with it. I chose the fast way but I also would prefer the former one. I think I couldn't well describe what I meant: sendMessageHandleErrors method is called async and content of E("custom-filters-raw") can be changed before callback is fired so "lines[error.lineno - 1]" can have a wrong result. I think we should process the E("custom-filters-raw").value in the message responder rather than in the Async callback. I did mean to propose updating "filterValidation", because it might take some time as it's on the platform side, but not UI.
I should've fixed all the issues. I am investigating about the testing part. I need to implement that call and I guess also the test. First time for me, it might take a little while (hopefully not too long though)
thanks to both reviewers for hints and patience. ?filterError=true now shows an error at line 1
Mostly some details and suggestions. https://codereview.adblockplus.org/29674584/diff/29679590/background.js File background.js (right): https://codereview.adblockplus.org/29674584/diff/29679590/background.js#newco... background.js:344: if (params.filterError) Suggestion: copy pasting with the slight modification of current method -> https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/filterValidation.js... Can help us making better test, ex.: Testing 5+ and less than 5 lines. https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.htm... desktop-options.html:293: <a class="i18n_options_customFilters_learn" id="link-filters-on-edit-error" target="_blank"></a> Detail: The current ID of this element is missleading, as we don't only show this on error. https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.htm... desktop-options.html:318: <ul id="custom-filters-edit-filters"></ul> Detail/Suggestion: Why custom-filters-edit-filters ? What about custom-filters-error or smth along that lines ? https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js#... desktop-options.js:654: const filters = E("custom-filters-raw"); Detail/Suggestion: We don't use the element anywhere else. What about changing this to smth like: const filters = E("custom-filters-raw").value; https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js#... desktop-options.js:666: const lines = filtersValue.split(/\n+|\n\s+\n/); Splitting same as it's done in the filterValidation (https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/filterValidation.js...) should be enough -> split("\n"); Suggestion: I'll suggest to add a comment here mentioning that the filterValidation doesn't return "filter" when error occurs, also I think make sense to create a ticket for Platform to update filterValidation in order to return filter when error occurs, so we can get rid of this duplication in future and simplify this (let me know if you will need my help). https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js#... desktop-options.js:669: info.forEach( Detail: We usualy use for..of when looping through arrays, see -> https://adblockplus.org/en/coding-style#javascript-modern https://codereview.adblockplus.org/29674584/diff/29679590/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29674584/diff/29679590/messageResponder.js... messageResponder.js:275: errors.push({ Detail: Why not just pass "error" Object ? errors.push(error);
answered inline https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js#... desktop-options.js:654: const filters = E("custom-filters-raw"); On 2018/01/29 11:44:48, saroyanm wrote: > Detail/Suggestion: We don't use the element anywhere else. > What about changing this to smth like: > const filters = E("custom-filters-raw").value; I'll do that, but this is nitpicking ;-) https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js#... desktop-options.js:666: const lines = filtersValue.split(/\n+|\n\s+\n/); On 2018/01/29 11:44:48, saroyanm wrote: > Splitting same as it's done in the filterValidation > (https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/filterValidation.js...) > should be enough -> split("\n"); I've tested this before writing the RegExp, their logic keeps going through empty lines so it's definitively not the same. This split works better so I don't think I should change it. > Suggestion: I'll suggest to add a comment here mentioning that the > filterValidation doesn't return "filter" when error occurs, also I think make > sense to create a ticket for Platform to update filterValidation in order to > return filter when error occurs, so we can get rid of this duplication in future > and simplify this (let me know if you will need my help). I'll add a comment and also I'll probably need your help. https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js#... desktop-options.js:669: info.forEach( On 2018/01/29 11:44:48, saroyanm wrote: > Detail: We usualy use for..of when looping through arrays, see -> > https://adblockplus.org/en/coding-style#javascript-modern have we banned the forEach from the code? performance wise forEach it's much faster than for..of but I guess we don't really have performance issues so, if that's the case, shouldn't we write in the styleguide that forEach is not OK?
https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js#... desktop-options.js:666: const lines = filtersValue.split(/\n+|\n\s+\n/); On 2018/01/29 13:09:02, a.giammarchi wrote: > On 2018/01/29 11:44:48, saroyanm wrote: > > Splitting same as it's done in the filterValidation > > > (https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/filterValidation.js...) > > should be enough -> split("\n"); > > I've tested this before writing the RegExp, their logic keeps going through > empty lines so it's definitively not the same. This split works better so I > don't think I should change it. I don't understand what you mean by "Logic keeps going through the lines". In order to match implementation in backEnd this should be consistent until we have the logic implemented there. Can you please let me know with which exact input have you tested ? https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js#... desktop-options.js:669: info.forEach( On 2018/01/29 13:09:02, a.giammarchi wrote: > On 2018/01/29 11:44:48, saroyanm wrote: > > Detail: We usualy use for..of when looping through arrays, see -> > > https://adblockplus.org/en/coding-style#javascript-modern > > have we banned the forEach from the code? performance wise forEach it's much > faster than for..of That is a news for me. AFAIK we used for...of because it's faster than forEach.
answered again https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js#... desktop-options.js:666: const lines = filtersValue.split(/\n+|\n\s+\n/); On 2018/01/29 14:30:49, saroyanm wrote: > On 2018/01/29 13:09:02, a.giammarchi wrote: > > On 2018/01/29 11:44:48, saroyanm wrote: > > > Splitting same as it's done in the filterValidation > > > > > > (https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/filterValidation.js...) > > > should be enough -> split("\n"); > > > > I've tested this before writing the RegExp, their logic keeps going through > > empty lines so it's definitively not the same. This split works better so I > > don't think I should change it. > I don't understand what you mean by "Logic keeps going through the lines". > In order to match implementation in backEnd this should be consistent until we > have the logic implemented there. > > Can you please let me know with which exact input have you tested ? if you have a textarea with a b c d the filter you pointed at me will loop through all lines. If d is an error the lineno will be 4. Accordingly, using just \n won't be consistent. The filter is fault tolerant with new lines, it doesn't bother. Neither I do, including possible spaces or multiple new lines. As summary, splitting by \n only has possible cases not covered (multiple \n\n) while current RegExp covers that too without any possible harm to the software, actually granting more consistency with backend. https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js#... desktop-options.js:669: info.forEach( On 2018/01/29 14:30:49, saroyanm wrote: > On 2018/01/29 13:09:02, a.giammarchi wrote: > > On 2018/01/29 11:44:48, saroyanm wrote: > > > Detail: We usualy use for..of when looping through arrays, see -> > > > https://adblockplus.org/en/coding-style#javascript-modern > > > > have we banned the forEach from the code? performance wise forEach it's much > > faster than for..of > That is a news for me. AFAIK we used for...of because it's faster than forEach. It's actually usually slower because it involved Array.prototype[Symbol.iterator] [1] access and uses objects like generators to go through each obj.next() and retrieve the value. While just recently Chrome said that forEach and for/of are just on pair of regular for loops [2], Firefox and Edge aren't quite there yet. Regardless, we don't have here a performance cryptical application, choosing any pattern for performance reasons smells a bit like premature optimisations. Specially here where having a million filters would result in way more troubles than a for loop boost. However, I do care about consistency so if we want to always use for...of loops we should state that somewhere so that next person won't waste time thinking about which one is better/preferred ;-) I really would like all these details to be clear and easy to point at because to me reviews should be fast and/or focus on the right thing. [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... [2] https://v8project.blogspot.de/2017/09/
asked some extra clarification https://codereview.adblockplus.org/29674584/diff/29679590/background.js File background.js (right): https://codereview.adblockplus.org/29674584/diff/29679590/background.js#newco... background.js:344: if (params.filterError) On 2018/01/29 11:44:48, saroyanm wrote: > Suggestion: copy pasting with the slight modification of current method -> > https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/filterValidation.js... not sure I understand this one. What do you mean? > Can help us making better test, ex.: Testing 5+ and less than 5 lines. what would be the best way for doing that? the amount of errors through the queryString? https://codereview.adblockplus.org/29674584/diff/29679590/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29674584/diff/29679590/messageResponder.js... messageResponder.js:275: errors.push({ On 2018/01/29 11:44:48, saroyanm wrote: > Detail: Why not just pass "error" Object ? > errors.push(error); I've missed this: because it comes from a class unknown to the outer world. This was actually Thomas hint on how this error should've passed and I agree it's cleaner this way.
Published the meeting notes. And a question to @Thomas https://codereview.adblockplus.org/29674584/diff/29679590/background.js File background.js (right): https://codereview.adblockplus.org/29674584/diff/29679590/background.js#newco... background.js:344: if (params.filterError) On 2018/01/29 17:36:32, a.giammarchi wrote: > On 2018/01/29 11:44:48, saroyanm wrote: > > Suggestion: copy pasting with the slight modification of current method -> > > > https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/filterValidation.js... > > not sure I understand this one. What do you mean? > > > > Can help us making better test, ex.: Testing 5+ and less than 5 lines. > > what would be the best way for doing that? the amount of errors through the > queryString? > We will try to use part of filterValidation in order to validate the filters and we might get rid of "params.filterError". https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29679590/desktop-options.js#... desktop-options.js:666: const lines = filtersValue.split(/\n+|\n\s+\n/); On 2018/01/29 15:01:09, a.giammarchi wrote: > On 2018/01/29 14:30:49, saroyanm wrote: > > On 2018/01/29 13:09:02, a.giammarchi wrote: > > > On 2018/01/29 11:44:48, saroyanm wrote: > > > > Splitting same as it's done in the filterValidation > > > > > > > > > > (https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/filterValidation.js...) > > > > should be enough -> split("\n"); > > > > > > I've tested this before writing the RegExp, their logic keeps going through > > > empty lines so it's definitively not the same. This split works better so I > > > don't think I should change it. > > I don't understand what you mean by "Logic keeps going through the lines". > > In order to match implementation in backEnd this should be consistent until we > > have the logic implemented there. > > > > Can you please let me know with which exact input have you tested ? > > if you have a textarea with > > a > b > > c > > > d > > the filter you pointed at me will loop through all lines. If d is an error the > lineno will be 4. > Accordingly, using just \n won't be consistent. The filter is fault tolerant > with new lines, it doesn't bother. > Neither I do, including possible spaces or multiple new lines. > > As summary, splitting by \n only has possible cases not covered (multiple \n\n) > while current RegExp covers that too without any possible harm to the software, > actually granting more consistency with backend. This doesn't work on my(Manvel) test enviroment, Andrea will recheck and we will come back to this again. My point (Manvel's) - that implementation should match the one on the backend. https://codereview.adblockplus.org/29674584/diff/29679590/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29674584/diff/29679590/messageResponder.js... messageResponder.js:275: errors.push({ On 2018/01/29 17:36:32, a.giammarchi wrote: > On 2018/01/29 11:44:48, saroyanm wrote: > > Detail: Why not just pass "error" Object ? > > errors.push(error); > > I've missed this: because it comes from a class unknown to the outer world. This > was actually Thomas hint on how this error should've passed and I agree it's > cleaner this way. @Thomas, can you please let me know what is the reason of not passing actual error Object ? Because from what I can see it's the only place where we are exctracting data out of the object rather than passing the actual object. https://codereview.adblockplus.org/29674584/diff/29684610/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29684610/desktop-options.js#... desktop-options.js:675: editFilters.appendChild(li).textContent = message; The Error keep being added to the error container.
https://codereview.adblockplus.org/29674584/diff/29679590/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29674584/diff/29679590/messageResponder.js... messageResponder.js:275: errors.push({ On 2018/01/30 12:03:44, saroyanm wrote: > @Thomas, can you please let me know what is the reason of not passing actual > error Object ? We can only pass plain objects using the messaging API: "A message can contain any valid JSON object (null, boolean, number, string, array, or object)." Source: https://developer.chrome.com/apps/messaging "An object that can be serialized to JSON." Source: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/sendMes... Alternatively, we may be able to extend the errors to expose a `toJSON()` method but it's not clear whether Chrome uses it and it's probably best not to rely on implementation details because it might differ for other browsers. > Because from what I can see it's the only place where we are > exctracting data out of the object rather than passing the actual object. We're also converting `Subscription` and `Filter` objects (see https://hg.adblockplus.org/adblockplusui/file/tip/messageResponder.js#l54). @Andrea If you want you should be able to reuse the `convertObject()` function I linked to above for converting errors.
On 2018/01/30 13:08:59, Thomas Greiner wrote: > @Andrea If you want you should be able to reuse the `convertObject()` function I > linked to above for converting errors. I like that !
removed the need for filterError=true, added better filter validation mock (write bad CSS to make it fail) using convertObject(error) instead of the manual addressed properties approach.
Comments are ready. Thanks Andrea for tackling the Mock implementation, that makes testing much more easier. I think we have a room for improvement here(specs wise), so I suggested a change. https://codereview.adblockplus.org/29674584/diff/29684628/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29674584/diff/29684628/desktop-options.htm... desktop-options.html:308: <div id="custom-filters-edit-error" class="i18n_options_customFilters_edit_error"></div> Suggestion: In order to keep simplicity, enhance semantics and have better accessibility I think we should have here something along the lines: <div class="side-controls"> ... </div> <div role="alert" aria-hidden="true"> <heading></heading> <ul></ul> </div> I do understand that the way this is defined in the specs the implementation is becoming tricky and keeping it simple as well. So if you agree with me I'll suggest this options: Align with Jeen and move the error message below buttons in the specs, so we can keep the implementation simple. This will help us to simplify the CSS implementation as well. (Sorry that I didn't notice this earlier) https://codereview.adblockplus.org/29674584/diff/29684628/desktop-options.js File desktop-options.js (left): https://codereview.adblockplus.org/29674584/diff/29684628/desktop-options.js#... desktop-options.js:1162: (filters) => Why was this change necessary ? https://codereview.adblockplus.org/29674584/diff/29684628/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29684628/desktop-options.js#... desktop-options.js:666: editFilters.textContent = ""; I think the "many" class should also be removed here, same as it's done when we are switching to the read mode. In order to avoid forgetting details about resetting the "custom-filters-error" I'll suggest to do that in a separate function. https://codereview.adblockplus.org/29674584/diff/29684628/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29674584/diff/29684628/messageResponder.js... messageResponder.js:273: errors.push(convertObject(error)); convertObject method accepts Keys as first parameter. https://codereview.adblockplus.org/29674584/diff/29684628/skin/desktop-option... File skin/desktop-options.css (left): https://codereview.adblockplus.org/29674584/diff/29684628/skin/desktop-option... skin/desktop-options.css:117: */ Nit: I think it's your IDE making this changes, as we didn't agree yet on how we are going to proceed with the comments, let's keep the way it was before.
updated - still missing: errors under buttons https://codereview.adblockplus.org/29674584/diff/29684628/desktop-options.js File desktop-options.js (left): https://codereview.adblockplus.org/29674584/diff/29684628/desktop-options.js#... desktop-options.js:1162: (filters) => On 2018/02/02 12:30:58, saroyanm wrote: > Why was this change necessary ? I guess it was just regular maintenance duty. I'll put as it was but I don't usually wrap same signature for no reason. Curious to know why would you.
https://codereview.adblockplus.org/29674584/diff/29684628/desktop-options.js File desktop-options.js (left): https://codereview.adblockplus.org/29674584/diff/29684628/desktop-options.js#... desktop-options.js:1162: (filters) => On 2018/02/02 13:09:53, a.giammarchi wrote: > On 2018/02/02 12:30:58, saroyanm wrote: > > Why was this change necessary ? > > I guess it was just regular maintenance duty. I'll put as it was but I don't > usually wrap same signature for no reason. Curious to know why would you. No no need, to put it back. Nit: Please just don't move the callback function to the next line, please keep it consistent with other similar implementation: browser.runtime.sendMessage({ type: "filters.get", subscriptionUrl: subscription.url }, loadCustomFilters);
On 2018/02/02 13:44:26, saroyanm wrote: > https://codereview.adblockplus.org/29674584/diff/29684628/desktop-options.js > File desktop-options.js (left): > > https://codereview.adblockplus.org/29674584/diff/29684628/desktop-options.js#... > desktop-options.js:1162: (filters) => > On 2018/02/02 13:09:53, a.giammarchi wrote: > > On 2018/02/02 12:30:58, saroyanm wrote: > > > Why was this change necessary ? > > > > I guess it was just regular maintenance duty. I'll put as it was but I don't > > usually wrap same signature for no reason. Curious to know why would you. > > No no need, to put it back. > Nit: Please just don't move the callback function to the next line, please keep > it consistent with other similar implementation: > browser.runtime.sendMessage({ > type: "filters.get", > subscriptionUrl: subscription.url > }, loadCustomFilters); I don't do anything, my IDE auto-format for me ... I'll try to set it up to not bother in these cases
Almost ready. mainly details and nits. https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.htm... desktop-options.html:303: <a class="i18n_options_customFilters_learn" id="link-filters-2" target="_blank"></a> Suggestion: What about specifying a class or data-attribute, in order not to manage ID number orders ? https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.htm... desktop-options.html:308: <div id="custom-filters-edit-error" class="i18n_options_customFilters_edit_error"></div> Nit: You call this element "custom-filters-edit-error" and the list item "custom-filters-error". I don't understand why the one called "edit" when the other one is not, even oth only appear in edit mode. So I'll suggest to rename this to something more descriptive. ex.: custom-filters-error-message https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.htm... desktop-options.html:318: <ul id="custom-filters-error"></ul> While we don't have yet plan simplifying the semantics, we can improve the accessibility. Suggestion#2: <ul aria-labelledby="custom-filters-edit-error" role="alert" id="custom-filters-error"></ul> This will help assisting technologies, to specify the relationship between the error message and filters and inform the user when the issue will appear also this will help us meet WCAG2 spec requirement. https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.js#... desktop-options.js:664: E("custom-filters").classList.add("warning"); Detail: You can avoid duplication by moving this also into "clearAndGetCustomFiltersError" https://codereview.adblockplus.org/29674584/diff/29687636/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29674584/diff/29687636/skin/desktop-option... skin/desktop-options.css:664: Nit: I think this is again an IDE auto-correction. https://codereview.adblockplus.org/29674584/diff/29687636/skin/desktop-option... skin/desktop-options.css:1414: margin-right: 1rem; Why was this change necessary ? We already had end margin specified.
done some change https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.htm... desktop-options.html:303: <a class="i18n_options_customFilters_learn" id="link-filters-2" target="_blank"></a> On 2018/02/02 16:57:19, saroyanm wrote: > Suggestion: What about specifying a class or data-attribute, in order not to > manage ID number orders ? only reason I've used ID is that we use this E(...) function quite a lot to setup languages so I've thought it was somehow a best practice to use IDs (performance, aria-labelleedby, etc) so, is ist OK if I just use a class instead of the following? E("link-filters-1").setAttribute("href", link); E("link-filters-2").setAttribute("href", link); https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.htm... desktop-options.html:308: <div id="custom-filters-edit-error" class="i18n_options_customFilters_edit_error"></div> On 2018/02/02 16:57:19, saroyanm wrote: > Nit: You call this element "custom-filters-edit-error" and the list item > "custom-filters-error". > > I don't understand why the one called "edit" when the other one is not, even oth > only appear in edit mode. > > So I'll suggest to rename this to something more descriptive. ex.: > custom-filters-error-message I think this is very subjective. This is an error about editing within the edit area so I prefer current id. https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.htm... desktop-options.html:318: <ul id="custom-filters-error"></ul> On 2018/02/02 16:57:20, saroyanm wrote: > While we don't have yet plan simplifying the semantics, we can improve the > accessibility. > Suggestion#2: > <ul aria-labelledby="custom-filters-edit-error" role="alert" > id="custom-filters-error"></ul> > > This will help assisting technologies, to specify the relationship between the > error message and filters and inform the user when the issue will appear also > this will help us meet WCAG2 spec requirement. I like that, will do. https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.js#... desktop-options.js:664: E("custom-filters").classList.add("warning"); On 2018/02/02 16:57:20, saroyanm wrote: > Detail: You can avoid duplication by moving this also into > "clearAndGetCustomFiltersError" there's no duplication? this is .add the other one is .remove. Putting this inside clearAndGetCustomFiltersError function means the order of adding the class warning is now coupled with another element, the vustomFiltersEditor. I'll do that, but I think it's a bit of a dirty approach. https://codereview.adblockplus.org/29674584/diff/29687636/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29674584/diff/29687636/skin/desktop-option... skin/desktop-options.css:1414: margin-right: 1rem; On 2018/02/02 16:57:20, saroyanm wrote: > Why was this change necessary ? > We already had end margin specified. We cannot use margin-end because it's not compatible with Edge. This part was not covered by the other ticket that will remove following margin-end from this file. I'm fixing issues knowing already there will be one (tested on Edge)
Replied to the comments. https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.htm... desktop-options.html:303: <a class="i18n_options_customFilters_learn" id="link-filters-2" target="_blank"></a> On 2018/02/02 17:22:00, a.giammarchi wrote: > On 2018/02/02 16:57:19, saroyanm wrote: > > Suggestion: What about specifying a class or data-attribute, in order not to > > manage ID number orders ? > > only reason I've used ID is that we use this E(...) function quite a lot to > setup languages so I've thought it was somehow a best practice to use IDs > (performance, aria-labelleedby, etc) > > so, is ist OK if I just use a class instead of the following? > E("link-filters-1").setAttribute("href", link); > E("link-filters-2").setAttribute("href", link); We are using E only for not writing document.getElementById or "document.queryselector("ID")", so feel free to use whatever you think makes most sense here. We also planing to simplify documentation link with issue #6114. https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.htm... desktop-options.html:308: <div id="custom-filters-edit-error" class="i18n_options_customFilters_edit_error"></div> On 2018/02/02 17:22:00, a.giammarchi wrote: > On 2018/02/02 16:57:19, saroyanm wrote: > > Nit: You call this element "custom-filters-edit-error" and the list item > > "custom-filters-error". > > > > I don't understand why the one called "edit" when the other one is not, even > oth > > only appear in edit mode. > > > > So I'll suggest to rename this to something more descriptive. ex.: > > custom-filters-error-message > > I think this is very subjective. This is an error about editing within the edit > area so I prefer current id. I see. In general edit-area IMO sounds bit ambiguous in this case, because textarea can also be considered as an edit-area. Currently we use "controls" (ex.: ".side-controls"), to define the form control elements container. Ideally we would use same styling to be consistent. Couple of suggestions for "custom-filters-edit-area": * .control * #custom-filters-control https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29674584/diff/29687636/desktop-options.js#... desktop-options.js:664: E("custom-filters").classList.add("warning"); On 2018/02/02 17:22:00, a.giammarchi wrote: > On 2018/02/02 16:57:20, saroyanm wrote: > > Detail: You can avoid duplication by moving this also into > > "clearAndGetCustomFiltersError" > > there's no duplication? this is .add the other one is .remove. Putting this > inside clearAndGetCustomFiltersError function means the order of adding the > class warning is now coupled with another element, the vustomFiltersEditor. > > I'll do that, but I think it's a bit of a dirty approach. Sorry, I missed that. I agree with you that it's not a duplication. https://codereview.adblockplus.org/29674584/diff/29687636/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29674584/diff/29687636/skin/desktop-option... skin/desktop-options.css:1414: margin-right: 1rem; On 2018/02/02 17:22:00, a.giammarchi wrote: > On 2018/02/02 16:57:20, saroyanm wrote: > > Why was this change necessary ? > > We already had end margin specified. > > We cannot use margin-end because it's not compatible with Edge. > This part was not covered by the other ticket that will remove following > margin-end from this file. > > I'm fixing issues knowing already there will be one (tested on Edge) Right, we have created issue #5813 to tackle that. The problem with tackling it here, we will need to be sure that we consider RTL languages, so this will probably require having a hack like: html[dir="rtl"] #social ul li { margin-left: 1rem; } and also removal of margin-end. While we need to tackle that not only for social media buttons, but we are using margin-end and padding-end in a lot of places, I'll suggest to leave it for #5813.
applied all required changes. I hope this is good now. Since AFAIK this is also blocked by the release due language changes I'll put this on hold waiting for the right time to merge. Thanks for reviewing.
applied all required changes. I hope this is good now. Since AFAIK this is also blocked by the release due language changes I'll put this on hold waiting for the right time to merge. Thanks for reviewing. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
