Settings Rewrite: Make subpage scrolling work.

Settings Rewrite: Make subpage scrolling work.

This also fixes the issue where the page reflows during animation. This is because it adds a fake 'placeholder' div which prevents the reflow.

BUG=535013
Committed: https://crrev.com/f99bfb03c2868720d9e869c609018613e24d531f
Cr-Commit-Position: refs/heads/master@{#350967}

[tommycli, 2015-09-25 00:00:58]

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

[tommycli, 2015-09-25 00:01:06]

dbeam: ptal, thanks

[Dan Beam, 2015-09-25 00:49:42]

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.css
(right):

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:17:
:host(.hidden) {
why do we need .hidden when we can just use... hidden?

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:49:
margin-bottom: 16px;
add a comment that this matches paper-material or move to a new rule:

paper-material,
#placeholder {
  margin-bottom: 16px;
  width: 605px;
}

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:52: width:
100px;
why wouldn't we just match the paper-material width?

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:55:
:host(.neon-animating) #placeholder {
:host(:not(.neon-animating)) #placeholder {
  position: absolute;
}

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:99:
this.currentRoute.section != this.section);
this.classList.toggle('hidden', ...); -> this.hidden = ...; ?

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:111: var
node = config.node.$.section;
update var name

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:135: if
(config.node.parentNode.children[i].fire)
when is this false?

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:151: var
node = config.node.$.section;
update var name

[tommycli, 2015-09-25 17:23:22]

dbeam: thanks!

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.css
(right):

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:17:
:host(.hidden) {
On 2015/09/25 00:49:41, Dan Beam wrote:
> why do we need .hidden when we can just use... hidden?

Done.

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:49:
margin-bottom: 16px;
On 2015/09/25 00:49:41, Dan Beam wrote:
> add a comment that this matches paper-material or move to a new rule:
> 
> paper-material,
> #placeholder {
>   margin-bottom: 16px;
>   width: 605px;
> }

Done.

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:52: width:
100px;
On 2015/09/25 00:49:41, Dan Beam wrote:
> why wouldn't we just match the paper-material width?

Done.

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:55:
:host(.neon-animating) #placeholder {
On 2015/09/25 00:49:41, Dan Beam wrote:
> :host(:not(.neon-animating)) #placeholder {
>   position: absolute;
> }

Done.

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:99:
this.currentRoute.section != this.section);
On 2015/09/25 00:49:42, Dan Beam wrote:
> this.classList.toggle('hidden', ...); -> this.hidden = ...; ?

Done.

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:111: var
node = config.node.$.section;
On 2015/09/25 00:49:42, Dan Beam wrote:
> update var name

Done.

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:135: if
(config.node.parentNode.children[i].fire)
On 2015/09/25 00:49:42, Dan Beam wrote:
> when is this false?

Done. First sibling is a STYLE tag. I updated the condition to make it more
clear.

https://codereview.chromium.org/1358313004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:151: var
node = config.node.$.section;
On 2015/09/25 00:49:42, Dan Beam wrote:
> update var name

Done.

[tommycli, 2015-09-25 18:45:10]

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

[commit-bot: I haz the power, 2015-09-25 18:45:56]

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

[commit-bot: I haz the power, 2015-09-25 19:42:14]

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

[commit-bot: I haz the power, 2015-09-25 19:42:15]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2015-09-25 23:18:02]

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:131:
config.node.classList.remove('neon-animating');
what is config.node?  can you make a short, descriptive variable alias to it and
re-use it?

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:136: if
(config.node.parentNode.children[i].nodeName == config.node.nodeName)
i'm confused.

can this be .querySelectorAll('tag-name')?  I think that'd be far less
confusing.  or is this supposed to be siblings with the same tagName?

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:152: var
section = config.node.$.section;
move lower, closer to where it's actually used

[Dan Beam, 2015-09-25 23:18:57]

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js (left):

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:14: *   
</cr-settings-section>
you should probably do this separately

[tommycli, 2015-09-25 23:36:40]

dbeam: thx

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js (left):

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:14: *   
</cr-settings-section>
On 2015/09/25 23:18:57, Dan Beam wrote:
> you should probably do this separately

Done.

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:131:
config.node.classList.remove('neon-animating');
On 2015/09/25 23:18:02, Dan Beam wrote:
> what is config.node?  can you make a short, descriptive variable alias to it
and
> re-use it?

Done. Sure. Not sure what to call it other than 'element'. It's the polymer
element as described in the file.

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:136: if
(config.node.parentNode.children[i].nodeName == config.node.nodeName)
On 2015/09/25 23:18:02, Dan Beam wrote:
> i'm confused.
> 
> can this be .querySelectorAll('tag-name')?  I think that'd be far less
> confusing.  or is this supposed to be siblings with the same tagName?

