MD History: Add responsive layout which hides the sidebar in thin windows

MD History: Hide sidebar behind toolbar menu button

This hides the sidebar and displays a menu button in the toolbar which
opens it in an overlay drawer on the left side of the screen. Also
updates the sidebar menu to have better tab focusing behavior.

This causes the search field in the toolbar to appear slightly
off-center, this will be fixed in a follow-up patch.

BUG=608968, 620030
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/105cabbb6d80245ba69e9097013594a03069f8cc
Cr-Commit-Position: refs/heads/master@{#403098}

[tsergeant, 2016-05-30 07:16:19]

Description was changed from

==========
MD History: Add responsive layout which hides the sidebar in thin windows

When the window is less than 900px wide, this hides the sidebar and
displays a menu button in the <cr-toolbar>. When clicked, a drawer opens
from the left side of the screen containing the sidebar.

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

to

==========
MD History: Add responsive layout which hides the sidebar in thin windows

When the window is less than 900px wide, this hides the sidebar and
displays a menu button in the <cr-toolbar>. When clicked, a drawer opens
from the left side of the screen containing the sidebar.

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

[tsergeant, 2016-05-30 07:28:58]

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

[tsergeant, 2016-05-30 07:28:58]

+calamity@ for initial review

[tsergeant, 2016-05-31 07:27:48]

On 2016/05/30 07:28:58, tsergeant wrote:
> +calamity@ for initial review

Update: I'm revoking my PTAL while I figure out how focus should work inside the
sidebar.

[tsergeant, 2016-06-08 07:53:35]

On 2016/05/31 07:27:48, tsergeant wrote:
> On 2016/05/30 07:28:58, tsergeant wrote:
> > +calamity@ for initial review
> 
> Update: I'm revoking my PTAL while I figure out how focus should work inside
the
> sidebar.

Further update: I've made the changes necessary to hide the toolbar by default,
but the search field is no longer centered correctly, and centering it is
proving...difficult.

[tsergeant, 2016-06-10 05:52:56]

Description was changed from

==========
MD History: Add responsive layout which hides the sidebar in thin windows

When the window is less than 900px wide, this hides the sidebar and
displays a menu button in the <cr-toolbar>. When clicked, a drawer opens
from the left side of the screen containing the sidebar.

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

to

==========
MD History: Add responsive layout which hides the sidebar in thin windows

When the window is less than 900px wide, this hides the sidebar and
displays a menu button in the <cr-toolbar>. When clicked, a drawer opens
from the left side of the screen containing the sidebar.

BUG=608968
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
COLLABORATORS=calamity@chromium.org
==========

[tsergeant, 2016-06-10 05:55:40]

Description was changed from

==========
MD History: Add responsive layout which hides the sidebar in thin windows

When the window is less than 900px wide, this hides the sidebar and
displays a menu button in the <cr-toolbar>. When clicked, a drawer opens
from the left side of the screen containing the sidebar.

BUG=608968
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
COLLABORATORS=calamity@chromium.org
==========

to

==========
MD History: Add responsive layout which hides the sidebar in thin windows

When the window is less than 900px wide, this hides the sidebar and
displays a menu button in the <cr-toolbar>. When clicked, a drawer opens
from the left side of the screen containing the sidebar.

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

[tsergeant, 2016-06-15 07:18:29]

Description was changed from

==========
MD History: Add responsive layout which hides the sidebar in thin windows

When the window is less than 900px wide, this hides the sidebar and
displays a menu button in the <cr-toolbar>. When clicked, a drawer opens
from the left side of the screen containing the sidebar.

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

to

==========
MD History: Hide sidebar behind toolbar menu button

This hides the sidebar and displays a menu button in the toolbar which
opens it in an overlay drawer on the left side of the screen. Also
changes the layout of the toolbar  so that the search field remains
centered with respect to the content, which is no longer offset right by
the width of the toolbar. Finally, also updates the sidebar menu to have
better tab focusing behavior.

BUG=608968,620030
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2016-06-15 07:26:29]

Patchset #2 (id:20001) has been deleted

[tsergeant, 2016-06-15 07:26:37]

Patchset #5 (id:100001) has been deleted

[tsergeant, 2016-06-15 07:26:57]

calamity@:

