DOMUI Prefs: Keep focus within the topmost page/overlay

DOMUI Prefs: Keep focus within the topmost page/overlay


BUG=60176, 67026
TEST=Open a subpage or overlay, and use tab to cycle through the elements; only elements within the page/overlay should be focused.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72653

[James Hawkins, 2011-01-26 01:29:40]

http://codereview.chromium.org/6315012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/6315012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:362:
document.addEventListener('focus', this.manageFocusChange_.bind(this),
nit: All params on one line or lined up in one column.

http://codereview.chromium.org/6315012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:394: var tabLoopRoot = null;
This var name is a little confusing.

http://codereview.chromium.org/6315012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:395: // If an overlay is
visible, that defines the tab loop
nit: Period at end of sentence.

http://codereview.chromium.org/6315012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:407: if (tabLoopRoot &&
!tabLoopRoot.contains(e.target)) {
nit: No need for braces.

[stuartmorgan, 2011-01-26 17:27:03]

http://codereview.chromium.org/6315012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/6315012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:362:
document.addEventListener('focus', this.manageFocusChange_.bind(this),
On 2011/01/26 01:29:41, James Hawkins wrote:
> nit: All params on one line or lined up in one column.

The style guide explicitly allows either form.

http://codereview.chromium.org/6315012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:394: var tabLoopRoot = null;
On 2011/01/26 01:29:41, James Hawkins wrote:
> This var name is a little confusing.

Changed to focusableItemsRoot; hopefully that's a bit clearer.

http://codereview.chromium.org/6315012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:395: // If an overlay is
visible, that defines the tab loop
On 2011/01/26 01:29:41, James Hawkins wrote:
> nit: Period at end of sentence.

Done.

http://codereview.chromium.org/6315012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:407: if (tabLoopRoot &&
!tabLoopRoot.contains(e.target)) {
On 2011/01/26 01:29:41, James Hawkins wrote:
> nit: No need for braces.

Done.

[arv (Not doing code reviews), 2011-01-26 17:55:49]

LGTM

http://codereview.chromium.org/6315012/diff/5001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/6315012/diff/5001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:394: var focusableItemsRoot =
null;
no need to assign null here.

http://codereview.chromium.org/6315012/diff/5001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:585: focusElement =
this.pageDiv.querySelector('button, input, list, select');
missing var

[James Hawkins, 2011-01-26 18:08:24]

LGTM

http://codereview.chromium.org/6315012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/6315012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:362:
document.addEventListener('focus', this.manageFocusChange_.bind(this),
On 2011/01/26 17:27:03, stuartmorgan wrote:
> On 2011/01/26 01:29:41, James Hawkins wrote:
> > nit: All params on one line or lined up in one column.
> 
> The style guide explicitly allows either form.

Interesting. I had been told otherwise by a reputable source, but I now see in
the style guide that I was incorrect.

[stuartmorgan, 2011-01-26 18:19:08]

http://codereview.chromium.org/6315012/diff/5001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/6315012/diff/5001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:394: var focusableItemsRoot =
null;
On 2011/01/26 17:55:49, arv wrote:
> no need to assign null here.

Done.

http://codereview.chromium.org/6315012/diff/5001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:585: focusElement =
this.pageDiv.querySelector('button, input, list, select');
On 2011/01/26 17:55:49, arv wrote:
> missing var

Done.