MD History: Show sidebar next to content on wide screens

MD History: Show sidebar next to content on wide screens

This ensures that the sidebar is always visible when the screen has
enough space, and hides it within a drawer otherwise. To achieve this,
we split <history-side-bar> apart so that it is no longer responsible
for the drawer. We also add extra capabilities to <cr-toolbar> to
support hiding the menu button and adjusting the margins.

These changes break the focus trapping inside the drawer (which already
didn't work in certain situations). This will be addressed in a follow-up.

BUG=608968
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/5844d81e3382eaabda8f366eafb424050a47ec52
Cr-Commit-Position: refs/heads/master@{#408572}

[tsergeant, 2016-07-20 07:30:46]

Description was changed from

==========
MD History: Show sidebar next to content on wide screens

This ensures that the sidebar is always visible when the screen has
enough space, and hides it within a drawer otherwise. To achieve this,
we split <history-side-bar> apart so that it is no longer responsible
for the drawer. We also add extra capabilities to <cr-toolbar> to
support hiding the menu button and adjusting the margins.

These changes break the focus trapping inside the drawer (which already
didn't work in certain situations). This will be addressed in a follow-up.

BUG=608968
==========

to

==========
MD History: Show sidebar next to content on wide screens

This ensures that the sidebar is always visible when the screen has
enough space, and hides it within a drawer otherwise. To achieve this,
we split <history-side-bar> apart so that it is no longer responsible
for the drawer. We also add extra capabilities to <cr-toolbar> to
support hiding the menu button and adjusting the margins.

These changes break the focus trapping inside the drawer (which already
didn't work in certain situations). This will be addressed in a follow-up.

BUG=608968
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2016-07-22 08:52:43]

Patchset #2 (id:20001) has been deleted

[tsergeant, 2016-07-22 08:52:51]

Patchset #2 (id:40001) has been deleted

[tsergeant, 2016-07-22 08:56:55]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 08:57:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 09:30:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 09:30:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tsergeant, 2016-07-25 01:19:24]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 01:19:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 02:16:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 02:16:40]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2016-07-25 03:09:57]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[tsergeant, 2016-07-25 03:09:58]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-07-25 03:10:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tsergeant, 2016-07-25 03:38:53]

Patchset #3 (id:80001) has been deleted

[tsergeant, 2016-07-25 03:51:43]

tsergeant@chromium.org changed reviewers:
+ calamity@chromium.org

[commit-bot: I haz the power, 2016-07-25 04:06:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 04:06:02]

Dry run: This issue passed the CQ dry run.

[calamity, 2016-07-26 05:05:18]

https://codereview.chromium.org/2165903003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_history/app.html (right):

https://codereview.chromium.org/2165903003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.html:51: border-bottom: 1px solid
rgba(0, 0, 0, 0.08);
Is this supposed to align to the bottom of the toolbar? It's sitting a pixel
below it with box-sizing.

https://codereview.chromium.org/2165903003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2165903003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:98: var drawer = this.$$('#drawer');
Under what circumstances does this get called without the menu existing?

https://codereview.chromium.org/2165903003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_toolbar.js (right):

https://codereview.chromium.org/2165903003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_toolbar.js:135: this.updateStyles();
Why is this necessary? Shouldn't the attribute reflection just update the styles
accordingly?

https://codereview.chromium.org/2165903003/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_drawer_test.js (right):

https://codereview.chromium.org/2165903003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_drawer_test.js:11: var drawerSideBar;
Last 2 unused.

[tsergeant, 2016-07-26 05:59:04]

https://codereview.chromium.org/2165903003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_history/app.html (right):

https://codereview.chromium.org/2165903003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.html:51: border-bottom: 1px solid
rgba(0, 0, 0, 0.08);
On 2016/07/26 05:05:18, calamity wrote:
> Is this supposed to align to the bottom of the toolbar? It's sitting a pixel
> below it with box-sizing.

I think the current behavior is correct. It matches Settings and (AFAICT) the
mocks.

https://codereview.chromium.org/2165903003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2165903003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:98: var drawer = this.$$('#drawer');
On 2016/07/26 05:05:18, calamity wrote:
> Under what circumstances does this get called without the menu existing?

None currently, but I'm just being cautious.

https://codereview.chromium.org/2165903003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_toolbar.js (right):

https://codereview.chromium.org/2165903003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_toolbar.js:135: this.updateStyles();
On 2016/07/26 05:05:18, calamity wrote:
> Why is this necessary? Shouldn't the attribute reflection just update the
styles
> accordingly?

