[MD settings] moving dialog css to shared to use in OnStartup and Reset settings

[MD settings] moving dialog css to shared to use in OnStartup and Reset settings

This CL moves the existing reset dialog CSS styles out to
settings_dialog.css and uses them in both OnStartup and
Reset settings.  Some of the sub-styles were shortened by
removing the 'dialog-' prefix and using a paper-dialog
selector path instead.

BUG=531750
Committed: https://crrev.com/ac5411472bdd953ee14c3865b98aeb51ae15d0bc
Cr-Commit-Position: refs/heads/master@{#364397}

[dschuyler, 2015-12-09 02:27:03]

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

[dschuyler, 2015-12-09 02:27:04]

Hi Demetrios, I like the styles in the reset settings
and I'd like to use them in OnStartup as well.

Along the way, I wondered about shortening the style
names and using a selector path for context.  Such as
naming 'dialog-title' as 'title' and scoping it within
paper-dialog.  Is that good, bad, or just a matter of
style? (sorry for the pun).

[dpapad, 2015-12-09 02:54:43]

On 2015/12/09 at 02:27:04, dschuyler wrote:
> Hi Demetrios, I like the styles in the reset settings
> and I'd like to use them in OnStartup as well.
> 
> Along the way, I wondered about shortening the style
> names and using a selector path for context.  Such as
> naming 'dialog-title' as 'title' and scoping it within
> paper-dialog.  Is that good, bad, or just a matter of
> style? (sorry for the pun).

Hi Dave.
I am positive on sharing CSS as much as possible. But I have a few concerns
which I'll try to summarize here.

Before the existence of Shadow DOM and CSS scoping, a CSS style would apply on
an entire DOM tree by default.
Separating styles to different CSS files was only a matter of improving
readability (or CSS would be shared across
different websites).

With the introduction of CSS scoping and Web Components this is no longer true.
Every shadowRoot node has its own
<style> node and corresponding CSS scope. That means that generating an shared
broad_css_file.css and injecting it on
multiple web components those styles are replicated multiple times (see the
<style> element, of each component that
includes settings_shared.css), which is my concern.

Specifically for this CL,
1) <reset-profile-dialog>,<powerwash-dialog> now inherit several CSS rules from
settings_shared.css that are never used
   (.settings-box, h2, .text-elide, list-item).
2) In the same way, whoever includes settings_shared.css inherits CSS rules
related to dialogs, that will never be used.

Both 1 and 2 make me uncomfortable. Private CSS scopes are polluted for
everyone, in a way to simulate the pre-shadowDom era. That
is why I am in favor of narrow scope shared CSS files, such that the "pollution"
of a component's CSS scope is minimized. On that
note the StartUp section in the mocks does not seem to include a dialog. How is
moving the paper-dialog related CSS rules helping?

[dschuyler, 2015-12-09 03:26:18]

On 2015/12/09 02:54:43, dpapad wrote:
> On 2015/12/09 at 02:27:04, dschuyler wrote:
> > Hi Demetrios, I like the styles in the reset settings
> > and I'd like to use them in OnStartup as well.
> > 
> > Along the way, I wondered about shortening the style
> > names and using a selector path for context.  Such as
> > naming 'dialog-title' as 'title' and scoping it within
> > paper-dialog.  Is that good, bad, or just a matter of
> > style? (sorry for the pun).
> 
> Hi Dave.
> I am positive on sharing CSS as much as possible. But I have a few concerns
> which I'll try to summarize here.
> 
> Before the existence of Shadow DOM and CSS scoping, a CSS style would apply on
> an entire DOM tree by default.
> Separating styles to different CSS files was only a matter of improving
> readability (or CSS would be shared across
> different websites).
> 
> With the introduction of CSS scoping and Web Components this is no longer
true.
> Every shadowRoot node has its own
> <style> node and corresponding CSS scope. That means that generating an shared
> broad_css_file.css and injecting it on
> multiple web components those styles are replicated multiple times (see the
> <style> element, of each component that
> includes settings_shared.css), which is my concern.
> 
> Specifically for this CL,
> 1) <reset-profile-dialog>,<powerwash-dialog> now inherit several CSS rules
from
> settings_shared.css that are never used
>    (.settings-box, h2, .text-elide, list-item).
> 2) In the same way, whoever includes settings_shared.css inherits CSS rules
> related to dialogs, that will never be used.
> 
> Both 1 and 2 make me uncomfortable. Private CSS scopes are polluted for
> everyone, in a way to simulate the pre-shadowDom era. That
> is why I am in favor of narrow scope shared CSS files, such that the
"pollution"
> of a component's CSS scope is minimized. On that
> note the StartUp section in the mocks does not seem to include a dialog. How
is
> moving the paper-dialog related CSS rules helping?

