Show Pages in chrome://md-settings

Show Pages in chrome://md-settings

Add cr-settings-a11y-page and cr-settings-downloads-page to
md-settings.

Follow-up to http://crrev.com/1007803003.

Screenshot: http://i.imgur.com/qT28Xqo.png (colors are a bit off due to chromoting, will upload another tomorrow)

BUG=464979
R=stevenjb@chromium.org,jhawkins@chromium.org

Committed: https://crrev.com/8749e547dcb00f0000a303abd010a7de306efa7f
Cr-Commit-Position: refs/heads/master@{#321194}

[michaelpg, 2015-03-17 23:42:14]

PTAL!

[Kyle Horimoto, 2015-03-17 23:53:47]

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

[Kyle Horimoto, 2015-03-17 23:53:48]

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/downloads_page/downloads_page.html
(right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.html:15:
<cr-settings-page-header page="{{}}"></cr-settings-page-header>
page="{{}}"?

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_main/settings_main.css (right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.css:32: /* This
prevents the page from taking up the entire container, which
Why is the page taking up the entire container to begin with? I thought the
other file said that it was NOT taking up the whole container?

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:94: if (page)
When would page be falsey?

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:95:
this.$.pageContainer.style.height = page.scrollHeight + 'px';
Can you add a comment explaining this? It's not clear to me what's going on.

[Jeremy Klein, 2015-03-18 00:12:04]

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

[Jeremy Klein, 2015-03-18 00:12:05]

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/downloads_page/downloads_page.css
(right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.css:6:
-webkit-margin-end: 10px;
Are these core-label styles just going to be shared for all pages? Maybe they
should be in settings_page.css?

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_main/settings_main.css (right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.css:28:
transition: height 300ms ease-out;
When does this transition happen? Is it due to different pages sliding in and
changing the inherited height of the container?

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:69:
}.bind(this));
this.async already binds to this:

https://www.polymer-project.org/0.5/docs/polymer/polymer.html#asyncmethod

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:87: * Updates
the height style for core-animated-pages using the page's
Is this really necessary? Does the core-animated-pages require an explicit
height or something?

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:110: * pages
are position: absolute, so the container won't size to the page.
It seems like the intended design here is to have the core-animated-pages choose
its own fixed height and then the pages size to it instead of the other way
around. It there any way we could keep that behavior without going against the
mocks? Maybe some of the demos at:

https://www.polymer-project.org/0.5/components/core-animated-pages/demo.html

have hints?

[michaelpg, 2015-03-18 00:18:00]

Closing for now. Will put together short demo.

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/downloads_page/downloads_page.html
(right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.html:15:
<cr-settings-page-header page="{{}}"></cr-settings-page-header>
On 2015/03/17 23:53:48, Kyle Horimoto wrote:
> page="{{}}"?

Like "this". We're giving the cr-settings-page-header element our page as a
parameter, from which it gets the title and icon info.

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_main/settings_main.css (right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.css:32: /* This
prevents the page from taking up the entire container, which
On 2015/03/17 23:53:48, Kyle Horimoto wrote:
> Why is the page taking up the entire container to begin with? I thought the
> other file said that it was NOT taking up the whole container?

I'll clarify in the comments. Explanation follows but it's a lot. Maybe I'll
write up a doc and show it to the Polymer folks.

Initially the container height is 0 since its children (the pages) are position:
absolute, meaning the children don't resize the parent. We show a page and set
the container's height to the page's height.

Because all the pages are bottom: 0, they resize to fit their parent (the
core-animated-pages). So core-animated-pages doesn't resize automatically, but
the pages will fill whatever size we set the container to.

When we hide a page and make another page visible, the newly visible page fills
the size of the container so it's the same size as the previous page. Setting
bottom: initial prevents the page from filling the container when the container
is larger than the page, so we don't have extra space at the bottom.

We could clear the parent container's height before asking the page for its
height, but then the smooth transition doesn't play.

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:110: * pages
are position: absolute, so the container won't size to the page.
On 2015/03/18 00:12:05, Jeremy Klein wrote:
> It seems like the intended design here is to have the core-animated-pages
choose
> its own fixed height and then the pages size to it instead of the other way
> around. It there any way we could keep that behavior without going against the
> mocks? Maybe some of the demos at:
> 
> https://www.polymer-project.org/0.5/components/core-animated-pages/demo.html
> 
> have hints?

I can't find a demo where the actual height of the pages changes. I spoke with
ffu@ and he said this was tricky because of the way core-animated-pages is
built. I'll put together a simple demo of the problem and send that to the
Polymer guys who worked on this. Since this whole CL is basically that, I'll
close this for now. Thanks for the feedback!

[michaelpg, 2015-03-18 01:34:26]

Patchset #3 (id:40001) has been deleted

[michaelpg, 2015-03-18 01:34:30]

Patchset #3 (id:60001) has been deleted

[michaelpg, 2015-03-18 01:36:12]

Patchset #2 (id:20001) has been deleted

[michaelpg, 2015-03-18 01:36:19]

Patchset #2 (id:80001) has been deleted

[michaelpg, 2015-03-18 01:40:09]

OK, let's try this. Moving paper-shadow inside the individual pages means we
don't have to worry about the height of the core-animated-pages itself.

The transition is a little weird (the white background fades to grey and then
back to white) but meh, we can deal with that after the animations are actually
mocked up.

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/downloads_page/downloads_page.css
(right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.css:6:
-webkit-margin-end: 10px;
On 2015/03/18 00:12:04, Jeremy Klein wrote:
> Are these core-label styles just going to be shared for all pages? Maybe they
> should be in settings_page.css?

Done.

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_main/settings_main.css (right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.css:28:
transition: height 300ms ease-out;
On 2015/03/18 00:12:04, Jeremy Klein wrote:
> When does this transition happen? Is it due to different pages sliding in and
> changing the inherited height of the container?

Removed. (yes)

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.css:32: /* This
prevents the page from taking up the entire container, which
On 2015/03/17 23:53:48, Kyle Horimoto wrote:
> Why is the page taking up the entire container to begin with? I thought the
> other file said that it was NOT taking up the whole container?

Removed.

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:69:
}.bind(this));
On 2015/03/18 00:12:05, Jeremy Klein wrote:
> this.async already binds to this:
> 
> https://www.polymer-project.org/0.5/docs/polymer/polymer.html#asyncmethod

Acknowledged.

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:87: * Updates
the height style for core-animated-pages using the page's
On 2015/03/18 00:12:05, Jeremy Klein wrote:
> Is this really necessary? Does the core-animated-pages require an explicit
> height or something?

This was my attempt to keep <paper-shadow> outside of <core-animated-pages>.
Wrapping the contents of each page in a <paper-shadow> (we were already using a
<section>) makes the height stuff not necessary.

[Jeremy Klein, 2015-03-18 01:58:30]

lgtm

Much cleaner :-)

https://codereview.chromium.org/1013833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/a11y_page/a11y_page.html (right):

https://codereview.chromium.org/1013833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/a11y_page/a11y_page.html:13: <paper-shadow
layout vertical cross-fade>
Here and in other pages: import paper-shadow.

https://codereview.chromium.org/1013833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.html (right):

https://codereview.chromium.org/1013833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.html:16:
on-core-animated-pages-transition-prepare="{{pageTransitionPrepared_}}">
We don't need the event listener anymore, right?

[michaelpg, 2015-03-18 02:07:22]

https://codereview.chromium.org/1013833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/a11y_page/a11y_page.html (right):

https://codereview.chromium.org/1013833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/a11y_page/a11y_page.html:13: <paper-shadow
layout vertical cross-fade>
On 2015/03/18 01:58:30, Jeremy Klein wrote:
> Here and in other pages: import paper-shadow.

Done.

https://codereview.chromium.org/1013833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.html (right):

https://codereview.chromium.org/1013833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.html:16:
on-core-animated-pages-transition-prepare="{{pageTransitionPrepared_}}">
On 2015/03/18 01:58:30, Jeremy Klein wrote:
> We don't need the event listener anymore, right?

Done.

[Kyle Horimoto, 2015-03-18 18:29:01]

lgtm

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/downloads_page/downloads_page.html
(right):

https://codereview.chromium.org/1013833003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.html:15:
<cr-settings-page-header page="{{}}"></cr-settings-page-header>
On 2015/03/18 00:18:00, michaelpg wrote:
> On 2015/03/17 23:53:48, Kyle Horimoto wrote:
> > page="{{}}"?
> 
> Like "this". We're giving the cr-settings-page-header element our page as a
> parameter, from which it gets the title and icon info.

Interesting, I had no idea that you could do this!

[michaelpg, 2015-03-18 18:48:32]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2015-03-18 18:48:33]

The patchset sent to the CQ was uploaded after l-g-t-m from jlklein@chromium.org
Link to the patchset: https://codereview.chromium.org/1013833003/#ps120001
(title: "jlklein's feedback")

[commit-bot: I haz the power, 2015-03-18 18:49:14]

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

[commit-bot: I haz the power, 2015-03-18 19:39:37]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2015-03-18 19:40:20]

Patchset 3 (id:??) landed as
https://crrev.com/8749e547dcb00f0000a303abd010a7de306efa7f
Cr-Commit-Position: refs/heads/master@{#321194}