downloads: put [column-type] in HTML template instead of JS.

downloads: put [column-type] in HTML template instead of JS.

R=hcarmona@chromium.org
BUG=none

Committed: https://crrev.com/11940994a992a8531150ab256fc6e711df63d142
Cr-Commit-Position: refs/heads/master@{#320762}

[hcarmona, 2015-03-12 17:28:32]

https://codereview.chromium.org/1000163002/diff/1/chrome/browser/resources/do...
File chrome/browser/resources/downloads/item_view.js (right):

https://codereview.chromium.org/1000163002/diff/1/chrome/browser/resources/do...
chrome/browser/resources/downloads/item_view.js:230: var link =
this.controlledBy_.querySelector('a');
|link| should be the focusable element rather than |controlledBy_| in order to
toggle |link|'s TAB-ability.

[Dan Beam, 2015-03-12 18:52:40]

https://codereview.chromium.org/1000163002/diff/1/chrome/browser/resources/do...
File chrome/browser/resources/downloads/item_view.js (right):

https://codereview.chromium.org/1000163002/diff/1/chrome/browser/resources/do...
chrome/browser/resources/downloads/item_view.js:230: var link =
this.controlledBy_.querySelector('a');
On 2015/03/12 17:28:32, Hector Carmona wrote:
> |link| should be the focusable element rather than |controlledBy_| in order to
> toggle |link|'s TAB-ability.

i noticed this but figured that's what downloads.js did before I split it into
item_view.js, but looking again: you're right -- controlByExtensionLink_
https://codereview.chromium.org/977473002/diff/370001/chrome/browser/resource...

Done.

[Dan Beam, 2015-03-12 18:53:07]

Patchset #2 (id:20001) has been deleted

[hcarmona, 2015-03-12 19:12:52]

https://codereview.chromium.org/1000163002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/downloads/manager.js (right):

https://codereview.chromium.org/1000163002/diff/40001/chrome/browser/resource...
chrome/browser/resources/downloads/manager.js:135: if
(focusRow.focusableElements.indexOf(activeElement) >= 0 &&
This should stay "if (focusRow.contains(activeElement))" because we want to
update focus for the row that contains the active element and |activeElement|
may be hidden. The call to |getEquivalentElement| will ensure that the element
that you call |focus| on is visible.

[Dan Beam, 2015-03-12 20:19:03]

https://codereview.chromium.org/1000163002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/downloads/manager.js (right):

https://codereview.chromium.org/1000163002/diff/40001/chrome/browser/resource...
chrome/browser/resources/downloads/manager.js:135: if
(focusRow.focusableElements.indexOf(activeElement) >= 0 &&
On 2015/03/12 19:12:52, Hector Carmona wrote:
> This should stay "if (focusRow.contains(activeElement))" because we want to
> update focus for the row that contains the active element and |activeElement|
> may be hidden. The call to |getEquivalentElement| will ensure that the element
> that you call |focus| on is visible.

Fixed (differently though, see updated code)

https://codereview.chromium.org/1000163002/diff/80001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/1000163002/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_row.js:106: assert(this.contains(element));
oh, i missed this

https://codereview.chromium.org/1000163002/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_row.js:121: var isActive =
this.contains(element);
and this

[Dan Beam, 2015-03-12 20:20:21]

i think eventually focusrow should be able to deal with elements outside of its
sub-DOM.  it's not there yet, but we're getting closer.

[hcarmona, 2015-03-12 20:54:51]

lgtm

[Dan Beam, 2015-03-12 23:48:37]

dbeam@chromium.org changed reviewers:
+ asanka@chromium.org

[Dan Beam, 2015-03-12 23:48:37]

hcarmona@: updated, ptal

+asanka@ (hcarmona@'s not a committer yet)

[asanka, 2015-03-13 14:38:06]

rubberstamp LGTM

[hcarmona, 2015-03-14 00:55:03]

https://codereview.chromium.org/1000163002/diff/160001/ui/webui/resources/js/...
File ui/webui/resources/js/cr/ui/focus_grid.js (right):

https://codereview.chromium.org/1000163002/diff/160001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_grid.js:160: row.makeActive(false);
This will always be a no-op because of the change to |makeActive| and can be
removed.

https://codereview.chromium.org/1000163002/diff/160001/ui/webui/resources/js/...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/1000163002/diff/160001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_row.js:107: 
element.tabIndex = -1;

Because we are no longer updating this when a row is added, it's necessary in
order to avoid each element being tab-able.

[Dan Beam, 2015-03-14 02:54:17]

https://codereview.chromium.org/1000163002/diff/160001/ui/webui/resources/js/...
File ui/webui/resources/js/cr/ui/focus_grid.js (right):

https://codereview.chromium.org/1000163002/diff/160001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_grid.js:160: row.makeActive(false);
On 2015/03/14 00:55:02, Hector Carmona wrote:
> This will always be a no-op because of the change to |makeActive| and can be
> removed.

Done.

https://codereview.chromium.org/1000163002/diff/160001/ui/webui/resources/js/...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/1000163002/diff/160001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_row.js:107: 
On 2015/03/14 00:55:02, Hector Carmona wrote:
> element.tabIndex = -1;
> 
> Because we are no longer updating this when a row is added, it's necessary in
> order to avoid each element being tab-able.

Done. (kinda)

[hcarmona, 2015-03-16 17:22:22]

lgtm

[Dan Beam, 2015-03-16 17:57:43]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-03-16 17:57:43]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/1000163002/#ps180001
(title: "hcarmona@ review")

[commit-bot: I haz the power, 2015-03-16 17:58:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1000163002/180001

[commit-bot: I haz the power, 2015-03-16 18:59:05]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-03-16 19:00:01]

Patchset 9 (id:??) landed as
https://crrev.com/11940994a992a8531150ab256fc6e711df63d142
Cr-Commit-Position: refs/heads/master@{#320762}