Make cr.ui.Focus* play nice with Shadow DOM and MD chrome://downloads

Make cr.ui.Focus* play nice with Shadow DOM and MD chrome://downloads

R=hcarmona@chromium.org
BUG=425626
Committed: https://crrev.com/5d5147694e7b8179ef9014bcf3a021c233be7b46
Cr-Commit-Position: refs/heads/master@{#345257}

[Dan Beam, 2015-08-22 03:31:22]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2015-08-24 19:39:21]

Patchset #1 (id:20001) has been deleted

[Dan Beam, 2015-08-24 19:48:46]

Patchset #2 (id:60001) has been deleted

[hcarmona, 2015-08-24 21:00:38]

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/focus_row.js (right):

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_downloads/focus_row.js:24:
this.addItem('controlled-by', '#controlled-by a');
Why no assert for this?

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/manager.js (right):

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:105: if (item == activeItem &&
!cr.ui.FocusRow.isFocusable(activeControl))
When would you get an active control that isn't focusable?

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:193:
Promise.all(allReady).then(this.rebuildFocusGrid_.bind(this));
Is it possible to update the row rather than rebuild the grid each time
an item is ready?

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

https://codereview.chromium.org/1313453002/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_row.js:251: e.currentTarget.tabIndex = 0;
Will focus be automatically added in this case?

[Dan Beam, 2015-08-24 22:59:30]

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/focus_row.js (right):

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_downloads/focus_row.js:24:
this.addItem('controlled-by', '#controlled-by a');
On 2015/08/24 21:00:37, Hector Carmona wrote:
> Why no assert for this?

it's not guaranteed to exist

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/manager.js (right):

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:105: if (item == activeItem &&
!cr.ui.FocusRow.isFocusable(activeControl))
On 2015/08/24 21:00:37, Hector Carmona wrote:
> When would you get an active control that isn't focusable?

enter on "Pause" hides and "Resume" is shown

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:193:
Promise.all(allReady).then(this.rebuildFocusGrid_.bind(this));
On 2015/08/24 21:00:37, Hector Carmona wrote:
> Is it possible to update the row rather than rebuild the grid each time
> an item is ready?

yes, but until there's a reason to, it just complicates the code

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

https://codereview.chromium.org/1313453002/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_row.js:251: e.currentTarget.tabIndex = 0;
On 2015/08/24 21:00:37, Hector Carmona wrote:
> Will focus be automatically added in this case?

yes, 'mousedown' is handled before an element is considered for focus

[hcarmona, 2015-08-24 23:49:20]

lgtm

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/focus_row.js (right):

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_downloads/focus_row.js:24:
this.addItem('controlled-by', '#controlled-by a');
On 2015/08/24 22:59:30, Dan Beam wrote:
> On 2015/08/24 21:00:37, Hector Carmona wrote:
> > Why no assert for this?
> 
> it's not guaranteed to exist

Acknowledged.

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/manager.js (right):

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:105: if (item == activeItem &&
!cr.ui.FocusRow.isFocusable(activeControl))
On 2015/08/24 22:59:30, Dan Beam wrote:
> On 2015/08/24 21:00:37, Hector Carmona wrote:
> > When would you get an active control that isn't focusable?
> 
> enter on "Pause" hides and "Resume" is shown

Acknowledged.

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:193:
Promise.all(allReady).then(this.rebuildFocusGrid_.bind(this));
On 2015/08/24 22:59:30, Dan Beam wrote:
> On 2015/08/24 21:00:37, Hector Carmona wrote:
> > Is it possible to update the row rather than rebuild the grid each time
> > an item is ready?
> 
> yes, but until there's a reason to, it just complicates the code

One problem from the extensions page was that focus would be lost when
the grid was rebuilt. This made it hard to use with the keyboard.

I don't think your code has this issue because you're not changing the
elements, just decorating them.

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

https://codereview.chromium.org/1313453002/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_row.js:251: e.currentTarget.tabIndex = 0;
On 2015/08/24 22:59:30, Dan Beam wrote:
> On 2015/08/24 21:00:37, Hector Carmona wrote:
> > Will focus be automatically added in this case?
> 
> yes, 'mousedown' is handled before an element is considered for focus

Acknowledged.

[Dan Beam, 2015-08-25 03:22:30]

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/manager.js (right):

https://codereview.chromium.org/1313453002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:193:
Promise.all(allReady).then(this.rebuildFocusGrid_.bind(this));
On 2015/08/24 23:49:19, Hector Carmona wrote:
> On 2015/08/24 22:59:30, Dan Beam wrote:
> > On 2015/08/24 21:00:37, Hector Carmona wrote:
> > > Is it possible to update the row rather than rebuild the grid each time
> > > an item is ready?
> > 
> > yes, but until there's a reason to, it just complicates the code
> 
> One problem from the extensions page was that focus would be lost when
> the grid was rebuilt.

I know, I fixed these issues recently ;)

> This made it hard to use with the keyboard.

that's why we store the active control before we destroy() the grid

> 
> I don't think your code has this issue because you're not changing the
> elements, just decorating them.

right

[Dan Beam, 2015-08-25 03:22:33]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2015-08-25 03:22:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313453002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313453002/80001

[commit-bot: I haz the power, 2015-08-25 04:47:56]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2015-08-25 04:48:53]

Patchset 2 (id:??) landed as
https://crrev.com/5d5147694e7b8179ef9014bcf3a021c233be7b46
Cr-Commit-Position: refs/heads/master@{#345257}