Ah, thanks for describing that.  Okay, how about having the dialog styles in a 
separate file that is still in a shared area?  So those pieces of code that want
dialog styling can use the css without pulling in unneeded things.

I need to double check this new patch that has a separate settings_dialog.css
file, but I wanted to upload it to get your thoughts on something like this.

[dpapad, 2015-12-09 04:10:38]

On 2015/12/09 at 03:26:18, dschuyler wrote:
> On 2015/12/09 02:54:43, dpapad wrote:
> > On 2015/12/09 at 02:27:04, dschuyler wrote:
> > > Hi Demetrios, I like the styles in the reset settings
> > > and I'd like to use them in OnStartup as well.
> > > 
> > > Along the way, I wondered about shortening the style
> > > names and using a selector path for context.  Such as
> > > naming 'dialog-title' as 'title' and scoping it within
> > > paper-dialog.  Is that good, bad, or just a matter of
> > > style? (sorry for the pun).
> > 
> > Hi Dave.
> > I am positive on sharing CSS as much as possible. But I have a few concerns
> > which I'll try to summarize here.
> > 
> > Before the existence of Shadow DOM and CSS scoping, a CSS style would apply
on
> > an entire DOM tree by default.
> > Separating styles to different CSS files was only a matter of improving
> > readability (or CSS would be shared across
> > different websites).
> > 
> > With the introduction of CSS scoping and Web Components this is no longer
true.
> > Every shadowRoot node has its own
> > <style> node and corresponding CSS scope. That means that generating an
shared
> > broad_css_file.css and injecting it on
> > multiple web components those styles are replicated multiple times (see the
> > <style> element, of each component that
> > includes settings_shared.css), which is my concern.
> > 
> > Specifically for this CL,
> > 1) <reset-profile-dialog>,<powerwash-dialog> now inherit several CSS rules
from
> > settings_shared.css that are never used
> >    (.settings-box, h2, .text-elide, list-item).
> > 2) In the same way, whoever includes settings_shared.css inherits CSS rules
> > related to dialogs, that will never be used.
> > 
> > Both 1 and 2 make me uncomfortable. Private CSS scopes are polluted for
> > everyone, in a way to simulate the pre-shadowDom era. That
> > is why I am in favor of narrow scope shared CSS files, such that the
"pollution"
> > of a component's CSS scope is minimized. On that
> > note the StartUp section in the mocks does not seem to include a dialog. How
is
> > moving the paper-dialog related CSS rules helping?
> 
> Ah, thanks for describing that.  Okay, how about having the dialog styles in a

> separate file that is still in a shared area?  So those pieces of code that
want
> dialog styling can use the css without pulling in unneeded things.
> 
> I need to double check this new patch that has a separate settings_dialog.css
> file, but I wanted to upload it to get your thoughts on something like this.

A share-able settings_dialog.css sounds much better, since it has a narrow
scope.

On a case-by-case basis we might want to implement re-usable pre-styled
components (<settings-dialog>,
<settings-action-button> etc), such that sharing styles would be done by reusing
the component as a whole
and not by sharing the CSS separately. Anyway, the shared settings_dialog.css
sounds like a good start.

[dschuyler, 2015-12-09 22:30:59]