Alright, have at it.

[calamity, 2016-06-17 03:02:01]

https://codereview.chromium.org/2020963002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_history/compiled_resources2.gyp (right):

https://codereview.chromium.org/2020963002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/compiled_resources2.gyp:93:
'<(DEPTH)/third_party/polymer/v1_0/components-chromium/app-layout/app-drawer/compiled_resources2.gyp:app-drawer-extracted',
nit: sorting.

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

https://codereview.chromium.org/2020963002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.html:41: right: 0;
I had no idea you could do this.

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

https://codereview.chromium.org/2020963002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/side_bar.js:23: // focus to the selected
item.
I don't fully understand this comment. What problems does focusing the first
focusable child have?

https://codereview.chromium.org/2020963002/diff/120001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right):

https://codereview.chromium.org/2020963002/diff/120001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:43: resizeListener_:
null,
Can you just set the value here?

https://codereview.chromium.org/2020963002/diff/120001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:64: * @private
This insanity deserves a comment.

// Sets position of the search field based on its show state.

In fact, this method should probably be renamed to setSearchFieldPosition_ and
the one below should be updateSearchFieldPositionNoAnimate_ or similar.

https://codereview.chromium.org/2020963002/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html
(right):

https://codereview.chromium.org/2020963002/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:95:
width: calc(100% - 12px);
Pull 12px into a css var.

[tsergeant, 2016-06-20 05:05:47]

Patchset #8 (id:180001) has been deleted

[tsergeant, 2016-06-20 05:05:57]

https://codereview.chromium.org/2020963002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_history/compiled_resources2.gyp (right):

https://codereview.chromium.org/2020963002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/compiled_resources2.gyp:93:
'<(DEPTH)/third_party/polymer/v1_0/components-chromium/app-layout/app-drawer/compiled_resources2.gyp:app-drawer-extracted',
On 2016/06/17 03:02:00, calamity wrote:
> nit: sorting.

Done.

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

https://codereview.chromium.org/2020963002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.html:41: right: 0;
On 2016/06/17 03:02:00, calamity wrote:
> I had no idea you could do this.

This whole layout was a bit weeiiiird, and has been replaced with something
simpler in https://codereview.chromium.org/2073703002/

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

https://codereview.chromium.org/2020963002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/side_bar.js:23: // focus to the selected
item.
On 2016/06/17 03:02:00, calamity wrote:
> I don't fully understand this comment. What problems does focusing the first
> focusable child have?

I've expanded the comment to try and explain it. Let me know if I need to keep
tweaking it.

https://codereview.chromium.org/2020963002/diff/120001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right):

https://codereview.chromium.org/2020963002/diff/120001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:43: resizeListener_:
null,
On 2016/06/17 03:02:00, calamity wrote:
> Can you just set the value here?

Nope, this.setSearchFieldPosition_ is undefined at this point.

https://codereview.chromium.org/2020963002/diff/120001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:64: * @private
On 2016/06/17 03:02:00, calamity wrote:
> This insanity deserves a comment.
> 
> // Sets position of the search field based on its show state.
> 
> In fact, this method should probably be renamed to setSearchFieldPosition_ and
> the one below should be updateSearchFieldPositionNoAnimate_ or similar.

Done. I went with updateSearchFieldPosition_ and
updateSearchFieldPositionNoAnimate_.

https://codereview.chromium.org/2020963002/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html
(right):

https://codereview.chromium.org/2020963002/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:95:
width: calc(100% - 12px);
On 2016/06/17 03:02:00, calamity wrote:
> Pull 12px into a css var.

Done.

[tsergeant, 2016-06-23 03:55:22]

Description was changed from

==========
MD History: Hide sidebar behind toolbar menu button

This hides the sidebar and displays a menu button in the toolbar which
opens it in an overlay drawer on the left side of the screen. Also
changes the layout of the toolbar  so that the search field remains
centered with respect to the content, which is no longer offset right by
the width of the toolbar. Finally, also updates the sidebar menu to have
better tab focusing behavior.

BUG=608968,620030
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD History: Hide sidebar behind toolbar menu button

This hides the sidebar and displays a menu button in the toolbar which
opens it in an overlay drawer on the left side of the screen. Also
updates the sidebar menu to have better tab focusing behavior.

