extensions: fix focus management for all dialogs (not just options).

extensions: fix focus management for all dialogs (not just options).

R=kalman@chromium.org
BUG=438301, 438777
Committed: https://crrev.com/54ea920b092bee8ddcfea3462dd23b406eed80de
Cr-Commit-Position: refs/heads/master@{#315126}

[Dan Beam, 2015-02-06 01:18:05]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2015-02-06 01:22:35]

Patchset #1 (id:20001) has been deleted

[not at google - send to devlin, 2015-02-06 21:31:39]

https://codereview.chromium.org/862403006/diff/40001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_options_overlay.js (right):

https://codereview.chromium.org/862403006/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_options_overlay.js:184:
this.setVisible_(true);
Unfortunately showing the overlay early serves a purpose - the extensionoptions
element grows to fit the width of the dialogue, and then the preferred size
logic kicks in, and we resize the dialogue accordingly. Importantly, the width
starts off at the width of the dialogue.

Showing it late means that on construction the width is 0 so the initial
preferred size rendering is like:

This
Is
An
Options
Page

rather than:

This Is An
Options Page

... or whatever. This is similar to crbug.com/431001. The point is, it has a
huge height, so the overlay sizes itself large, but never shrinks. You end up
getting a really tall overlay with a lot of whitespace at the bottom.

If you want to test this, grab
https://github.com/kalman/chrome-extensions/tree/master/browser-clock and change
the manifest from {"options_page": "options.html"} to {"options_ui": {"page":
"options.html"}}.

The real way to fix all of this - which would also address crbug.com/408274 I
think - is to attach the ExtensionOptions element off-screen to something, wait
for it to render (pulling the initial width from somewhere, wherever the
dialogue gets it from) detatch() it (not available yet, see
https://codereview.chromium.org/867393002/), then re-attach it, then set the
dialogue to visible.

I understand if you don't want to do this now.

https://codereview.chromium.org/862403006/diff/40001/chrome/browser/resources...
File chrome/browser/resources/extensions/extensions.js (right):

https://codereview.chromium.org/862403006/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/extensions.js:377: * @param {HTMLElement}
node The overlay page to show. If falsey, all overlays
You might as well fix this comment - |node| certainly can't be "falsey" (unless
it's some kind of JS term for something-resembling-false), hopefully just "null"
so that the {HTMLElement} annotation holds.

https://codereview.chromium.org/862403006/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/extensions.js:394: lastFocused.focus();
Correct me if I'm wrong (in which case I'm trying this patch out wrong), but
this won't actually work until the patch for not rebuilding the page goes in.
Currently, when the options dialogue is closed, it closes the options page which
causes a View change in the extension which causes the extensions page to
re-render, killing focus anyway.

Still seems like the right change to make, though.

[Dan Beam, 2015-02-06 21:50:02]

https://codereview.chromium.org/862403006/diff/40001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_options_overlay.js (right):

https://codereview.chromium.org/862403006/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_options_overlay.js:184:
this.setVisible_(true);
On 2015/02/06 21:31:38, kalman wrote:
> Unfortunately showing the overlay early serves a purpose - the
extensionoptions
> element grows to fit the width of the dialogue, and then the preferred size
> logic kicks in, and we resize the dialogue accordingly. Importantly, the width
> starts off at the width of the dialogue.
> 
> Showing it late means that on construction the width is 0 so the initial
> preferred size rendering is like:
> 
> This
> Is
> An
> Options
> Page
> 
> rather than:
> 
> This Is An
> Options Page
> 
> ... or whatever. This is similar to crbug.com/431001. The point is, it has a
> huge height, so the overlay sizes itself large, but never shrinks. You end up
> getting a really tall overlay with a lot of whitespace at the bottom.
> 
> If you want to test this, grab
> https://github.com/kalman/chrome-extensions/tree/master/browser-clock and
change
> the manifest from {"options_page": "options.html"} to {"options_ui": {"page":
> "options.html"}}.
> 
> The real way to fix all of this - which would also address crbug.com/408274 I
> think - is to attach the ExtensionOptions element off-screen to something,
wait
> for it to render (pulling the initial width from somewhere, wherever the
> dialogue gets it from) detatch() it (not available yet, see
> https://codereview.chromium.org/867393002/), then re-attach it, then set the
> dialogue to visible.
> 
> I understand if you don't want to do this now.

i don't ;), moved back

https://codereview.chromium.org/862403006/diff/40001/chrome/browser/resources...
File chrome/browser/resources/extensions/extensions.js (right):

https://codereview.chromium.org/862403006/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/extensions.js:377: * @param {HTMLElement}
node The overlay page to show. If falsey, all overlays
On 2015/02/06 21:31:38, kalman wrote:
> You might as well fix this comment - |node| certainly can't be "falsey"
(unless
> it's some kind of JS term for something-resembling-false), hopefully just
"null"
> so that the {HTMLElement} annotation holds.

Done.

https://codereview.chromium.org/862403006/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/extensions.js:394: lastFocused.focus();
On 2015/02/06 21:31:38, kalman wrote:
> Correct me if I'm wrong (in which case I'm trying this patch out wrong), but
> this won't actually work until the patch for not rebuilding the page goes in.
> Currently, when the options dialogue is closed, it closes the options page
which
> causes a View change in the extension which causes the extensions page to
> re-render, killing focus anyway.

i know

> 
> Still seems like the right change to make, though.

right

[not at google - send to devlin, 2015-02-06 21:59:16]

lgtm

[Dan Beam, 2015-02-06 22:02:45]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2015-02-06 22:03:10]

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

[commit-bot: I haz the power, 2015-02-06 23:05:11]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-02-06 23:06:06]

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