On 2015/12/09 04:10:38, dpapad wrote:
> On 2015/12/09 at 03:26:18, dschuyler wrote:
> > On 2015/12/09 02:54:43, dpapad wrote:
> > > On 2015/12/09 at 02:27:04, dschuyler wrote:
> > > > Hi Demetrios, I like the styles in the reset settings
> > > > and I'd like to use them in OnStartup as well.
> > > > 
> > > > Along the way, I wondered about shortening the style
> > > > names and using a selector path for context.  Such as
> > > > naming 'dialog-title' as 'title' and scoping it within
> > > > paper-dialog.  Is that good, bad, or just a matter of
> > > > style? (sorry for the pun).
> > > 
> > > Hi Dave.
> > > I am positive on sharing CSS as much as possible. But I have a few
concerns
> > > which I'll try to summarize here.
> > > 
> > > Before the existence of Shadow DOM and CSS scoping, a CSS style would
apply
> on
> > > an entire DOM tree by default.
> > > Separating styles to different CSS files was only a matter of improving
> > > readability (or CSS would be shared across
> > > different websites).
> > > 
> > > With the introduction of CSS scoping and Web Components this is no longer
> true.
> > > Every shadowRoot node has its own
> > > <style> node and corresponding CSS scope. That means that generating an
> shared
> > > broad_css_file.css and injecting it on
> > > multiple web components those styles are replicated multiple times (see
the
> > > <style> element, of each component that
> > > includes settings_shared.css), which is my concern.
> > > 
> > > Specifically for this CL,
> > > 1) <reset-profile-dialog>,<powerwash-dialog> now inherit several CSS rules
> from
> > > settings_shared.css that are never used
> > >    (.settings-box, h2, .text-elide, list-item).
> > > 2) In the same way, whoever includes settings_shared.css inherits CSS
rules
> > > related to dialogs, that will never be used.
> > > 
> > > Both 1 and 2 make me uncomfortable. Private CSS scopes are polluted for
> > > everyone, in a way to simulate the pre-shadowDom era. That
> > > is why I am in favor of narrow scope shared CSS files, such that the
> "pollution"
> > > of a component's CSS scope is minimized. On that
> > > note the StartUp section in the mocks does not seem to include a dialog.
How
> is
> > > moving the paper-dialog related CSS rules helping?
> > 
> > Ah, thanks for describing that.  Okay, how about having the dialog styles in
a
> 
> > separate file that is still in a shared area?  So those pieces of code that
> want
> > dialog styling can use the css without pulling in unneeded things.
> > 
> > I need to double check this new patch that has a separate
settings_dialog.css
> > file, but I wanted to upload it to get your thoughts on something like this.
> 
> A share-able settings_dialog.css sounds much better, since it has a narrow
> scope.
> 
> On a case-by-case basis we might want to implement re-usable pre-styled
> components (<settings-dialog>,
> <settings-action-button> etc), such that sharing styles would be done by
reusing
> the component as a whole
> and not by sharing the CSS separately. Anyway, the shared settings_dialog.css
> sounds like a good start.

Ok, I've had a chance to check this change and reset dialogs still look good
after this change.  PTAL.

Oh, and the question about the OnStartup dialog, there is a dialog being added.
Alan may not have it up on the folio yet.  It's a dialog that comes up to add
a new startup page.

[dpapad, 2015-12-09 23:12:06]

Could you update the CL description, since the dialog styles are no longer moved
to settings_shared.css? Also could you provide some context (screenshot maybe?)
for the changes
in settings_shared.css, or could the changes to that file be moved to a separate
CL? They seem
unrelated to the move of the dialog styles.

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_dialog.css (right):

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_dialog.css:9: 
Nit: Unnecessary blank line.

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared.css (right):

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared.css:77: .link-button {
Are we still using buttons as links? They were removed from the reset page
dialogs, and regular links are used now.

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared.css:82: padding-top: 6px;
Lines 79-82 can be compressed as follows:
padding: 6px 0;

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared.css:92: .secondary-color {
This one is a bit odd. 
1) Secondary to what? Is there a primary-color class.
2) Most importantly this class resembles a CSS variable, instead of having some
semantic meaning. It is similar to doing something like this.

.display-block {
  display: block;
}

[dschuyler, 2015-12-09 23:23:14]

Description was changed from

==========
[MD settings] moving dialog css to shared to use in OnStartup and Reset settings

This CL moves the existing reset dialog CSS styles out to settings_shared.css
and uses them in both OnStartup and Reset settings.  Some of the sub-styles
were shortened by removing the 'dialog-' prefix and using a paper-dialog
selector path instead.

BUG=531750
==========

to

==========
[MD settings] moving dialog css to shared to use in OnStartup and Reset settings