This causes the search field in the toolbar to appear slightly
off-center, this will be fixed in a follow-up patch.

BUG=608968,620030
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2016-06-23 04:03:04]

Patchset #9 (id:220001) has been deleted

[tsergeant, 2016-06-23 04:04:19]

calamity@: Can you please take another look?

As discussed, I've decided to split apart the patch into two. This means that
the search field in this patch is off-center, but fixing that is annoying enough
that it deserves a patch of its own.

I've also updated to include some recent changes to Settings.

[calamity, 2016-06-24 04:11:51]

lgtm

[tsergeant, 2016-06-24 04:41:31]

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

[tsergeant, 2016-06-24 04:41:32]

+dpapad@ for settings and cr_elements

[Dan Beam, 2016-06-25 00:55:20]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-06-25 00:55:21]

are you sure we want to hide the drawer/sidenav by default?

tbuckley@ wasn't so sure the last time I asked

bettes@ is OOO so it's hard to ask him

it'd mean you get a bit less usage of Synced Tab + CLEAR BROWSER DATA

[dpapad, 2016-06-25 01:24:44]

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

https://codereview.chromium.org/2020963002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:83: onMenuTap_: function() {
@private

https://codereview.chromium.org/2020963002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:101: unselectAll: function() {
Could this also be private?

https://codereview.chromium.org/2020963002/diff/240001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right):

https://codereview.chromium.org/2020963002/diff/240001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:16: margin: 6px 6px
6px 18px;
This margin does not seem RTL friendly. Should it be as follows?

-webkit-margin-start: 18px;
-webkit-margin-end: 6px;
margin-top: 6px;
margin-bottom: 6px;

https://codereview.chromium.org/2020963002/diff/240001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:49: <paper-icon-button
id="menuButton" icon="cr:menu" on-tap="onMenuTap_">
Need to add an HTML import for paper-icon-button.

[tsergeant, 2016-06-27 04:57:39]

On 2016/06/25 00:55:21, Dan Beam wrote:
> are you sure we want to hide the drawer/sidenav by default?
> 
> tbuckley@ wasn't so sure the last time I asked
> 
> bettes@ is OOO so it's hard to ask him
> 
> it'd mean you get a bit less usage of Synced Tab + CLEAR BROWSER DATA

Alan had requested this from us in our meetings and in our UX notes document.
This is what Settings is doing now too, right?

I think it's best to land this now and we can revisit it later if need be.

[tsergeant, 2016-06-27 04:57:49]

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

https://codereview.chromium.org/2020963002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:83: onMenuTap_: function() {
On 2016/06/25 01:24:44, dpapad wrote:
> @private

Done.

https://codereview.chromium.org/2020963002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:101: unselectAll: function() {
On 2016/06/25 01:24:44, dpapad wrote:
> Could this also be private?

Done.

https://codereview.chromium.org/2020963002/diff/240001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right):

https://codereview.chromium.org/2020963002/diff/240001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:16: margin: 6px 6px
6px 18px;
On 2016/06/25 01:24:44, dpapad wrote:
> This margin does not seem RTL friendly. Should it be as follows?
> 
> -webkit-margin-start: 18px;
> -webkit-margin-end: 6px;
> margin-top: 6px;
> margin-bottom: 6px;

Good point, done.

https://codereview.chromium.org/2020963002/diff/240001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:49: <paper-icon-button
id="menuButton" icon="cr:menu" on-tap="onMenuTap_">
On 2016/06/25 01:24:44, dpapad wrote:
> Need to add an HTML import for paper-icon-button.

Done.

[Dan Beam, 2016-06-27 19:23:37]

On 2016/06/27 04:57:39, tsergeant wrote:
> On 2016/06/25 00:55:21, Dan Beam wrote:
> > are you sure we want to hide the drawer/sidenav by default?
> > 
> > tbuckley@ wasn't so sure the last time I asked
> > 
> > bettes@ is OOO so it's hard to ask him
> > 
> > it'd mean you get a bit less usage of Synced Tab + CLEAR BROWSER DATA
> 
> Alan had requested this from us in our meetings and in our UX notes document.

he mentioned "no narrow mode by default in history" in a meeting 1-4h before he
went OOO.