Nope. The polymer custom style shim isn't always able to propagate dynamic
changes automatically
(https://www.polymer-project.org/1.0/docs/devguide/styling#custom-properties-s...).

I think this will be unnecessary when we turn native custom properties on,
though.

https://codereview.chromium.org/2165903003/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_drawer_test.js (right):

https://codereview.chromium.org/2165903003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_drawer_test.js:11: var drawerSideBar;
On 2016/07/26 05:05:18, calamity wrote:
> Last 2 unused.

Done.

[tsergeant, 2016-07-28 00:04:37]

Ping, can you take another look at this please?

[calamity, 2016-07-28 03:15:46]

lgtm

[tsergeant, 2016-07-28 03:31:54]

tsergeant@chromium.org changed reviewers:
+ dpapad@chromium.org

[tsergeant, 2016-07-28 03:31:55]

+dpapad for cr_elements

[dpapad, 2016-07-28 23:17:12]

I was not able to patch this CL locally due to conflits. Does it have any visual
impact on the toolbar of MD Settings page?

https://codereview.chromium.org/2165903003/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_drawer_test.js (right):

https://codereview.chromium.org/2165903003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_drawer_test.js:23:
assertNotEquals(null, drawer);
Nit (optional): assertTrue(!!drawer);

[tsergeant, 2016-07-29 01:38:02]

Patchset #6 (id:180001) has been deleted

[tsergeant, 2016-07-29 01:39:52]

There are no visible changes to settings. I've rebased, so you should be able to
patch in now.

https://codereview.chromium.org/2165903003/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_drawer_test.js (right):

https://codereview.chromium.org/2165903003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_drawer_test.js:23:
assertNotEquals(null, drawer);
On 2016/07/28 23:17:12, dpapad wrote:
> Nit (optional): assertTrue(!!drawer);

Done.

https://codereview.chromium.org/2165903003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_toolbar.html (right):

https://codereview.chromium.org/2165903003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_toolbar.html:45:
--cr-toolbar-field-margin: 0;
I had to change how this property was applied. Seems like the upgrade to Polymer
1.6.0 broke this? Anyway, it's effectively the same as the old way.

(both the old way and the new way still work with native custom props enabled)

[dpapad, 2016-07-29 01:45:58]

LGTM with optional comment.

https://codereview.chromium.org/2165903003/diff/200001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_drawer_test.js (right):

https://codereview.chromium.org/2165903003/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_drawer_test.js:17: app.selectedPage_ =
'syncedTabs';
Accessing private properties is usually a sign that the test is too coupled with
the current implementation instead of testing publicly exposed behavior.

[tsergeant, 2016-07-29 02:47:54]

https://codereview.chromium.org/2165903003/diff/200001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_drawer_test.js (right):

https://codereview.chromium.org/2165903003/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_drawer_test.js:17: app.selectedPage_ =
'syncedTabs';
On 2016/07/29 01:45:58, dpapad wrote:
> Accessing private properties is usually a sign that the test is too coupled
with
> the current implementation instead of testing publicly exposed behavior.

Acknowledged.

[tsergeant, 2016-07-29 02:48:00]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2016-07-29 02:48:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
calamity@chromium.org
Link to the patchset: https://codereview.chromium.org/2165903003/#ps200001
(title: "dpapad@ review")

[commit-bot: I haz the power, 2016-07-29 02:48:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 03:56:25]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2016-07-29 03:58:14]

Description was changed from

==========
MD History: Show sidebar next to content on wide screens

This ensures that the sidebar is always visible when the screen has
enough space, and hides it within a drawer otherwise. To achieve this,
we split <history-side-bar> apart so that it is no longer responsible
for the drawer. We also add extra capabilities to <cr-toolbar> to
support hiding the menu button and adjusting the margins.

These changes break the focus trapping inside the drawer (which already
didn't work in certain situations). This will be addressed in a follow-up.

BUG=608968
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD History: Show sidebar next to content on wide screens

This ensures that the sidebar is always visible when the screen has
enough space, and hides it within a drawer otherwise. To achieve this,
we split <history-side-bar> apart so that it is no longer responsible
for the drawer. We also add extra capabilities to <cr-toolbar> to
support hiding the menu button and adjusting the margins.

These changes break the focus trapping inside the drawer (which already
didn't work in certain situations). This will be addressed in a follow-up.

BUG=608968
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/5844d81e3382eaabda8f366eafb424050a47ec52
Cr-Commit-Position: refs/heads/master@{#408572}
==========

[commit-bot: I haz the power, 2016-07-29 03:58:16]

Patchset 6 (id:??) landed as
https://crrev.com/5844d81e3382eaabda8f366eafb424050a47ec52
Cr-Commit-Position: refs/heads/master@{#408572}