This CL moves the existing reset dialog CSS styles out to
settings_dialog.css and uses them in both OnStartup and 
Reset settings.  Some of the sub-styles were shortened by 
removing the 'dialog-' prefix and using a paper-dialog
selector path instead.

BUG=531750
==========

[dschuyler, 2015-12-09 23:30:00]

You're right about the settings_shared.css
that's not part of this CL at this point.

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_dialog.css (right):

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_dialog.css:9: 
On 2015/12/09 23:12:06, dpapad wrote:
> Nit: Unnecessary blank line.

Without this line it looks like the comment is
for the next style description, but it's a 
file level comment.  I've removed the blank 
line, but I think it looks weird.

Done.

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared.css (right):

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared.css:77: .link-button {
On 2015/12/09 23:12:06, dpapad wrote:
> Are we still using buttons as links? They were removed from the reset page
> dialogs, and regular links are used now.

Acknowledged.

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared.css:82: padding-top: 6px;
On 2015/12/09 23:12:06, dpapad wrote:
> Lines 79-82 can be compressed as follows:
> padding: 6px 0;

Acknowledged.

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared.css:92: .secondary-color {
On 2015/12/09 23:12:06, dpapad wrote:
> This one is a bit odd. 
> 1) Secondary to what? Is there a primary-color class.
> 2) Most importantly this class resembles a CSS variable, instead of having
some
> semantic meaning. It is similar to doing something like this.
> 
> .display-block {
>   display: block;
> }

Acknowledged.

[dpapad, 2015-12-09 23:39:50]

LGTM

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_dialog.css (right):

https://codereview.chromium.org/1509913004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_dialog.css:9: 
On 2015/12/09 at 23:29:59, dschuyler wrote:
> On 2015/12/09 23:12:06, dpapad wrote:
> > Nit: Unnecessary blank line.
> 
> Without this line it looks like the comment is
> for the next style description, but it's a 
> file level comment.  I've removed the blank 
> line, but I think it looks weird.
> 
> Done.

I see, did not think about it that way. I am fine if you want to re-add the
blank line here.

[dschuyler, 2015-12-10 00:03:09]

Thanks

[dschuyler, 2015-12-10 17:57:43]

fyi the fix for the browser tests related to my using some i18n strings in
OnStartup that are not in yet.  No change to the reset or css stuff.

[dschuyler, 2015-12-10 17:57:50]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2015-12-10 17:57:50]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/1509913004/#ps120001
(title: "fix for browsertest
")

[commit-bot: I haz the power, 2015-12-10 17:58:45]

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

[commit-bot: I haz the power, 2015-12-10 18:08:06]

Description was changed from

==========
[MD settings] moving dialog css to shared to use in OnStartup and Reset settings

This CL moves the existing reset dialog CSS styles out to
settings_dialog.css and uses them in both OnStartup and 
Reset settings.  Some of the sub-styles were shortened by 
removing the 'dialog-' prefix and using a paper-dialog
selector path instead.

BUG=531750
==========

to

==========
[MD settings] moving dialog css to shared to use in OnStartup and Reset settings

This CL moves the existing reset dialog CSS styles out to
settings_dialog.css and uses them in both OnStartup and 
Reset settings.  Some of the sub-styles were shortened by 
removing the 'dialog-' prefix and using a paper-dialog
selector path instead.

BUG=531750
==========

[commit-bot: I haz the power, 2015-12-10 18:08:07]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-12-10 18:09:54]

Description was changed from

==========
[MD settings] moving dialog css to shared to use in OnStartup and Reset settings

This CL moves the existing reset dialog CSS styles out to
settings_dialog.css and uses them in both OnStartup and 
Reset settings.  Some of the sub-styles were shortened by 
removing the 'dialog-' prefix and using a paper-dialog
selector path instead.

BUG=531750
==========

to

==========
[MD settings] moving dialog css to shared to use in OnStartup and Reset settings

This CL moves the existing reset dialog CSS styles out to
settings_dialog.css and uses them in both OnStartup and
Reset settings.  Some of the sub-styles were shortened by
removing the 'dialog-' prefix and using a paper-dialog
selector path instead.

BUG=531750

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

[commit-bot: I haz the power, 2015-12-10 18:09:55]

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