> This is what Settings is doing now too, right?

We don't really want to promote "Advanced" in Settings; we do want to promote
"Synced tabs" in History.

> 
> I think it's best to land this now and we can revisit it later if need be.

Feel free to roll the dice, it's your effort/time to remove it later if we don't
want it ;)

[dpapad, 2016-06-27 22:32:19]

LGTM code-wise. Regarding of what Alan actually wants for the toolbar/sidebar
behavior, I don't have first-hand knowledge, so whatever @dbeam said sounds
right.

https://codereview.chromium.org/2020963002/diff/260001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_ui/settings_ui.html (right):

https://codereview.chromium.org/2020963002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.html:8: <link
rel="import"
href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button.html">
No longer needed.

https://codereview.chromium.org/2020963002/diff/260001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right):

https://codereview.chromium.org/2020963002/diff/260001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:53: <paper-icon-button
id="menuButton" icon="cr:menu" on-tap="onMenuTap_">
Nit (optional): You can avoid the menuButton ID since it is only used for the
CSS selector above, as follows

#leftContent paper-icon-button {
 ...
}

[tsergeant, 2016-06-30 04:55:00]

> > 
> > I think it's best to land this now and we can revisit it later if need be.
> 
> Feel free to roll the dice, it's your effort/time to remove it later if we
don't
> want it ;)

Alright, we've decided to land this now. If we decide we want to have the
sidebar open by default it's going to be extra work for me either way, plus
landing it now means we can unblock some other work on history

[tsergeant, 2016-06-30 05:08:00]

Patchset #11 (id:280001) has been deleted

[tsergeant, 2016-06-30 05:08:08]

Patchset #11 (id:300001) has been deleted

[tsergeant, 2016-06-30 05:09:06]

https://codereview.chromium.org/2020963002/diff/260001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_ui/settings_ui.html (right):

https://codereview.chromium.org/2020963002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.html:8: <link
rel="import"
href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button.html">
On 2016/06/27 22:32:19, dpapad wrote:
> No longer needed.

Done.

https://codereview.chromium.org/2020963002/diff/260001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right):

https://codereview.chromium.org/2020963002/diff/260001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:53: <paper-icon-button
id="menuButton" icon="cr:menu" on-tap="onMenuTap_">
On 2016/06/27 22:32:19, dpapad wrote:
> Nit (optional): You can avoid the menuButton ID since it is only used for the
> CSS selector above, as follows
> 
> #leftContent paper-icon-button {
>  ...
> }

Done.

[tsergeant, 2016-06-30 05:09:31]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2016-06-30 05:09:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
calamity@chromium.org, dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2020963002/#ps320001
(title: "Address nits")

[commit-bot: I haz the power, 2016-06-30 05:09:39]

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-06-30 05:51:27]

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

[commit-bot: I haz the power, 2016-06-30 05:51:28]

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

[tsergeant, 2016-06-30 05:55:22]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2016-06-30 05:55:39]

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-06-30 06:15:55]

Committed patchset #11 (id:320001)

[commit-bot: I haz the power, 2016-06-30 06:16:05]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 06:18:41]

Description was changed from

==========
MD History: Hide sidebar behind toolbar menu button

This hides the sidebar and displays a menu button in the toolbar which
opens it in an overlay drawer on the left side of the screen. Also
updates the sidebar menu to have better tab focusing behavior.

This causes the search field in the toolbar to appear slightly
off-center, this will be fixed in a follow-up patch.

BUG=608968,620030
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD History: Hide sidebar behind toolbar menu button

This hides the sidebar and displays a menu button in the toolbar which
opens it in an overlay drawer on the left side of the screen. Also
updates the sidebar menu to have better tab focusing behavior.

This causes the search field in the toolbar to appear slightly
off-center, this will be fixed in a follow-up patch.

BUG=608968,620030
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/105cabbb6d80245ba69e9097013594a03069f8cc
Cr-Commit-Position: refs/heads/master@{#403098}
==========

[commit-bot: I haz the power, 2016-06-30 06:18:43]

Patchset 11 (id:??) landed as
https://crrev.com/105cabbb6d80245ba69e9097013594a03069f8cc
Cr-Commit-Position: refs/heads/master@{#403098}