Done.

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:152: var
section = config.node.$.section;
On 2015/09/25 23:18:02, Dan Beam wrote:
> move lower, closer to where it's actually used

Done.

[Dan Beam, 2015-09-25 23:38:46]

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:131:
config.node.classList.remove('neon-animating');
On 2015/09/25 23:36:40, tommycli wrote:
> On 2015/09/25 23:18:02, Dan Beam wrote:
> > what is config.node?  can you make a short, descriptive variable alias to it
> and
> > re-use it?
> 
> Done. Sure. Not sure what to call it other than 'element'. It's the polymer
> element as described in the file.

it's at least a parent of a section, right?  sectionWrapper?  sectionParent?

[Dan Beam, 2015-09-25 23:49:03]

https://codereview.chromium.org/1358313004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:137: var
siblings = element.parentNode.querySelectorAll('cr-settings-section');
let's make sure this is limited to 1 level of depth if that's what we want

https://codereview.chromium.org/1358313004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:137: var
siblings = element.parentNode.querySelectorAll('cr-settings-section');
this includes |element|.  make sure to capture this somehow

[tommycli, 2015-09-25 23:53:30]

dbeam: thx

https://codereview.chromium.org/1358313004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:137: var
siblings = element.parentNode.querySelectorAll('cr-settings-section');
On 2015/09/25 23:49:03, Dan Beam wrote:
> this includes |element|.  make sure to capture this somehow

Done.

https://codereview.chromium.org/1358313004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:137: var
siblings = element.parentNode.querySelectorAll('cr-settings-section');
On 2015/09/25 23:49:03, Dan Beam wrote:
> let's make sure this is limited to 1 level of depth if that's what we want

Done.

[Dan Beam, 2015-09-26 00:09:11]

https://codereview.chromium.org/1358313004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:111: var
sectionWrapper = config.node;
like in packaging, a wrapper is around something.  config.node is a
cr-settings-section, not the parent of a cr-setting-section like sectionWrapper
implies, correct?  so, wouldn't this just be section?

https://codereview.chromium.org/1358313004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:139: var
siblings = sectionWrapper.parentNode.children;
i think just naming this var `sections` or something would probably be better
than the comment you have added

[tommycli, 2015-09-26 00:17:33]

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:131:
config.node.classList.remove('neon-animating');
On 2015/09/25 23:38:45, Dan Beam wrote:
> On 2015/09/25 23:36:40, tommycli wrote:
> > On 2015/09/25 23:18:02, Dan Beam wrote:
> > > what is config.node?  can you make a short, descriptive variable alias to
it
> > and
> > > re-use it?
> > 
> > Done. Sure. Not sure what to call it other than 'element'. It's the polymer
> > element as described in the file.
> 
> it's at least a parent of a section, right?  sectionWrapper?  sectionParent?

Done.

https://codereview.chromium.org/1358313004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:111: var
sectionWrapper = config.node;
On 2015/09/26 00:09:11, Dan Beam wrote:
> like in packaging, a wrapper is around something.  config.node is a
> cr-settings-section, not the parent of a cr-setting-section like
sectionWrapper
> implies, correct?  so, wouldn't this just be section?

Done.

https://codereview.chromium.org/1358313004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:139: var
siblings = sectionWrapper.parentNode.children;
On 2015/09/26 00:09:10, Dan Beam wrote:
> i think just naming this var `sections` or something would probably be better
> than the comment you have added

Done.

[Dan Beam, 2015-09-26 00:22:07]

lgtm

https://codereview.chromium.org/1358313004/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:139: for
(var i = 0; i < sections.length; ++i) {
assert(sections[i].parent == section.parent);

[Dan Beam, 2015-09-26 00:22:29]

https://codereview.chromium.org/1358313004/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:139: for
(var i = 0; i < sections.length; ++i) {
On 2015/09/26 00:22:07, Dan Beam wrote:
> assert(sections[i].parent == section.parent);

parentNode* (or parentElement?)

[tommycli, 2015-09-26 00:32:16]

https://codereview.chromium.org/1358313004/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1358313004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:139: for
(var i = 0; i < sections.length; ++i) {
On 2015/09/26 00:22:07, Dan Beam wrote:
> assert(sections[i].parent == section.parent);

It would be benign to fire these on deeper descendants also. Those don't exist
yet anyways.

[tommycli, 2015-09-26 00:32:30]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-09-26 00:33:04]

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

[commit-bot: I haz the power, 2015-09-26 01:21:18]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-09-26 01:22:15]

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

[tommycli, 2015-09-28 18:21:00]

Patchset #10 (id:180001) has been deleted