Issue 29373665: issue 4264 - centralize action handling (Closed)
|
Created:
Jan. 27, 2017, 11:39 a.m. by saroyanm Modified:
May 19, 2017, 10:34 a.m. Reviewers:
Thomas Greiner Visibility:
Public. |
Descriptionissue 4264 - centralize action handling
Patch Set 1 : Thomas implementation #
Total comments: 1
Patch Set 2 : Rollback to the view tab if user filters are not loaded #
Total comments: 12
Patch Set 3 : Show custom filters view not edit on refresh #
Total comments: 3
Patch Set 4 : Rebase #Patch Set 5 : Added error log #Patch Set 6 : Rebased to changeset #108 #Patch Set 7 : Fixed ESLint conflicts #
Total comments: 4
Patch Set 8 : Moved key handling into onKeyUp #MessagesTotal messages: 17
Ready for review.
https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js#newc... new-options.js:681: switchTab("advanced-allFilterLists"); This appears to be a loop that only resolves when `isCustomFiltersLoaded` is `true`: onHashChange -> selectTabItem -> switchTab -> (next item in event loop) -> onHashChange -> ... Therefore this could turn into an infinite loop in case of issues with loading custom filters so it'd be great if we could stick to the current approach of updating the UI by reacting to events. But apart from that, is there a reason why this check even exists? Just because there might not be any custom filters to look at, doesn't mean that the tab doesn't provide any value. https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js#newc... new-options.js:709: if (tabId == "advanced-customFilters") What about making this more generic to declare the behavior in the HTML: It's a dependency that needs to be resolved before the tab should be shown. For instance, I could imagine "data-dependency='edit-custom-filters'" and handle it like this: if (tab.hasAttribute("data-dependency")) execAction(tab.getAttribute("data-dependency");
https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js#newc... new-options.js:681: switchTab("advanced-allFilterLists"); On 2017/01/27 16:04:33, Thomas Greiner wrote: > This appears to be a loop that only resolves when `isCustomFiltersLoaded` is > `true`: > onHashChange -> selectTabItem -> switchTab -> (next item in event loop) -> > onHashChange -> ... It's not a loop, switch from one tab to another if the one not ready to be load. > Therefore this could turn into an infinite loop in case of issues with loading > custom filters so it'd be great if we could stick to the current approach of > updating the UI by reacting to events. It shouldn't turn into infinite loop if there is error with page. > But apart from that, is there a reason why this check even exists? Just because > there might not be any custom filters to look at, doesn't mean that the tab > doesn't provide any value. We might reconsider the way we are showing the "Edit" tab while we currently using the implementation in current options page, which is tricky and doesn't include update of textarea when new filter is added. I think it's safer this way to switch tab if the filters are not loaded and we can fix the general issue with the texarea not being populated dynamical in separate issue. https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js#newc... new-options.js:709: if (tabId == "advanced-customFilters") On 2017/01/27 16:04:33, Thomas Greiner wrote: > What about making this more generic to declare the behavior in the HTML: It's a > dependency that needs to be resolved before the tab should be shown. > > For instance, I could imagine "data-dependency='edit-custom-filters'" and handle > it like this: > > if (tab.hasAttribute("data-dependency")) > execAction(tab.getAttribute("data-dependency"); I'll investigate that and provide in new patch, I agree that dependency should make more sense here.
https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js#newc... new-options.js:681: switchTab("advanced-allFilterLists"); On 2017/01/27 16:13:44, saroyanm wrote: > It's not a loop, switch from one tab to another if the one not ready to be load. You're right. I must've misread that as "advanced-allFilterlists" when I wrote the first part of the comment. > We might reconsider the way we are showing the "Edit" tab while we currently > using the implementation in current options page, which is tricky and doesn't > include update of textarea when new filter is added. I think it's safer this way > to switch tab if the filters are not loaded and we can fix the general issue > with the texarea not being populated dynamical in separate issue. It might be safer but it's a terrible user experience because: a) Clicking on the tab doesn't switch to it but you stay on the "advanced-allFilterlists" tab. b) Deep-linking to the "advanced-customFilters" tab will show the "advanced-allFilterlists" tab. This will cause confusion used by instructions that may refer to UI elements within that tab. Therefore I'd suggest to stick to the implementation that's in the current options page and not update the text area, only the list of filters.
https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js#newc... new-options.js:681: switchTab("advanced-allFilterLists"); On 2017/01/27 16:39:27, Thomas Greiner wrote: > On 2017/01/27 16:13:44, saroyanm wrote: > > It's not a loop, switch from one tab to another if the one not ready to be > load. > > You're right. I must've misread that as "advanced-allFilterlists" when I wrote > the first part of the comment. > > > We might reconsider the way we are showing the "Edit" tab while we currently > > using the implementation in current options page, which is tricky and doesn't > > include update of textarea when new filter is added. I think it's safer this > way > > to switch tab if the filters are not loaded and we can fix the general issue > > with the texarea not being populated dynamical in separate issue. > > It might be safer but it's a terrible user experience because: > > a) Clicking on the tab doesn't switch to it but you stay on the > "advanced-allFilterlists" tab. > b) Deep-linking to the "advanced-customFilters" tab will show the > "advanced-allFilterlists" tab. This will cause confusion used by instructions > that may refer to UI elements within that tab. > > Therefore I'd suggest to stick to the implementation that's in the current > options page and not update the text area, only the list of filters. I'll strongly disagree sticking to the current option page implementation while using deeplinking to the edit mode, because if we will allow user to switch to the edit mode while the filters are not loaded completely, click on save button might lead to the loose of the existing custom filter lists. So I can see two possibilities: 1) Do not allow deeplinking to the edit mode. 2) Only switch to edit mode if the custom filter lists are populated.
Also please note the lack of capitalization in the review title in case it reflects the commit title. https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js#newc... new-options.js:681: switchTab("advanced-allFilterLists"); On 2017/01/27 16:56:41, saroyanm wrote: > I'll strongly disagree sticking to the current option page implementation while > using deeplinking to the edit mode, because if we will allow user to switch to > the edit mode while the filters are not loaded completely, click on save button > might lead to the loose of the existing custom filter lists. > So I can see two possibilities: > 1) Do not allow deeplinking to the edit mode. > 2) Only switch to edit mode if the custom filter lists are populated. "advanced-customFilters" is not a deep-link to the edit mode. I assume that's where the confusion comes from. The text area should not be updated whereas the list of custom filters should, right? In that case maybe it'd be best to prevent the user from accessing edit mode until custom filters have finished loading.
https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js#newc... new-options.js:681: switchTab("advanced-allFilterLists"); On 2017/01/30 11:57:53, Thomas Greiner wrote: > On 2017/01/27 16:56:41, saroyanm wrote: > > I'll strongly disagree sticking to the current option page implementation > while > > using deeplinking to the edit mode, because if we will allow user to switch to > > the edit mode while the filters are not loaded completely, click on save > button > > might lead to the loose of the existing custom filter lists. > > So I can see two possibilities: > > 1) Do not allow deeplinking to the edit mode. > > 2) Only switch to edit mode if the custom filter lists are populated. > > "advanced-customFilters" is not a deep-link to the edit mode. I assume that's Probably, I was refering to: /new-options.html#advanced-customFilters > where the confusion comes from. Maybe I'm wrong > The text area should not be updated whereas the > list of custom filters should, right? > > In that case maybe it'd be best to prevent the user from accessing edit mode > until custom filters have finished loading. Does that mean that you are fine with current approach ?
https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js#newc... new-options.js:681: switchTab("advanced-allFilterLists"); On 2017/02/01 12:28:10, saroyanm wrote: > > where the confusion comes from. > Maybe I'm wrong > > The text area should not be updated whereas the > > list of custom filters should, right? > > > > In that case maybe it'd be best to prevent the user from accessing edit mode > > until custom filters have finished loading. > Does that mean that you are fine with current approach ? The "advanced-customFilters" can be accessed either in "view" or "edit" mode (see `editCustomFilters()`). While you should still be able to view the custom filters while they're loading, you shouldn't be able to edit them. Your current approach, however, does not allow the user to even view them.
https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js#newc... new-options.js:681: switchTab("advanced-allFilterLists"); On 2017/02/02 12:24:50, Thomas Greiner wrote: > On 2017/02/01 12:28:10, saroyanm wrote: > > > where the confusion comes from. > > Maybe I'm wrong > > > The text area should not be updated whereas the > > > list of custom filters should, right? > > > > > > In that case maybe it'd be best to prevent the user from accessing edit mode > > > until custom filters have finished loading. > > Does that mean that you are fine with current approach ? > > The "advanced-customFilters" can be accessed either in "view" or "edit" mode > (see `editCustomFilters()`). While you should still be able to view the custom > filters while they're loading, you shouldn't be able to edit them. > > Your current approach, however, does not allow the user to even view them. Clear, agree we should definately show the custom filters view tab instead of full filterlists tab, will update with the next patch
https://codereview.adblockplus.org/29373665/diff/29373666/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29373666/new-options.js#newc... new-options.js:730: case "advanced-customFilters": I think we shouldn't implement this change, so I removed it, considering our discussion in 2-nd patch. https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js#newc... new-options.js:709: if (tabId == "advanced-customFilters") On 2017/01/27 16:13:44, saroyanm wrote: > On 2017/01/27 16:04:33, Thomas Greiner wrote: > > What about making this more generic to declare the behavior in the HTML: It's > a > > dependency that needs to be resolved before the tab should be shown. > > > > For instance, I could imagine "data-dependency='edit-custom-filters'" and > handle > > it like this: > > > > if (tab.hasAttribute("data-dependency")) > > execAction(tab.getAttribute("data-dependency"); > > I'll investigate that and provide in new patch, I agree that dependency should > make more sense here. This is not relevant anymore while we are showing view of custom filters in new patch and also we can use data-action="switch-tab,edit-custom-filters" currently in HTML. https://codereview.adblockplus.org/29373665/diff/29374825/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29374825/new-options.js#newc... new-options.js:947: if (!isCustomFiltersLoaded) Theoretically user will not need this check, but technically it's still possible that the edit view might be triggered before the filters are loaded.
https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29373671/new-options.js#newc... new-options.js:709: if (tabId == "advanced-customFilters") On 2017/02/08 12:14:00, saroyanm wrote: > On 2017/01/27 16:13:44, saroyanm wrote: > > On 2017/01/27 16:04:33, Thomas Greiner wrote: > > > What about making this more generic to declare the behavior in the HTML: > It's > > a > > > dependency that needs to be resolved before the tab should be shown. > > > > > > For instance, I could imagine "data-dependency='edit-custom-filters'" and > > handle > > > it like this: > > > > > > if (tab.hasAttribute("data-dependency")) > > > execAction(tab.getAttribute("data-dependency"); > > > > I'll investigate that and provide in new patch, I agree that dependency should > > make more sense here. > > This is not relevant anymore while we are showing view of custom filters in new > patch and also we can use data-action="switch-tab,edit-custom-filters" currently > in HTML. You're right. Using "data-action" is more appropriate in this case but you removed it in your first patch. https://codereview.adblockplus.org/29373665/diff/29374825/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29374825/new-options.js#newc... new-options.js:947: if (!isCustomFiltersLoaded) On 2017/02/08 12:14:01, saroyanm wrote: > Theoretically user will not need this check, but technically it's still possible > that the edit view might be triggered before the filters are loaded. Nevertheless it'd be good to use `console.error()` to indicate that we've encountered some unexpected behavior. Note that it's still unclear how we should communicate this to the user.
New patch uploaded https://codereview.adblockplus.org/29373665/diff/29374825/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29374825/new-options.js#newc... new-options.js:947: if (!isCustomFiltersLoaded) On 2017/03/02 15:10:13, Thomas Greiner wrote: > On 2017/02/08 12:14:01, saroyanm wrote: > > Theoretically user will not need this check, but technically it's still > possible > > that the edit view might be triggered before the filters are loaded. > > Nevertheless it'd be good to use `console.error()` to indicate that we've > encountered some unexpected behavior. Agree, done > Note that it's still unclear how we should communicate this to the user. I think we should revisit all console.errors we will have as soon we will implement user notifications in options page.
The code is rebased and ESLint conflicts resolved. Ready for the review.
https://codereview.adblockplus.org/29373665/diff/29422642/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29422642/new-options.js#newc... new-options.js:477: function execAction(action, key, element) `key` is only used for keyboard-initiated actions so let's keep them in `onKeyUp()`. Also note that it's only used for "switch-tabs" and there `key` is merely determining the value of `element` so moving it out of here shouldn't be an issue. https://codereview.adblockplus.org/29373665/diff/29422642/new-options.js#newc... new-options.js:597: if (element.getAttribute("aria-checked") == "true") Did you verify that this still works? The reason why I'm asking is thta this is the only place where we're referring to `element` directly rather than just for using it for `findParentData()`. Because before `element` had the value `findParentData(e.target, "action", true)` whereas now it has the value `e.target` so it might be a different element than before.
https://codereview.adblockplus.org/29373665/diff/29422642/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29373665/diff/29422642/new-options.js#newc... new-options.js:477: function execAction(action, key, element) On 2017/05/05 14:33:05, Thomas Greiner wrote: > `key` is only used for keyboard-initiated actions so let's keep them in > `onKeyUp()`. > > Also note that it's only used for "switch-tabs" and there `key` is merely > determining the value of `element` so moving it out of here shouldn't be an > issue. Done. https://codereview.adblockplus.org/29373665/diff/29422642/new-options.js#newc... new-options.js:597: if (element.getAttribute("aria-checked") == "true") On 2017/05/05 14:33:05, Thomas Greiner wrote: > Did you verify that this still works? The reason why I'm asking is thta this is > the only place where we're referring to `element` directly rather than just for > using it for `findParentData()`. Because before `element` had the value > `findParentData(e.target, "action", true)` whereas now it has the value > `e.target` so it might be a different element than before. It does work, until we introduce child element to the "Checkbox button".
Ready to review
LGTM 
|
