Issue 29338983: issue 3741 - Add "remove" option to list items in new options page (Closed)
|
Created:
March 23, 2016, 4:10 p.m. by saroyanm Modified:
June 24, 2016, 5:11 p.m. Reviewers:
Thomas Greiner Visibility:
Public. |
Descriptionissue 3741 - Add "remove" option to list items in new options page
Patch Set 1 : #
Total comments: 7
Patch Set 2 : Rebase to changeset #83 #
Total comments: 6
Patch Set 3 : Impoved accessibility, removed onclick assignments from the collections #
Total comments: 3
Patch Set 4 : Added title attribute to whitelisting and custom list table #Patch Set 5 : Rebaseed to changeset 84 #
Total comments: 10
Patch Set 6 : Translated title attributes #
Total comments: 2
Patch Set 7 : #
Total comments: 2
Patch Set 8 : Removed aria-labelledby #
Total comments: 6
Patch Set 9 : Small fix #Patch Set 10 : Removed label's "for" attribute, added "aria-lable" to addItem #
MessagesTotal messages: 16
Thomas can you please have a look when you have time ? https://codereview.adblockplus.org/29338983/diff/29338996/options.js File options.js (left): https://codereview.adblockplus.org/29338983/diff/29338996/options.js#oldcode363 options.js:363: if (change[i].name == "disabled") This implementation doesn't belong to here as well, we should have handled this when subscription were added or removed, I think currently the implementation should be more understandable. https://codereview.adblockplus.org/29338983/diff/29338996/options.js#oldcode427 options.js:427: { This doesn't have anything to do with updateSubscription() function, this should be done whenever we are adding a new subscription, so I've updated this as well in scope of current review, I think now it's make more sense. https://codereview.adblockplus.org/29338983/diff/29338996/options.js File options.js (right): https://codereview.adblockplus.org/29338983/diff/29338996/options.js#newcode305 options.js:305: onClick: toggleDisableSubscription I think we do not need this listeners being specified on collections, I think we can specify them in HTML data-action="". I can handle that in this in this review or separately, please let me know know what you think.
https://codereview.adblockplus.org/29338983/diff/29338996/options.js File options.js (left): https://codereview.adblockplus.org/29338983/diff/29338996/options.js#oldcode363 options.js:363: if (change[i].name == "disabled") On 2016/03/23 16:34:38, saroyanm wrote: > This implementation doesn't belong to here as well, we should have handled this > when subscription were added or removed, I think currently the implementation > should be more understandable. Please ignore this change as well, as specified below. https://codereview.adblockplus.org/29338983/diff/29338996/options.js#oldcode427 options.js:427: { On 2016/03/23 16:34:38, saroyanm wrote: > This doesn't have anything to do with updateSubscription() function, this should > be done whenever we are adding a new subscription, so I've updated this as well > in scope of current review, I think now it's make more sense. The implementation was changed, I decided to introduce this change and the one above separately in case needed, so please ignore this and comment above, it will make the review quicker and rebasing as well.
https://codereview.adblockplus.org/29338983/diff/29338996/options.js File options.js (right): https://codereview.adblockplus.org/29338983/diff/29338996/options.js#newcode305 options.js:305: onClick: toggleDisableSubscription On 2016/03/23 16:34:38, saroyanm wrote: > I think we do not need this listeners being specified on collections, I think we > can specify them in HTML data-action="". > I can handle that in this in this review or separately, please let me know know > what you think. I'd be fine with either but if it's not too much effort to do it'd be great to include it in this review. https://codereview.adblockplus.org/29338983/diff/29341141/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29338983/diff/29341141/new-options.html#ne... new-options.html:101: <span class="display"></span> What's the reason for changing this to `<span>` here? Seems like it was done by accident since the other `<label>` elements are unchanged. https://codereview.adblockplus.org/29338983/diff/29341141/new-options.html#ne... new-options.html:102: <button data-action="remove-subscription" class="delete control"></button> This button's accessibility needs to be improved since it's unclear to visually impaired users what this button does. At the same time we may also want to clarify the action of the checkbox since its label might not be sufficient anymore due to there being two actions now. So what I could imagine is something along those lines: <li id="item-42" role="section"> <button role="checkbox" class="control" title="Toggle subscription"></button> <label for="item-42">EasyList</label> <button class="control delete" data-action="remove-subscription" title="Remove subscription"></button> </li> If you want, this can be tackled in a separate ticket though. https://codereview.adblockplus.org/29338983/diff/29341141/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29338983/diff/29341141/skin/new-options.cs... skin/new-options.css:416: height: 10px; These dimensions are being overridden by a rule below which causes the "x" buttons to be 8x8 instead of 10x10 which is more accurate to how they're shown in the design mockup. Note that the icons in the sprite need to be adapted too.
https://codereview.adblockplus.org/29338983/diff/29338996/options.js File options.js (right): https://codereview.adblockplus.org/29338983/diff/29338996/options.js#newcode305 options.js:305: onClick: toggleDisableSubscription On 2016/05/23 12:43:25, Thomas Greiner wrote: > On 2016/03/23 16:34:38, saroyanm wrote: > > I think we do not need this listeners being specified on collections, I think > we > > can specify them in HTML data-action="". > > I can handle that in this in this review or separately, please let me know > know > > what you think. > > I'd be fine with either but if it's not too much effort to do it'd be great to > include it in this review. Done. https://codereview.adblockplus.org/29338983/diff/29341141/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29338983/diff/29341141/new-options.html#ne... new-options.html:101: <span class="display"></span> On 2016/05/23 12:43:25, Thomas Greiner wrote: > What's the reason for changing this to `<span>` here? Seems like it was done by > accident since the other `<label>` elements are unchanged. I think, missed this during the rebase, thanks. https://codereview.adblockplus.org/29338983/diff/29341141/new-options.html#ne... new-options.html:102: <button data-action="remove-subscription" class="delete control"></button> On 2016/05/23 12:43:25, Thomas Greiner wrote: > This button's accessibility needs to be improved since it's unclear to visually > impaired users what this button does. At the same time we may also want to > clarify the action of the checkbox since its label might not be sufficient > anymore due to there being two actions now. > > So what I could imagine is something along those lines: > > <li id="item-42" role="section"> > <button role="checkbox" class="control" title="Toggle subscription"></button> > <label for="item-42">EasyList</label> > <button class="control delete" data-action="remove-subscription" title="Remove > subscription"></button> > </li> > > If you want, this can be tackled in a separate ticket though. Updated: I tested the suggestion you've made with the screen recorder, apparently it's still not obvious what element should toggle/remove the buttons, so I've added "aria-labelledby" to both of the elements to point the listElement, but I had to add "label" attribute to the list element as well to make that connection work, anyway I can't understand the value of role element and using for on "label" element. Also the only advantage I can see after testing with current approach that it will be possible to setup label on parent element regardless of the label, so for example we can have "Filter list: German". https://codereview.adblockplus.org/29338983/diff/29341141/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29338983/diff/29341141/skin/new-options.cs... skin/new-options.css:416: height: 10px; On 2016/05/23 12:43:26, Thomas Greiner wrote: > These dimensions are being overridden by a rule below which causes the "x" > buttons to be 8x8 instead of 10x10 which is more accurate to how they're shown > in the design mockup. > > Note that the icons in the sprite need to be adapted too. Done. https://codereview.adblockplus.org/29338983/diff/29345651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29345651/new-options.js#newc... new-options.js:184: if (title) I think we should only specify the "data-search" attribute only for "all-lang-table" table, I'm not sure why we have this check here, seems it doesn't provide any value.
https://codereview.adblockplus.org/29338983/diff/29345651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29345651/new-options.js#newc... new-options.js:184: if (title) On 2016/06/08 15:21:07, saroyanm wrote: > I think we should only specify the "data-search" attribute only for > "all-lang-table" table, I'm not sure why we have this check here, seems it > doesn't provide any value. Ok, feel free to change it so that it's only added to items in "all-lang-table". https://codereview.adblockplus.org/29338983/diff/29345746/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29338983/diff/29345746/new-options.html#ne... new-options.html:92: <button data-action="toggle-disable-subscription" role="checkbox" class="control" title="toggle"></button> The titles need to be translatable as well. https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js#newc... new-options.js:104: var ItemId = "item-" + (++maxItemId); Detail: Variable names should start with a lower-case letter. https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js#newc... new-options.js:188: element.setAttribute("label", title); What's the "label" attribute supposed to do?
https://codereview.adblockplus.org/29338983/diff/29345651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29345651/new-options.js#newc... new-options.js:184: if (title) On 2016/06/16 10:42:22, Thomas Greiner wrote: > On 2016/06/08 15:21:07, saroyanm wrote: > > I think we should only specify the "data-search" attribute only for > > "all-lang-table" table, I'm not sure why we have this check here, seems it > > doesn't provide any value. > > Ok, feel free to change it so that it's only added to items in "all-lang-table". Done. https://codereview.adblockplus.org/29338983/diff/29345746/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29338983/diff/29345746/new-options.html#ne... new-options.html:92: <button data-action="toggle-disable-subscription" role="checkbox" class="control" title="toggle"></button> On 2016/06/16 10:42:22, Thomas Greiner wrote: > The titles need to be translatable as well. Done. https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js#newc... new-options.js:104: var ItemId = "item-" + (++maxItemId); On 2016/06/16 10:42:22, Thomas Greiner wrote: > Detail: Variable names should start with a lower-case letter. Done. https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js#newc... new-options.js:188: element.setAttribute("label", title); On 2016/06/16 10:42:22, Thomas Greiner wrote: > What's the "label" attribute supposed to do? I was testing with screen reader and apparently in this case it helps to provide information about the list item, we are using labeldBy for the checkbox, but it's still not obvious for the people what is the item is about, so using label we can provide information about the item to the blind people.
Just some details left. https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js#newc... new-options.js:188: element.setAttribute("label", title); On 2016/06/16 18:22:59, saroyanm wrote: > On 2016/06/16 10:42:22, Thomas Greiner wrote: > > What's the "label" attribute supposed to do? > > I was testing with screen reader and apparently in this case it helps to provide > information about the list item, we are using labeldBy for the checkbox, but > it's still not obvious for the people what is the item is about, so using label > we can provide information about the item to the blind people. Ok, in that case please use the standardized "aria-label" attribute instead (see https://www.w3.org/TR/wai-aria/states_and_properties#aria-label) because I couldn't find any documentation for "label". https://codereview.adblockplus.org/29338983/diff/29346598/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29338983/diff/29346598/locale/en-US/new-op... locale/en-US/new-options.json:39: "description": "Value of title attribute for toggle elements", Detail: Neither the message ID nor the description are specifically stating what the "toggle element" is. What about changing them to: options_control_toggle_title Title of toggle controls Same for "remove element" below.
https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js#newc... new-options.js:188: element.setAttribute("label", title); On 2016/06/17 14:07:50, Thomas Greiner wrote: > On 2016/06/16 18:22:59, saroyanm wrote: > > On 2016/06/16 10:42:22, Thomas Greiner wrote: > > > What's the "label" attribute supposed to do? > > > > I was testing with screen reader and apparently in this case it helps to > provide > > information about the list item, we are using labeldBy for the checkbox, but > > it's still not obvious for the people what is the item is about, so using > label > > we can provide information about the item to the blind people. > > Ok, in that case please use the standardized "aria-label" attribute instead (see > https://www.w3.org/TR/wai-aria/states_and_properties#aria-label) because I > couldn't find any documentation for "label". I had another look and seems like removing this attribute works fine as well, for some reason now I can hear duplication if I keep this attribute, so I think it's better to remove it for now. So your suspicions regarding this label was correct, sorry for that. But I was sure that this was the reason I used the attribute :/ https://codereview.adblockplus.org/29338983/diff/29346598/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29338983/diff/29346598/locale/en-US/new-op... locale/en-US/new-options.json:39: "description": "Value of title attribute for toggle elements", On 2016/06/17 14:07:50, Thomas Greiner wrote: > Detail: Neither the message ID nor the description are specifically stating what > the "toggle element" is. What about changing them to: > > options_control_toggle_title > Title of toggle controls > > Same for "remove element" below. Done.
https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js#newc... new-options.js:188: element.setAttribute("label", title); On 2016/06/21 13:29:56, saroyanm wrote: > On 2016/06/17 14:07:50, Thomas Greiner wrote: > > On 2016/06/16 18:22:59, saroyanm wrote: > > > On 2016/06/16 10:42:22, Thomas Greiner wrote: > > > > What's the "label" attribute supposed to do? > > > > > > I was testing with screen reader and apparently in this case it helps to > > provide > > > information about the list item, we are using labeldBy for the checkbox, but > > > it's still not obvious for the people what is the item is about, so using > > label > > > we can provide information about the item to the blind people. > > > > Ok, in that case please use the standardized "aria-label" attribute instead > (see > > https://www.w3.org/TR/wai-aria/states_and_properties#aria-label) because I > > couldn't find any documentation for "label". > > I had another look and seems like removing this attribute works fine as well, > for some reason now I can hear duplication if I keep this attribute, so I think > it's better to remove it for now. So your suspicions regarding this label was > correct, sorry for that. But I was sure that this was the reason I used the > attribute :/ I could imagine that the duplication might be because when you focus the control, it reads first what section it's in and then its "aria-labelledby" attribute which again refers to the section. In that case it should be sufficient to remove the "aria-labelledby" attribute. https://codereview.adblockplus.org/29338983/diff/29346907/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29346907/new-options.js#newc... new-options.js:125: getMessage(controls[k].getAttribute("title"))) Detail: Mind splitting that up into two separate statements? Putting three separate function calls into a single line, which you then end up splitting up into two anyway, is a bit contradictory.
https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29345746/new-options.js#newc... new-options.js:188: element.setAttribute("label", title); On 2016/06/22 10:26:13, Thomas Greiner wrote: > On 2016/06/21 13:29:56, saroyanm wrote: > > On 2016/06/17 14:07:50, Thomas Greiner wrote: > > > On 2016/06/16 18:22:59, saroyanm wrote: > > > > On 2016/06/16 10:42:22, Thomas Greiner wrote: > > > > > What's the "label" attribute supposed to do? > > > > > > > > I was testing with screen reader and apparently in this case it helps to > > > provide > > > > information about the list item, we are using labeldBy for the checkbox, > but > > > > it's still not obvious for the people what is the item is about, so using > > > label > > > > we can provide information about the item to the blind people. > > > > > > Ok, in that case please use the standardized "aria-label" attribute instead > > (see > > > https://www.w3.org/TR/wai-aria/states_and_properties#aria-label) because I > > > couldn't find any documentation for "label". > > > > I had another look and seems like removing this attribute works fine as well, > > for some reason now I can hear duplication if I keep this attribute, so I > think > > it's better to remove it for now. So your suspicions regarding this label was > > correct, sorry for that. But I was sure that this was the reason I used the > > attribute :/ > > I could imagine that the duplication might be because when you focus the > control, it reads first what section it's in and then its "aria-labelledby" > attribute which again refers to the section. In that case it should be > sufficient to remove the "aria-labelledby" attribute. Genius! Done. https://codereview.adblockplus.org/29338983/diff/29346907/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29346907/new-options.js#newc... new-options.js:125: getMessage(controls[k].getAttribute("title"))) On 2016/06/22 10:26:13, Thomas Greiner wrote: > Detail: Mind splitting that up into two separate statements? Putting three > separate function calls into a single line, which you then end up splitting up > into two anyway, is a bit contradictory. Done.
https://codereview.adblockplus.org/29338983/diff/29346956/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29346956/new-options.js#newc... new-options.js:122: var titleValue = getMessage(controls[k].getAttribute("title")); Detail: Please keep this in the if-statement as it was before because we don't need to call `getMessage()` if the control doesn't have a "title" attribute. https://codereview.adblockplus.org/29338983/diff/29346956/new-options.js#newc... new-options.js:192: element.setAttribute("aria-label", title); Detail: Was this change intentional? If so, why is it necessary to update this attribute? It's not being set when creating the list item.
https://codereview.adblockplus.org/29338983/diff/29346956/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29346956/new-options.js#newc... new-options.js:122: var titleValue = getMessage(controls[k].getAttribute("title")); On 2016/06/22 15:49:46, Thomas Greiner wrote: > Detail: Please keep this in the if-statement as it was before because we don't > need to call `getMessage()` if the control doesn't have a "title" attribute. Done. https://codereview.adblockplus.org/29338983/diff/29346956/new-options.js#newc... new-options.js:192: element.setAttribute("aria-label", title); On 2016/06/22 15:49:46, Thomas Greiner wrote: > Detail: Was this change intentional? If so, why is it necessary to update this > attribute? It's not being set when creating the list item. Yes this is how we are marking the label of the section, I did remove this in previous patch because this were causing the duplication, so I brought it back and works okey when I'm using screen-reader, otherwise it's not being obvious what the control element applies to.
https://codereview.adblockplus.org/29338983/diff/29346956/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29346956/new-options.js#newc... new-options.js:192: element.setAttribute("aria-label", title); On 2016/06/23 15:04:23, saroyanm wrote: > On 2016/06/22 15:49:46, Thomas Greiner wrote: > > Detail: Was this change intentional? If so, why is it necessary to update this > > attribute? It's not being set when creating the list item. > > Yes this is how we are marking the label of the section, I did remove this in > previous patch because this were causing the duplication, so I brought it back > and works okey when I'm using screen-reader, otherwise it's not being obvious > what the control element applies to. But why is it necessary since you already use `<label for="...">` to define the label for this element? And why are you setting it in `updateItem()` but not in `addItems()`?
https://codereview.adblockplus.org/29338983/diff/29346956/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29338983/diff/29346956/new-options.js#newc... new-options.js:192: element.setAttribute("aria-label", title); On 2016/06/24 15:22:10, Thomas Greiner wrote: > On 2016/06/23 15:04:23, saroyanm wrote: > > On 2016/06/22 15:49:46, Thomas Greiner wrote: > > > Detail: Was this change intentional? If so, why is it necessary to update > this > > > attribute? It's not being set when creating the list item. > > > > Yes this is how we are marking the label of the section, I did remove this in > > previous patch because this were causing the duplication, so I brought it back > > and works okey when I'm using screen-reader, otherwise it's not being obvious > > what the control element applies to. > > But why is it necessary since you already use `<label for="...">` to define the > label for this element? When I only use <label for="..."> and focusing the control element using screen reader I can't hear anything regarding the list item itself. > And why are you setting it in `updateItem()` but not in `addItems()`? The title can be changed, so we need to update the element during the update call.
Thomas can please check the last patch. I've removed the "for" attribute from the <label> element, while we are using the aria-label for the parent item and with current patch we are adding "aria-label" during the list item creation as well, not only during updating them.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
