Issue 29332808: Issue 2408 - Improved accessibility of checkboxes in options page (Closed)
|
Created:
Dec. 16, 2015, 1:31 p.m. by Thomas Greiner Modified:
Jan. 27, 2016, 2:51 p.m. Reviewers:
saroyanm Visibility:
Public. |
DescriptionNote that apart from the changes for the ticket, this review also introduces `Collection.prototype.updateItem` to centralize item updates.
Patch Set 1 #Patch Set 2 : Rebased to 59920e6112a6 #
Total comments: 17
Patch Set 3 : #
Total comments: 9
Patch Set 4 : Reverted styles for Advanced tab #
MessagesTotal messages: 6
Sorry that the review took so long. https://codereview.adblockplus.org/29332808/diff/29332870/options.html File options.html (right): https://codereview.adblockplus.org/29332808/diff/29332870/options.html#newcode74 options.html:74: <div id="tab-content"> Detail: I think this can become tabs-content for readability of the file, while we have introduced tab-content classes. https://codereview.adblockplus.org/29332808/diff/29332870/options.html#newcod... options.html:233: <li> This will be changed with blocking list implementation, so I think doesn't make sense to introduce this change here for readability. https://codereview.adblockplus.org/29332808/diff/29332870/options.js File options.js (right): https://codereview.adblockplus.org/29332808/diff/29332870/options.js#newcode73 options.js:73: var label = listItem.querySelector(".display"); Nit: we can have two lines merged: listItem.querySelector(".display").setAttribute("id", labelId); https://codereview.adblockplus.org/29332808/diff/29332870/options.js#newcode115 options.js:115: while (true) What about ? while (tab.tagName == "BODY") ? In that case you can remove "if (!tab)" part https://codereview.adblockplus.org/29332808/diff/29332870/options.js#newcode137 options.js:137: Collection.prototype.updateItem = function(item) I think it make sense to have second optional argument where we can send the listItem. In that case we will not need to query for the element when called from "addItems" method, this should reduce the query of the listItem which we already have. ex.: updateItem(subscription, listElement) https://codereview.adblockplus.org/29332808/diff/29332870/options.js#newcode172 options.js:172: var focusables = container.querySelectorAll("a, button, .control"); What about have a focus for input elements as well ? I can see the only place we will need it is in "Add other website language" section, which is unlikely that user will add all languages, so it's not really priority of having that change, but for consistency reasons we can have it IMO. https://codereview.adblockplus.org/29332808/diff/29332870/options.js#newcode175 options.js:175: if (index + 1 < focusables.length) I think we can write this one line, something like: index == focusables.length - 1 ? index-- : index++; https://codereview.adblockplus.org/29332808/diff/29332870/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29332808/diff/29332870/skin/options.css#ne... skin/options.css:103: padding: 0px 0px 0px 0px; Nit: Use single value. I know it's something that we already had, but I think we can also fix it here.
https://codereview.adblockplus.org/29332808/diff/29332870/options.html File options.html (right): https://codereview.adblockplus.org/29332808/diff/29332870/options.html#newcode74 options.html:74: <div id="tab-content"> On 2016/01/19 11:19:49, saroyanm wrote: > Detail: I think this can become tabs-content for readability of the file, while > we have introduced tab-content classes. Done. For consistency I chose to go with "content" since its child elements follow the pattern "content-*". https://codereview.adblockplus.org/29332808/diff/29332870/options.html#newcod... options.html:233: <li> On 2016/01/19 11:19:49, saroyanm wrote: > This will be changed with blocking list implementation, so I think doesn't make > sense to introduce this change here for readability. Done. https://codereview.adblockplus.org/29332808/diff/29332870/options.js File options.js (right): https://codereview.adblockplus.org/29332808/diff/29332870/options.js#newcode73 options.js:73: var label = listItem.querySelector(".display"); On 2016/01/19 11:19:50, saroyanm wrote: > Nit: > we can have two lines merged: > listItem.querySelector(".display").setAttribute("id", labelId); Done. https://codereview.adblockplus.org/29332808/diff/29332870/options.js#newcode115 options.js:115: while (true) On 2016/01/19 11:19:49, saroyanm wrote: > What about ? > while (tab.tagName == "BODY") ? > > In that case you can remove "if (!tab)" part Done. Checking for the element type should be the last resort but I was able to move the exit condition into the while-statement. https://codereview.adblockplus.org/29332808/diff/29332870/options.js#newcode137 options.js:137: Collection.prototype.updateItem = function(item) On 2016/01/19 11:19:50, saroyanm wrote: > I think it make sense to have second optional argument where we can send the > listItem. > In that case we will not need to query for the element when called from > "addItems" method, this should reduce the query of the listItem which we already > have. > ex.: updateItem(subscription, listElement) But an item can be associated with multiple list elements so updating just one of them will lead to syncing issues. https://codereview.adblockplus.org/29332808/diff/29332870/options.js#newcode172 options.js:172: var focusables = container.querySelectorAll("a, button, .control"); On 2016/01/19 11:19:50, saroyanm wrote: > What about have a focus for input elements as well ? I can see the only place we > will need it is in "Add other website language" section, which is unlikely that > user will add all languages, so it's not really priority of having that change, > but for consistency reasons we can have it IMO. Done. https://codereview.adblockplus.org/29332808/diff/29332870/options.js#newcode175 options.js:175: if (index + 1 < focusables.length) On 2016/01/19 11:19:49, saroyanm wrote: > I think we can write this one line, something like: > index == focusables.length - 1 ? index-- : index++; Done. https://codereview.adblockplus.org/29332808/diff/29332870/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29332808/diff/29332870/skin/options.css#ne... skin/options.css:103: padding: 0px 0px 0px 0px; On 2016/01/19 11:19:50, saroyanm wrote: > Nit: Use single value. I know it's something that we already had, but I think we > can also fix it here. Done.
https://codereview.adblockplus.org/29332808/diff/29332870/options.js File options.js (right): https://codereview.adblockplus.org/29332808/diff/29332870/options.js#newcode115 options.js:115: while (true) On 2016/01/19 15:15:04, Thomas Greiner wrote: > On 2016/01/19 11:19:49, saroyanm wrote: > > What about ? > > while (tab.tagName == "BODY") ? > > > > In that case you can remove "if (!tab)" part > > Done. Checking for the element type should be the last resort but I was able to > move the exit condition into the while-statement. you can also check for the element itself "document.body != tab" Anyway I see you currently went with another approach, which does make more sense. https://codereview.adblockplus.org/29332808/diff/29333987/options.js File options.js (right): https://codereview.adblockplus.org/29332808/diff/29333987/options.js#newcode113 options.js:113: var tab = element.parentElement; Detail: This is not only tab already, so maybe make sense to rename it to something more generic. ex.: focusContainer https://codereview.adblockplus.org/29332808/diff/29333987/options.js#newcode114 options.js:114: while (tab) What if the dialog where the collection is implemented doesn't contain any other focusable elements, should the close button be selected ? https://codereview.adblockplus.org/29332808/diff/29333987/options.js#newcode117 options.js:117: || tab.classList.contains("dialog-content")) I think we need to make it obvious whether it's something related to removeItem() or separate function, because currently feel like we try to fix what focusNextElement do not capable, so I'll suggest to implement whole functionality here without separate function, or make the existing function more complex not to list all the focusablecontainers that are ancestors of current element on current line. Here is how I imagine the function behavior: * receive element * find ancestor that need to be so called "focusContainer" * if "focusContainer" doesn't contain focusable children go up recursively until reached the top. We can also introduce focusableContainer class or data attribute to make this manual trigger more manageable. https://codereview.adblockplus.org/29332808/diff/29333987/skin/options.css File skin/options.css (left): https://codereview.adblockplus.org/29332808/diff/29333987/skin/options.css#ol... skin/options.css:403: .table input[type="checkbox"] I think this style changes also belong to blocking list implementation and I think it doesn't make sense to introduce them here, same applies to style removal below.
https://codereview.adblockplus.org/29332808/diff/29333987/options.js File options.js (right): https://codereview.adblockplus.org/29332808/diff/29333987/options.js#newcode113 options.js:113: var tab = element.parentElement; On 2016/01/25 14:45:43, saroyanm wrote: > Detail: This is not only tab already, so maybe make sense to rename it to > something more generic. ex.: focusContainer Done. https://codereview.adblockplus.org/29332808/diff/29333987/options.js#newcode114 options.js:114: while (tab) On 2016/01/25 14:45:43, saroyanm wrote: > What if the dialog where the collection is implemented doesn't contain any other > focusable elements, should the close button be selected ? The main challenge with focus management is that you can make it as extensive as you want but the more you do that the more complicated it gets. Therefore I decided to go with the most minimal implementation that's necessary for our UI. At the moment there's always a focusable element in the dialog (see "default-focus" class name) which is why this problem only exists in theory. https://codereview.adblockplus.org/29332808/diff/29333987/options.js#newcode117 options.js:117: || tab.classList.contains("dialog-content")) On 2016/01/25 14:45:43, saroyanm wrote: > I think we need to make it obvious whether it's something related to > removeItem() or separate function, because currently feel like we try to fix > what focusNextElement do not capable, so I'll suggest to implement whole > functionality here without separate function We could put this into a separate function that's called by MutationObserver but I wonder whether it's worth setting up an observer for this single case that's only triggered in this exact spot. > or make the existing function more > complex not to list all the focusablecontainers that are ancestors of current > element on current line. > Here is how I imagine the function behavior: > * receive element > * find ancestor that need to be so called "focusContainer" > * if "focusContainer" doesn't contain focusable children go up recursively until > reached the top. > > We can also introduce focusableContainer class or data attribute to make this > manual trigger more manageable. Unfortunately, it's not as easy as recursing up the DOM to the top element because usually, it makes more sense that the focus shifts to a sibling element rather than the parent. That's what I meant with my previous comment when I said that it can get complicated real fast. https://codereview.adblockplus.org/29332808/diff/29333987/skin/options.css File skin/options.css (left): https://codereview.adblockplus.org/29332808/diff/29333987/skin/options.css#ol... skin/options.css:403: .table input[type="checkbox"] On 2016/01/25 14:45:44, saroyanm wrote: > I think this style changes also belong to blocking list implementation and I > think it doesn't make sense to introduce them here, same applies to style > removal below. Done.
LGTM https://codereview.adblockplus.org/29332808/diff/29333987/options.js File options.js (right): https://codereview.adblockplus.org/29332808/diff/29333987/options.js#newcode114 options.js:114: while (tab) On 2016/01/25 18:04:21, Thomas Greiner wrote: > On 2016/01/25 14:45:43, saroyanm wrote: > > What if the dialog where the collection is implemented doesn't contain any > other > > focusable elements, should the close button be selected ? > > The main challenge with focus management is that you can make it as extensive as > you want but the more you do that the more complicated it gets. Therefore I > decided to go with the most minimal implementation that's necessary for our UI. > > At the moment there's always a focusable element in the dialog (see > "default-focus" class name) which is why this problem only exists in theory. Fare enough. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
