Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
807593005:
Make downloads list keyboard shortcuts more consistent. (Closed)
|
Created:
6 years ago by hcarmona Modified:
5 years, 10 months ago CC:
chromium-reviews, asanka, benjhayden+dwatch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake downloads list keyboard shortcuts more consistent.
The changes make it possible to focus individual downloads by using UP/DOWN on the keyboard (like other tables and lists). It is possible to move between the links in a specific download using LEFT/RIGHT and TAB. Focus changes with TAB are achieved by relying on the focusing of elements rather than capturing the keys. This allows screen readers to focus elements without weird behavior.
Changes in FocusRow to support TAB navigation will also affect the history page. This change will keep both pages consistent.
BUG=427670, 452870
Committed: https://crrev.com/71d141817d79c0b42562b72f2b0af817c8c170fb
Cr-Commit-Position: refs/heads/master@{#314416}
Patch Set 1 #Patch Set 2 : Apply Initial Feedback #
Total comments: 32
Patch Set 3 : Apply Dan's feedback #Patch Set 4 : Fix TODOs #Patch Set 5 : fix tests #
Total comments: 14
Patch Set 6 : Apply feedback #Patch Set 7 : Apply feedback after talk #
Total comments: 24
Patch Set 8 : Apply feedback #Patch Set 9 : nit #
Total comments: 22
Patch Set 10 : Apply feedback #
Total comments: 30
Patch Set 11 : Apply Feedback #
Total comments: 24
Patch Set 12 : Apply Feedback #
Total comments: 4
Patch Set 13 : Apply feedback #Messages
Total messages: 37 (5 generated)
hcarmona@chromium.org changed reviewers: + dmazzoni@chromium.org
dmazzoni@ could you take a look at this patch and provide some usability feedback? Thanks!
The behavior change sounds good. As for the code, my only concern is detecting the TAB key. Would it be possible to implement the same thing by adding focus/blur event listeners? The reason is that screen readers have the ability to move focus to an element with keys other than tab - so if a screen reader user moved from one focusable element to the next I'd expect it to behave the same, but currently the behavior would be different. Does that make sense? If you think that won't work, please explain more.
On 2014/12/18 18:08:53, dmazzoni wrote: > The behavior change sounds good. > > As for the code, my only concern is detecting the TAB key. > > Would it be possible to implement the same thing by adding focus/blur event > listeners? The reason is that screen readers have the ability to move focus to > an element with keys other than tab - so if a screen reader user moved from one > focusable element to the next I'd expect it to behave the same, but currently > the behavior would be different. > > Does that make sense? If you think that won't work, please explain more. Thanks for the feedback! I will look into using focus/blur instead of TAB and continue in this direction.
hcarmona@chromium.org changed reviewers: + dbeam@chromium.org
This CL has 2 bugs that I'm still looking at, but it's at a state where it can be given a first look. The issues it currently has: - Focus behaves weird when a new item is downloaded and the downloads page is open - The "Keep" and "Discard" buttons that appear when a "dangerous" file is downloaded are not behaving consistently with the other links
Just a few minor things - this looks pretty good https://codereview.chromium.org/807593005/diff/20001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/807593005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:194: Downloads.makeFocusable = function(elem) { Maybe makePartOfFocusGrid or addFocusGridItemClass? makeFocusable makes me think it's going to set the tabIndex. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:140: else if (wasActive) Formatting nit: the "else" should be on the same line as the "{", and the braces around the "else" block are required if you used them for the "if" block, e.g.: if (foo) { bar; } else { baz; } https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:162: * Focuses the item at |index|. Update these docs?
https://codereview.chromium.org/807593005/diff/20001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/807593005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:185: grid.push(node.getElementsByClassName('row-item')); use querySelectorAll('.row-item') unless you want all of these to change when row-item classes are added/re oved from the DOM https://codereview.chromium.org/807593005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:194: Downloads.makeFocusable = function(elem) { what happens if this class is added after the focusgrid has been created? how does it get updated? can we add an assert or a rebuildFocusGrid_ or something here to make sure? https://codereview.chromium.org/807593005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:330: function DownloadFocusObserver() {} you could probably make the HistoryFocusObserver into a more generic ActiveFocusObserver or something like that (that just takes an ancestor class and active class to apply) https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:95: FocusRow.isFocusable = function(element) { why is this public? and static? https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:96: var style = window.getComputedStyle(element); getComputedStyle already takes the entire hierarchy into account -- no need to traverse https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:97: if (style.visibility == 'hidden' || style.display == 'none') what about tabindex < 0? https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:100: if (element.parentElement) this can also be null when the element's not in the DOM (not just at the top of the tree) https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:138: }.bind(this)); pass in a thisArg instead of .bind()ing https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:140: else if (wasActive) } else if (...) { https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:159: previousIndex_: 0, not very explanatory here... https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:166: focusIndex: function(index) { change from index to ID like bondd@ did in settings (and allow specifying a column ID for each type of field) https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:198: assert(false); assertNotReached() https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:203: this.previousIndex_ = index; previouslyFocusedIndex_?
hcarmona@chromium.org changed reviewers: + jhawkins@chromium.org
Hi James, Hoping you're the right person to review these changes. Originally had Dan as a reviewer, but he's OoO. I still have some Todos that I need to address, but I wanted to get some feedback. Thanks, Hector https://codereview.chromium.org/807593005/diff/20001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/807593005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:185: grid.push(node.getElementsByClassName('row-item')); On 2014/12/22 23:19:59, Dan Beam wrote: > use querySelectorAll('.row-item') unless you want all of these to change when > row-item classes are added/re oved from the DOM Elements are now added explicitly, so there's no need to query for them. https://codereview.chromium.org/807593005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:194: Downloads.makeFocusable = function(elem) { On 2014/12/22 23:14:07, dmazzoni wrote: > Maybe makePartOfFocusGrid or addFocusGridItemClass? > > makeFocusable makes me think it's going to set the tabIndex. Done. https://codereview.chromium.org/807593005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:194: Downloads.makeFocusable = function(elem) { On 2014/12/22 23:19:59, Dan Beam wrote: > what happens if this class is added after the focusgrid has been created? how > does it get updated? can we add an assert or a rebuildFocusGrid_ or something > here to make sure? Created the rebuildFocusGrid_ method to make it more explicit when the grid is rebuilt. It doesn't make sense to call it inside this method because this is called multiple times while building a single download. rebuildFocusGrid_ will be called in updateResults. https://codereview.chromium.org/807593005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:330: function DownloadFocusObserver() {} On 2014/12/22 23:19:59, Dan Beam (ooo till 1-22) wrote: > you could probably make the HistoryFocusObserver into a more generic > ActiveFocusObserver or something like that (that just takes an ancestor class > and active class to apply) Observer will now handle the case when a column misses. This will allow any page to handle a miss any way they want to. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:95: FocusRow.isFocusable = function(element) { On 2014/12/22 23:19:59, Dan Beam wrote: > why is this public? and static? Made private and moved to the downloads page to be used in determining whether to add an element to the FocusRow. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:96: var style = window.getComputedStyle(element); On 2014/12/22 23:19:59, Dan Beam wrote: > getComputedStyle already takes the entire hierarchy into account -- no need to > traverse This doesn't work for display because display:none is added to a parent element but the child element has display:block set. This means that even though the element has display:block, it's hidden because its parent has display:none and it doesn't look like there's a concept of priority other than the most specific rule wins. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:97: if (style.visibility == 'hidden' || style.display == 'none') On 2014/12/22 23:19:59, Dan Beam wrote: > what about tabindex < 0? This will be used to update the tabIndex. All elements should have a tabIndex of 0 when a row is focused and -1 when they're not focused. This allows TAB to be used to traverse elements in the selected row and skip out of the list when tabbing after the last element. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:100: if (element.parentElement) On 2014/12/22 23:19:59, Dan Beam wrote: > this can also be null when the element's not in the DOM (not just at the top of > the tree) Done. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:138: }.bind(this)); On 2014/12/22 23:19:59, Dan Beam wrote: > pass in a thisArg instead of .bind()ing bind is left over from previous code. Removed it. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:140: else if (wasActive) On 2014/12/22 23:14:07, dmazzoni wrote: > Formatting nit: the "else" should be on the same line as the "{", and the braces > around the "else" block are required if you used them for the "if" block, e.g.: > > if (foo) { > bar; > } else { > baz; > } Done. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:140: else if (wasActive) On 2014/12/22 23:19:59, Dan Beam wrote: > } else if (...) { Done. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:159: previousIndex_: 0, On 2014/12/22 23:19:59, Dan Beam (ooo till 1-22) wrote: > not very explanatory here... Code changed so this is no longer necessary. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:162: * Focuses the item at |index|. On 2014/12/22 23:14:07, dmazzoni wrote: > Update these docs? Code changed so this is no longer necessary. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:166: focusIndex: function(index) { On 2014/12/22 23:19:59, Dan Beam (ooo till 1-22) wrote: > change from index to ID like bondd@ did in settings (and allow specifying a > column ID for each type of field) Done. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:198: assert(false); On 2014/12/22 23:19:59, Dan Beam wrote: > assertNotReached() Done. https://codereview.chromium.org/807593005/diff/20001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:203: this.previousIndex_ = index; On 2014/12/22 23:19:59, Dan Beam (ooo till 1-22) wrote: > previouslyFocusedIndex_? Code has changed and this is different now.
hcarmona@chromium.org changed reviewers: + estade@chromium.org - jhawkins@chromium.org
Hi Evan, James said you might be able to review this change. Thanks, Hector
On 2015/01/14 22:02:49, Hector Carmona wrote: > Hi Evan, > > James said you might be able to review this change. > > Thanks, > Hector have you addressed the review comments from dmazzoni and dbeam? Once you do, and once dmazzoni gives LG, I can review.
On 2015/01/15 02:01:29, Evan Stade wrote: > On 2015/01/14 22:02:49, Hector Carmona wrote: > > Hi Evan, > > > > James said you might be able to review this change. > > > > Thanks, > > Hector > > have you addressed the review comments from dmazzoni and dbeam? Once you do, and > once dmazzoni gives LG, I can review. (it's customary to respond to review comments with "Done" or other stuff as needed)
On 2015/01/15 02:02:19, Evan Stade wrote: > On 2015/01/15 02:01:29, Evan Stade wrote: > > On 2015/01/14 22:02:49, Hector Carmona wrote: > > > Hi Evan, > > > > > > James said you might be able to review this change. > > > > > > Thanks, > > > Hector > > > > have you addressed the review comments from dmazzoni and dbeam? Once you do, > and > > once dmazzoni gives LG, I can review. > > (it's customary to respond to review comments with "Done" or other stuff as > needed) I've replied to dmazzoni and dbeam's comments with either Done or another reply. Some of the code has changed where the original comment no longer applies, what's the best way to reply in that scenario? dmazzoni, would you take another look at this change since it's somewhat different from the original CL? Thanks!
On 2015/01/15 19:09:37, Hector Carmona wrote: > On 2015/01/15 02:02:19, Evan Stade wrote: > > On 2015/01/15 02:01:29, Evan Stade wrote: > > > On 2015/01/14 22:02:49, Hector Carmona wrote: > > > > Hi Evan, > > > > > > > > James said you might be able to review this change. > > > > > > > > Thanks, > > > > Hector > > > > > > have you addressed the review comments from dmazzoni and dbeam? Once you do, > > and > > > once dmazzoni gives LG, I can review. > > > > (it's customary to respond to review comments with "Done" or other stuff as > > needed) > > I've replied to dmazzoni and dbeam's comments with either Done or another reply. oops, missed them somehow. > Some of the code has changed where the original comment no longer applies, > what's the best way to reply in that scenario? You could respond with "code removed" or something in that case. > > > > dmazzoni, would you take another look at this change since it's somewhat > different from the original CL? > > Thanks!
lgtm https://codereview.chromium.org/807593005/diff/80001/chrome/browser/resources... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/807593005/diff/80001/chrome/browser/resources... chrome/browser/resources/history/history.js:1727: // These 2 buttons are mutually exclusive, but visually the same. I don't quite understand this comment. https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:240: enableRowTab: function(allow) { I'd call this something like makeRowFocusable or at least enableRowTabIndex - "tab" isn't clear https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:262: this.getElement(this.elementIds[0]); Just checking, this works fine on an empty list / grid?
https://codereview.chromium.org/807593005/diff/80001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.html (right): https://codereview.chromium.org/807593005/diff/80001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.html:13: <script src="chrome://resources/js/assert.js"></script> alphabetize? https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:111: onElementIdMiss: assertNotReached, this name is super confusing to me. It should be something like getElementIdForMissingElementId. Also, I think you're overloading Id here, which leads to lots of confusion. I'd recommend not using "ID" for anything except the HTML attribute. https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:131: getElement: function (elementId) { nit: extra space https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:249: * @return {Element} A focusable element that best matches |elementId|. I am confused how you could "best match" an ID. IDs are generally used for exact matches.
Applied dmazzoni and estade's feedback. Biggest change was element id to element code. The CL is still working the same way, but the change is intended to avoid confusing "id" with the HTML attribute. As part of this change, I've renamed a few functions to make their intentions more clear and updated comments. https://codereview.chromium.org/807593005/diff/80001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.html (right): https://codereview.chromium.org/807593005/diff/80001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.html:13: <script src="chrome://resources/js/assert.js"></script> On 2015/01/15 23:38:09, Evan Stade wrote: > alphabetize? Done. https://codereview.chromium.org/807593005/diff/80001/chrome/browser/resources... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/807593005/diff/80001/chrome/browser/resources... chrome/browser/resources/history/history.js:1727: // These 2 buttons are mutually exclusive, but visually the same. On 2015/01/15 19:38:19, dmazzoni wrote: > I don't quite understand this comment. Done. https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:111: onElementIdMiss: assertNotReached, On 2015/01/15 23:38:09, Evan Stade wrote: > this name is super confusing to me. It should be something like > getElementIdForMissingElementId. Done > Also, I think you're overloading Id here, which leads to lots of confusion. I'd > recommend not using "ID" for anything except the HTML attribute. Makes sense. Changed the "elementID" concept to "element code". I think "code" doesn't have the context of "ID" where an exact match is expected. I was also thinking about "tag" or "token", but I think both of those words also have other meanings where "code" doesn't. What do you think? https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:131: getElement: function (elementId) { On 2015/01/15 23:38:09, Evan Stade wrote: > nit: extra space Done. https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:240: enableRowTab: function(allow) { On 2015/01/15 19:38:19, dmazzoni wrote: > I'd call this something like makeRowFocusable or at least enableRowTabIndex - > "tab" isn't clear Done. https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:249: * @return {Element} A focusable element that best matches |elementId|. On 2015/01/15 23:38:09, Evan Stade wrote: > I am confused how you could "best match" an ID. IDs are generally used for exact > matches. Done. https://codereview.chromium.org/807593005/diff/80001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/focus_row.js:262: this.getElement(this.elementIds[0]); On 2015/01/15 19:38:19, dmazzoni wrote: > Just checking, this works fine on an empty list / grid? An empty grid would have no rows, so it should be skipped in the tab order. Which is good. However, if a row has no focus-able columns this won't work. Right now, this code is built with the assumption that a FocusRow has at least one focusable element. I've updated the documentation at the top of this file to reflect this.
This still seems very complicated overall. Why can't you use some combination of attributes and classes and querySelectorAll?
On 2015/01/16 22:13:16, Evan Stade wrote: > This still seems very complicated overall. Why can't you use some combination of > attributes and classes and querySelectorAll? The big problem in these UIs is that the columns are not consistent. A row in the history page has an extra button if it's bookmarked. A download can end up in one of many states, each with their own combination of columns. If we just use query selector and maintain the index of the focused element there will be unintuitive behavior. Easiest example is in history when there's a bookmark and you move up down it will switch from the site name to the bookmark, back to the site name. With these changes if you have the site focused and you switch to another row, the site will remain focused whether the site is bookmarked or not because we're using the element's code to look it up instead of an index. Using a look up code also lets each UI decide what to do on a miss.
Updated the FocusRow and FocusGrid based on discussion with estade. FocusRow and FocusGrid will no longer use "codes" or other weirdness and each page can override how it gets the equivalent element.
looking better https://codereview.chromium.org/807593005/diff/120001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/807593005/diff/120001/chrome/browser/resource... chrome/browser/resources/history/history.js:937: ret = this.getMatch('[column-type="checkbox"]'); what is getMatch? seems a lot like querySelector https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/focus_grid.js (right): https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_grid.js:89: return { row: i, element: target }; I don't think you actually use the |element| part of the response anywhere (it's redundant since it's always the passed in parameter) https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_grid.js:151: else { } else if { } else { } https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:52: this.rowElements = []; I can't figure out what this is or what this.rowElement is. Is this the elements within the row, and rowElement is the row itself? Confusing... needs better names. rowElement_ should be equivalent to |this| (via decorate) https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:114: setFocusableElement: function(element) { addFocusableElement https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:147: makeRowFocusable: function(allow) { nit: more like makeRowActive https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:154: this.onActivate(this); I don't think you need to pass |this| https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:155: } nit: } else { https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:158: this.onDeactivate(this); can you combine Activate and Deactivate? Then this code is: this.rowElement_.classList.toggle('focus-row-active', allow); this.onActiveStateChanged(); https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:172: * @param {Element} element The element that focus should be based on. nit: "An element from a row of the same type which previously held focus." https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:174: focusEquivalentElement: function(element) { this whole function seems a bit extraneous. The callsite could just be this.getEquivalentElement(element).focus(); https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:208: var element = this.getEquivalentElement(e.target); I'm confused about what this is doing, probably because |element| is not a descriptive variable name (and it's reused below). I think this function needs better docs to indicate it only handles in-row focus changes.
Applied feedback. https://codereview.chromium.org/807593005/diff/120001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/807593005/diff/120001/chrome/browser/resource... chrome/browser/resources/history/history.js:937: ret = this.getMatch('[column-type="checkbox"]'); On 2015/01/22 16:55:10, Evan Stade wrote: > what is getMatch? seems a lot like querySelector Done. Replaced getMatch with querySelector. https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/focus_grid.js (right): https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_grid.js:89: return { row: i, element: target }; On 2015/01/22 16:55:10, Evan Stade wrote: > I don't think you actually use the |element| part of the response anywhere (it's > redundant since it's always the passed in parameter) Good catch. Renamed to getRowIndexForTarget to make it more clear what it's doing. https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_grid.js:151: else { On 2015/01/22 16:55:10, Evan Stade wrote: > } else if { > > } else { > > } Done. https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:52: this.rowElements = []; On 2015/01/22 16:55:11, Evan Stade wrote: > I can't figure out what this is or what this.rowElement is. Is this the elements > within the row, and rowElement is the row itself? Confusing... needs better > names. rowElement_ should be equivalent to |this| (via decorate) |this| is now what rowElement_ used to be. Renamed |rowElements| to |focusableElements| to make the name more clear. https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:114: setFocusableElement: function(element) { On 2015/01/22 16:55:11, Evan Stade wrote: > addFocusableElement Done. https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:147: makeRowFocusable: function(allow) { On 2015/01/22 16:55:11, Evan Stade wrote: > nit: more like makeRowActive Done. https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:154: this.onActivate(this); On 2015/01/22 16:55:10, Evan Stade wrote: > I don't think you need to pass |this| Done. https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:155: } On 2015/01/22 16:55:11, Evan Stade wrote: > nit: } else { Done. https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:158: this.onDeactivate(this); On 2015/01/22 16:55:10, Evan Stade wrote: > can you combine Activate and Deactivate? Then this code is: > > this.rowElement_.classList.toggle('focus-row-active', allow); > this.onActiveStateChanged(); Done. https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:172: * @param {Element} element The element that focus should be based on. On 2015/01/22 16:55:11, Evan Stade wrote: > nit: "An element from a row of the same type which previously held focus." Done. https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:174: focusEquivalentElement: function(element) { On 2015/01/22 16:55:11, Evan Stade wrote: > this whole function seems a bit extraneous. The callsite could just be > this.getEquivalentElement(element).focus(); Done. https://codereview.chromium.org/807593005/diff/120001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:208: var element = this.getEquivalentElement(e.target); On 2015/01/22 16:55:11, Evan Stade wrote: > I'm confused about what this is doing, probably because |element| is not a > descriptive variable name (and it's reused below). Removed element because it will always equal e.target so it doesn't make sense to try and find a match. Renamed where it was reused to elementToFocus to make the intention more clear. > I think this function needs better docs to indicate it only handles in-row focus > changes. Done.
https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:94: return self; what's the point to this pattern of returning something instead of the default |this| that a constructor function typically returns automatically? https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:105: this.addFocusRow_(this.download_.nodeFileLink_, 'name'); i understand @private is file-level in closure, but this file is way bigger than it should be (arguably it should be split a few ways). can you add a TODO here to split these classes into more files and use accessors for these private members (e.g. nodeURL_) instead? https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:136: switch (element.getAttribute('column-type')) { what is this entire switch trying to accomplish? https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:137: case ('show'): no parens case 'show': https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:148: this.querySelector('[column-type="cancel"]'); querySelector('[column-type=show], [column-type=retry]...') https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/history/history.js:252: } nit: \n https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/history/history.js:254: bookmarkSection.tabIndex = -1; nit: \n https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/history/history.js:940: ret = this.querySelector('[column-type="checkbox"]'); nit: getColumn_: function(type) { return this.querySelector('[column-type=' + type + ']'); }, ... this.getColumn_('checkbox') https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... File chrome/browser/resources/history/other_devices.js (right): https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/history/other_devices.js:517: for (var i = 0; i < rows.length; ++i) nit: curlies https://codereview.chromium.org/807593005/diff/160001/chrome/test/data/webui/... File chrome/test/data/webui/history_browsertest.js (right): https://codereview.chromium.org/807593005/diff/160001/chrome/test/data/webui/... chrome/test/data/webui/history_browsertest.js:790: expectEquals(3, results.length); expectGE(1, results.length) https://codereview.chromium.org/807593005/diff/160001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/807593005/diff/160001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:97: * NOT return null or undefined. the "should not return null or undefined" is duplicative (@return {!Element} means this)
Applied Dan's feedback. https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:94: return self; On 2015/01/24 02:37:07, Dan Beam wrote: > what's the point to this pattern of returning something instead of the default > |this| that a constructor function typically returns automatically? I had seen this in the code when a constructor would create an element to represent the object. Although none of those were decorating an element that was passed in. I changed this to use a static decorate method so that it's more clear that the element is being decorated rather than a new object being created. https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:105: this.addFocusRow_(this.download_.nodeFileLink_, 'name'); On 2015/01/24 02:37:08, Dan Beam wrote: > i understand @private is file-level in closure, but this file is way bigger than > it should be (arguably it should be split a few ways). > > can you add a TODO here to split these classes into more files and use accessors > for these private members (e.g. nodeURL_) instead? Done. https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:136: switch (element.getAttribute('column-type')) { On 2015/01/24 02:37:08, Dan Beam wrote: > what is this entire switch trying to accomplish? Got rid of switch to make it more clear what's happening. There are a few UI elements that are at the same level visually, so the first available one should retain focus. https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:137: case ('show'): On 2015/01/24 02:37:07, Dan Beam wrote: > no parens > > case 'show': Done. https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:148: this.querySelector('[column-type="cancel"]'); On 2015/01/24 02:37:07, Dan Beam wrote: > querySelector('[column-type=show], [column-type=retry]...') Done. https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/history/history.js:252: } On 2015/01/24 02:37:08, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/history/history.js:254: bookmarkSection.tabIndex = -1; On 2015/01/24 02:37:08, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/history/history.js:940: ret = this.querySelector('[column-type="checkbox"]'); On 2015/01/24 02:37:08, Dan Beam wrote: > nit: > > getColumn_: function(type) { > return this.querySelector('[column-type=' + type + ']'); > }, > ... > this.getColumn_('checkbox') Done. https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... File chrome/browser/resources/history/other_devices.js (right): https://codereview.chromium.org/807593005/diff/160001/chrome/browser/resource... chrome/browser/resources/history/other_devices.js:517: for (var i = 0; i < rows.length; ++i) On 2015/01/24 02:37:08, Dan Beam wrote: > nit: curlies Done. https://codereview.chromium.org/807593005/diff/160001/chrome/test/data/webui/... File chrome/test/data/webui/history_browsertest.js (right): https://codereview.chromium.org/807593005/diff/160001/chrome/test/data/webui/... chrome/test/data/webui/history_browsertest.js:790: expectEquals(3, results.length); On 2015/01/24 02:37:08, Dan Beam wrote: > expectGE(1, results.length) Done. https://codereview.chromium.org/807593005/diff/160001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/807593005/diff/160001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:97: * NOT return null or undefined. On 2015/01/24 02:37:08, Dan Beam wrote: > the "should not return null or undefined" is duplicative (@return {!Element} > means this) Done.
overall I think this is looking good https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:88: DownloadFocusRow = function() {}; function DownloadFocusRow() {} is the more typical style https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:130: // All elements default to another element with the same type. nit: arguably var columnType = element.getAttribute('column-type'); ... re-use columnType ... https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:132: var ret = this.querySelector(query); nit: ret -> el, element, equiv, equivalent (all seem better) https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:136: var grayLinks = ['show', 'retry', 'pause', 'resume', 'remove', 'cancel']; nit: grayLinks -> equivalents (or rename so this isn't based on the presentation) https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:141: '[column-type=remove], [column-type=cancel]'); why are we duplicating this list of types? https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:145: // Any other miss should return the first focusable element. nit: // Return the first focusable element if no equivalent type is found. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:152: * @param {string} type The type to use for the element. can we make these doc strings more useful or remove? https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:174: if (style.visibility == 'hidden' || style.display == 'none') here's the full amount of checks that blink does, btw: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:183: return true; nit: // Verify all ancestors are focusable. return !element.parentNode || this.shouldFocus_(element.parentNode); https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:300: var keys = Object.keys(this.downloads_); I'm not sure that the order of keys returned from Object.keys() is defined, so you may want to sort to find another way to get a strong guarantee if it matters. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:303: DownloadFocusRow.decorate(download.node, download, this.node_); are all the rows always blasted away and rebuilt? otherwise it seems like these rows might be (harmfully or wastefully) re-decorated here. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/history/history.js:898: HistoryFocusRow = function() {}; why did you change from function Declaration() {} to Expression = function() {};? https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/history/history.js:961: * @return {Element?} The column matching the type. nit: ?Element https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/history/history.js:1250: row.querySelector('[column-type="domain"]'); nit: arguably combine querySelector() calls into 1 https://codereview.chromium.org/807593005/diff/180001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/focus_grid.js (right): https://codereview.chromium.org/807593005/diff/180001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_grid.js:51: }; nit: \n
https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:88: DownloadFocusRow = function() {}; On 2015/01/27 22:32:23, Dan Beam wrote: > function DownloadFocusRow() {} > > is the more typical style Done. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:130: // All elements default to another element with the same type. On 2015/01/27 22:32:23, Dan Beam wrote: > nit: arguably > > var columnType = element.getAttribute('column-type'); > ... re-use columnType ... Done. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:132: var ret = this.querySelector(query); On 2015/01/27 22:32:23, Dan Beam wrote: > nit: ret -> el, element, equiv, equivalent (all seem better) Done. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:136: var grayLinks = ['show', 'retry', 'pause', 'resume', 'remove', 'cancel']; On 2015/01/27 22:32:23, Dan Beam wrote: > nit: grayLinks -> equivalents (or rename so this isn't based on the > presentation) Done. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:141: '[column-type=remove], [column-type=cancel]'); On 2015/01/27 22:32:23, Dan Beam wrote: > why are we duplicating this list of types? Done. Not duplicated anymore. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:145: // Any other miss should return the first focusable element. On 2015/01/27 22:32:23, Dan Beam wrote: > nit: // Return the first focusable element if no equivalent type is found. Done. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:152: * @param {string} type The type to use for the element. On 2015/01/27 22:32:23, Dan Beam wrote: > can we make these doc strings more useful or remove? Done. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:174: if (style.visibility == 'hidden' || style.display == 'none') On 2015/01/27 22:32:23, Dan Beam wrote: > here's the full amount of checks that blink does, btw: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... This check is sufficient for the downloads page. Do we need to expand it to all possibilities? If so, can blink provide this information (to avoid duplicating code)? Or is it better to rename this function to make it clear that shouldFocus_ applies only to downloads? https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:183: return true; On 2015/01/27 22:32:23, Dan Beam wrote: > nit: > > // Verify all ancestors are focusable. > return !element.parentNode || this.shouldFocus_(element.parentNode); Done. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:300: var keys = Object.keys(this.downloads_); On 2015/01/27 22:32:23, Dan Beam wrote: > I'm not sure that the order of keys returned from Object.keys() is defined, so > you may want to sort to find another way to get a strong guarantee if it > matters. The order does matter. Updated code so that it is not dependent on the keys and instead the grid is based on querying for the downloads from the document. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:303: DownloadFocusRow.decorate(download.node, download, this.node_); On 2015/01/27 22:32:23, Dan Beam wrote: > are all the rows always blasted away and rebuilt? otherwise it seems like these > rows might be (harmfully or wastefully) re-decorated here. Changed so that rows are only decorated once. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/history/history.js:898: HistoryFocusRow = function() {}; On 2015/01/27 22:32:23, Dan Beam wrote: > why did you change from function Declaration() {} to Expression = function() > {};? Must have happened when I moved the code around. Changed back. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/history/history.js:961: * @return {Element?} The column matching the type. On 2015/01/27 22:32:23, Dan Beam wrote: > nit: ?Element Done. https://codereview.chromium.org/807593005/diff/180001/chrome/browser/resource... chrome/browser/resources/history/history.js:1250: row.querySelector('[column-type="domain"]'); On 2015/01/27 22:32:23, Dan Beam wrote: > nit: arguably combine querySelector() calls into 1 Done. https://codereview.chromium.org/807593005/diff/180001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/focus_grid.js (right): https://codereview.chromium.org/807593005/diff/180001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_grid.js:51: }; On 2015/01/27 22:32:24, Dan Beam wrote: > nit: \n Done.
https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... File chrome/browser/resources/downloads/downloads.html (right): https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.html:16: <script src="chrome://resources/js/cr/ui/focus_row.js"></script> do any of these classes assume knowledge of the others? e.g. does FocusGrid know about FocusRow? if so, we should probably use that inclusion order rather than alphabetical https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:136: // The following element types are equivalent. this comment doesn't seem useful now https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:140: var query = equivalentTypes.map(function(type) { s/query/allTypes/ https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:305: download.node.isFocusRow = true; move this functionality into cr.ui.FocusRow, e.g., cr.ui.FocusRow.decorate = function(row) { if (row.decorated) return; row.decorated = true; // or whatever, isFocusRow is fine I guess }; and then /** @override ... */ DownloadFocusRow.decorate = function(row) { cr.ui.FocusRow.decorate.apply(this, arguments); ... }; this means that this loop can be: var download = this.downloads_[keys[i]]; DownloadFocusRow.decorate(download.node, download, this.node_); with no duplicate code https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/history/history.js:912: * that a checkbox remains focused when focus changes. use // comments instead of /** comment */ (the latter means jsdoc) https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/history/history.js:912: * that a checkbox remains focused when focus changes. i don't think this comment is all that helpful. i think a reader will understand they're both the same type by both being 'checkbox'. https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/history/history.js:917: // Add the remaining elements. not useful https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/history/history.js:938: var ret = this.getColumn_(element.getAttribute('column-type')); s/ret/equivalent or el https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/history/history.js:974: addFocusRow_: function(query, type) { shouldn't this be addFocusItem_ or something? this isn't adding a focus *row*, is it? https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... File chrome/browser/resources/history/other_devices.js (right): https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/history/other_devices.js:434: function DevicesView.SingleColumnFocusRow() {} this isn't valid JavaScript. please try your code before uploading again. https://codereview.chromium.org/807593005/diff/200001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/807593005/diff/200001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:33: FocusRow = function() {}; function FocusRow() {} please apply feedback throughout reviews when I mention things other places https://codereview.chromium.org/807593005/diff/200001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:65: /** @type {!Node} */ nit: @private {!Node}
Applied feedback https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... File chrome/browser/resources/downloads/downloads.html (right): https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.html:16: <script src="chrome://resources/js/cr/ui/focus_row.js"></script> On 2015/01/29 18:46:46, Dan Beam wrote: > do any of these classes assume knowledge of the others? e.g. does FocusGrid > know about FocusRow? if so, we should probably use that inclusion order rather > than alphabetical Done. Also removed focus_manager.js since it's not being used. https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:136: // The following element types are equivalent. On 2015/01/29 18:46:46, Dan Beam wrote: > this comment doesn't seem useful now Done. https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:140: var query = equivalentTypes.map(function(type) { On 2015/01/29 18:46:46, Dan Beam wrote: > s/query/allTypes/ Done. https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:305: download.node.isFocusRow = true; On 2015/01/29 18:46:46, Dan Beam wrote: > move this functionality into cr.ui.FocusRow, e.g., > > cr.ui.FocusRow.decorate = function(row) { > if (row.decorated) > return; > row.decorated = true; // or whatever, isFocusRow is fine I guess > }; > > and then > > /** @override ... */ > DownloadFocusRow.decorate = function(row) { > cr.ui.FocusRow.decorate.apply(this, arguments); > ... > }; > > this means that this loop can be: > > var download = this.downloads_[keys[i]]; > DownloadFocusRow.decorate(download.node, download, this.node_); > > with no duplicate code Changed back to being "destructive". Decorating a FocusRow will update the focusableElements. This is important in the case when the displayed elements change. (eg: downloading (pause, cancel) -> downloaded (show in folder, remove from list)). The FocusGrid.destroy() function is also important here because it cleans up the FocusRows so they can be decorated again without ill effects. The elements are not destroyed, just re-decorated so focus is maintained. https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/history/history.js:912: * that a checkbox remains focused when focus changes. On 2015/01/29 18:46:46, Dan Beam wrote: > i don't think this comment is all that helpful. i think a reader will > understand they're both the same type by both being 'checkbox'. Done. https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/history/history.js:912: * that a checkbox remains focused when focus changes. On 2015/01/29 18:46:46, Dan Beam wrote: > use > > // comments > > instead of > > /** comment */ > > (the latter means jsdoc) Done. https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/history/history.js:917: // Add the remaining elements. On 2015/01/29 18:46:46, Dan Beam wrote: > not useful Done. https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/history/history.js:938: var ret = this.getColumn_(element.getAttribute('column-type')); On 2015/01/29 18:46:46, Dan Beam wrote: > s/ret/equivalent or el Done. https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/history/history.js:974: addFocusRow_: function(query, type) { On 2015/01/29 18:46:46, Dan Beam wrote: > shouldn't this be addFocusItem_ or something? this isn't adding a focus *row*, > is it? Done. https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... File chrome/browser/resources/history/other_devices.js (right): https://codereview.chromium.org/807593005/diff/200001/chrome/browser/resource... chrome/browser/resources/history/other_devices.js:434: function DevicesView.SingleColumnFocusRow() {} On 2015/01/29 18:46:46, Dan Beam wrote: > this isn't valid JavaScript. please try your code before uploading again. Updated. https://codereview.chromium.org/807593005/diff/200001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/807593005/diff/200001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:33: FocusRow = function() {}; On 2015/01/29 18:46:46, Dan Beam wrote: > function FocusRow() {} > > please apply feedback throughout reviews when I mention things other places Done. https://codereview.chromium.org/807593005/diff/200001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/focus_row.js:65: /** @type {!Node} */ On 2015/01/29 18:46:47, Dan Beam wrote: > nit: @private {!Node} Done.
lgtm https://codereview.chromium.org/807593005/diff/220001/chrome/browser/resource... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/807593005/diff/220001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:113: // there is a dangerous download right next to a malicious download. nit: Elements with the same type are mutually exclusive. https://codereview.chromium.org/807593005/diff/220001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/807593005/diff/220001/chrome/browser/resource... chrome/browser/resources/history/history.js:966: addElementIfExists_: function(query, type) { nit: s/Exists/Present
Applied Dan's feedback. estade@, any objections to committing this CL? https://codereview.chromium.org/807593005/diff/220001/chrome/browser/resource... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/807593005/diff/220001/chrome/browser/resource... chrome/browser/resources/downloads/downloads.js:113: // there is a dangerous download right next to a malicious download. On 2015/01/31 18:28:30, Dan Beam wrote: > nit: Elements with the same type are mutually exclusive. Done. https://codereview.chromium.org/807593005/diff/220001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/807593005/diff/220001/chrome/browser/resource... chrome/browser/resources/history/history.js:966: addElementIfExists_: function(query, type) { On 2015/01/31 18:28:30, Dan Beam wrote: > nit: s/Exists/Present Done.
i trust in dan's review. lgtm
The CQ bit was checked by hcarmona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/807593005/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/71d141817d79c0b42562b72f2b0af817c8c170fb Cr-Commit-Position: refs/heads/master@{#314416} |
