[Media Router WebUI] Move dialog load focusing to container.

[Media Router WebUI] Move dialog load focusing to container.

This ensures the first run flow is taken into account, since those elements may appear before the header elements in the DOM tree.

Manually tested on Linux/Mac.

BUG=598278
Committed: https://crrev.com/f6a6cb0dd0ffda3cdfe66c5676072cfdea25794f
Cr-Commit-Position: refs/heads/master@{#385213}

[apacible, 2016-04-02 08:26:29]

Description was changed from

==========
asdf

BUG=
==========

to

==========
[Media Router WebUI] Move dialog load focusing from header to container.

This ensures the first run flow is taken into account, since those elements may
appear before the header elements in the DOM tree.

BUG=598278
==========

[apacible, 2016-04-02 12:47:30]

Patchset #1 (id:1) has been deleted

[apacible, 2016-04-03 03:41:02]

Patchset #1 (id:20001) has been deleted

[apacible, 2016-04-03 03:41:08]

Patchset #1 (id:40001) has been deleted

[apacible, 2016-04-03 03:49:15]

Patchset #1 (id:60001) has been deleted

[apacible, 2016-04-04 16:08:38]

apacible@chromium.org changed reviewers:
+ mfoltz@chromium.org

[apacible, 2016-04-04 16:08:55]

Description was changed from

==========
[Media Router WebUI] Move dialog load focusing from header to container.

This ensures the first run flow is taken into account, since those elements may
appear before the header elements in the DOM tree.

BUG=598278
==========

to

==========
[Media Router WebUI] Move dialog load focusing to container.

This ensures the first run flow is taken into account, since those elements may
appear before the header elements in the DOM tree.

BUG=598278
==========

[apacible, 2016-04-04 16:09:13]

PTAL, thanks!

[mark a. foltz, 2016-04-04 20:40:37]

LGTM with some clarifications requested.

The patch description might go into a bit more detail that this is a workaround
for Polymer's Mac-specific focus behavior.

https://codereview.chromium.org/1856553002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1856553002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1351:
// If the focus event was not triggered by the user, remove focus from
can you be more specific, i.e. "was automatically fired by Polymer"

https://codereview.chromium.org/1856553002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1353:
// initially loaded. This only happens on mac.
I assume there is an issue for this in the Polymer GitHub tracker?

Please file a cleanup bug to remove this workaround when the upstream bug has
been fixed & merged into Chromium and leave a TODO(crbug/XXXX)

https://codereview.chromium.org/1856553002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1362:
if (placeholder && placeholder == document.activeElement) {
Should you check that the source of |event| is |placeholder|?
Or is that guaranteed to be document.activeElement in the onFocus event handler?

[apacible, 2016-04-05 17:15:06]

Description was changed from

==========
[Media Router WebUI] Move dialog load focusing to container.

This ensures the first run flow is taken into account, since those elements may
appear before the header elements in the DOM tree.

BUG=598278
==========

to

==========
[Media Router WebUI] Move dialog load focusing to container.

This ensures the first run flow is taken into account, since those elements may
appear before the header elements in the DOM tree.

Manually tested on Linux/Mac.

BUG=598278
==========

[apacible, 2016-04-05 17:30:33]

Also manually tested on Mac and Linux.

https://codereview.chromium.org/1856553002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1856553002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1351:
// If the focus event was not triggered by the user, remove focus from
On 2016/04/04 20:40:36, mark a. foltz wrote:
> can you be more specific, i.e. "was automatically fired by Polymer"

Done.

https://codereview.chromium.org/1856553002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1353:
// initially loaded. This only happens on mac.
On 2016/04/04 20:40:36, mark a. foltz wrote:
> I assume there is an issue for this in the Polymer GitHub tracker?
> 
> Please file a cleanup bug to remove this workaround when the upstream bug has
> been fixed & merged into Chromium and leave a TODO(crbug/XXXX)

No, this is WAI from the Polymer side. I'm not sure why it only happens on Mac
for MR, since I see this behavior on sites/apps implemented with Polymer on both
Linux/Mac (also WAI, by design). Though I do think it's cleaner to not have the
auto-focus since it's not user initiated.

https://codereview.chromium.org/1856553002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1362:
if (placeholder && placeholder == document.activeElement) {
On 2016/04/04 20:40:36, mark a. foltz wrote:
> Should you check that the source of |event| is |placeholder|?
> Or is that guaranteed to be document.activeElement in the onFocus event
handler?

It would be guaranteed to be this.shadowRoot.activeElement, not
document.activeElement. Updated to reflect this.

[apacible, 2016-04-05 17:30:38]

The CQ bit was checked by apacible@chromium.org

[apacible, 2016-04-05 17:30:39]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/1856553002/#ps100001
(title: "Changes per mfoltz@'s comments.")

[commit-bot: I haz the power, 2016-04-05 17:30:57]

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

[commit-bot: I haz the power, 2016-04-05 17:37:54]

Description was changed from

==========
[Media Router WebUI] Move dialog load focusing to container.

This ensures the first run flow is taken into account, since those elements may
appear before the header elements in the DOM tree.

Manually tested on Linux/Mac.

BUG=598278
==========

to

==========
[Media Router WebUI] Move dialog load focusing to container.

This ensures the first run flow is taken into account, since those elements may
appear before the header elements in the DOM tree.

Manually tested on Linux/Mac.

BUG=598278
==========

[commit-bot: I haz the power, 2016-04-05 17:37:55]

Committed patchset #2 (id:100001)

[commit-bot: I haz the power, 2016-04-05 17:39:00]

Description was changed from

==========
[Media Router WebUI] Move dialog load focusing to container.

This ensures the first run flow is taken into account, since those elements may
appear before the header elements in the DOM tree.

Manually tested on Linux/Mac.

BUG=598278
==========

to

==========
[Media Router WebUI] Move dialog load focusing to container.

This ensures the first run flow is taken into account, since those elements may
appear before the header elements in the DOM tree.

Manually tested on Linux/Mac.

BUG=598278

Committed: https://crrev.com/f6a6cb0dd0ffda3cdfe66c5676072cfdea25794f
Cr-Commit-Position: refs/heads/master@{#385213}
==========

[commit-bot: I haz the power, 2016-04-05 17:39:02]

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