Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2231593003:
Add [hidden] rule to cr-elements :root stylesheet (Closed)
|
Created:
4 years, 4 months ago by michaelpg Modified:
4 years, 3 months ago Reviewers:
dschuyler CC:
chromium-reviews, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-elements_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd [hidden] rule to cr-elements :root stylesheet
Chrome has a User-Agent style rule that sets [hidden]
to { display: none }, but this is overridden by almost
any other rule that modifies display (e.g. a class which
sets { display: flex }).
For [hidden] to work as expected, we need to add
!important to the rule. For shadow DOM, this needs to use
the (deprecated) /deep/ combinator to apply everywhere.
This CL is necessary to stop using iron-flex-layout/classes
which sets a ton of styles globally, most importantly this
one.
BUG=498405
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Patch Set 1 #
Total comments: 6
Patch Set 2 : comment spacing #
Dependent Patchsets: Messages
Total messages: 18 (6 generated)
Description was changed from
==========
Add [hidden] rule to cr-elements :root stylesheet
Necessary to stop using iron-flex-layout/classes which sets a ton of styles
globally, including [hidden] { display: none }.
BUG=498405
R=dschuyler@chromium.org
==========
to
==========
Add [hidden] rule to cr-elements :root stylesheet
Necessary to stop using iron-flex-layout/classes which sets a ton of styles
globally, including [hidden] { display: none }.
BUG=498405
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
Description was changed from
==========
Add [hidden] rule to cr-elements :root stylesheet
Necessary to stop using iron-flex-layout/classes which sets a ton of styles
globally, including [hidden] { display: none }.
BUG=498405
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
Add [hidden] rule to cr-elements :root stylesheet
Chrome has a User-Agent style rule that sets [hidden]
to { display: none }, but this is overridden by almost
any other rule that modifies display (e.g. a class which
sets { display: flex }).
For [hidden] to work as expected, we need to add
!important to the rule. For shadow DOM, this needs to use
the (deprecated) /deep/ combinator to apply everywhere.
This CL is necessary to stop using iron-flex-layout/classes
which sets a ton of styles globally, most importantly this
one.
BUG=498405
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2231593003/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2231593003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_vars_css.html:18: */ nit: /* Everybody seems to love useless lines. * They make comments so spacious. */ ^ ^^ https://codereview.chromium.org/2231593003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_vars_css.html:19: :root /deep/ [hidden] { why do we need the /deep/ though? also, why is this in a _vars_ file?
https://codereview.chromium.org/2231593003/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2231593003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_vars_css.html:18: */ On 2016/08/10 16:16:36, Dan Beam wrote: > nit: > > /* Everybody seems to love useless lines. > * They make comments so spacious. */ > ^ ^^ okay, that's our CSS style, but i have no love for useless lines... it's just how we do multiline comments everywhere else. https://codereview.chromium.org/2231593003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_vars_css.html:19: :root /deep/ [hidden] { On 2016/08/10 16:16:36, Dan Beam wrote: > why do we need the /deep/ though? see description > > also, why is this in a _vars_ file? see TODO (rationale: this file sets :root styles, not solely variables, it should be renamed as such)
https://codereview.chromium.org/2231593003/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2231593003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_vars_css.html:18: */ On 2016/08/10 16:27:56, michaelpg wrote: > On 2016/08/10 16:16:36, Dan Beam wrote: > > nit: > > > > /* Everybody seems to love useless lines. > > * They make comments so spacious. */ > > ^ ^^ > > okay, that's our CSS style, but i have no love for useless lines... it's just > how we do multiline comments everywhere else. no, it's not. if you show me all the other places we do this, I'll gladly clean them up for you to fix your (and probably others') incorrect interpretation.
On 2016/08/10 16:29:24, Dan Beam wrote: > https://codereview.chromium.org/2231593003/diff/1/ui/webui/resources/cr_eleme... > File ui/webui/resources/cr_elements/shared_vars_css.html (right): > > https://codereview.chromium.org/2231593003/diff/1/ui/webui/resources/cr_eleme... > ui/webui/resources/cr_elements/shared_vars_css.html:18: */ > On 2016/08/10 16:27:56, michaelpg wrote: > > On 2016/08/10 16:16:36, Dan Beam wrote: > > > nit: > > > > > > /* Everybody seems to love useless lines. > > > * They make comments so spacious. */ > > > ^ ^^ > > > > okay, that's our CSS style, but i have no love for useless lines... it's just > > how we do multiline comments everywhere else. > > no, it's not. if you show me all the other places we do this, I'll gladly clean > them up for you to fix your (and probably others') incorrect interpretation. er, "everywhere else" => "in most other languages, IIRC, but I don't care enough to dig through code and style guides"
https://codereview.chromium.org/2231593003/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2231593003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_vars_css.html:19: :root /deep/ [hidden] { On 2016/08/10 16:27:56, michaelpg wrote: > On 2016/08/10 16:16:36, Dan Beam wrote: > > why do we need the /deep/ though? > > see description > > > > > also, why is this in a _vars_ file? > > see TODO (rationale: this file sets :root styles, not solely variables, it > should be renamed as such) It was named root and got renamed to vars. Please see Stevenjb@ before changing it. The name was changed because the intent of the file was to contain only vars (so he/we felt that root was no longer the best name for it). How about moving this to the settings.html file?
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
+noms cuz dragons
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
On 2016/08/11 06:09:33, Dan Beam wrote: > +noms cuz dragons (audible, but understandable, sigh) side note: if you include `iron-flex-layout/iron-flex-layout-classes.html` everywhere, that should define this rule for you, sans dragons.
On 2016/08/11 07:15:23, noms (inactive) wrote: > On 2016/08/11 06:09:33, Dan Beam wrote: > > +noms cuz dragons > > (audible, but understandable, sigh) > > side note: if you include `iron-flex-layout/iron-flex-layout-classes.html` > everywhere, that should define this rule for you, sans dragons. Right, we would have to include that *and* do <style include="iron-positioning"> in every. single. component. Maybe that's the right thing to do, because any component that needs to hide content is, by definition, styling stuff, but the overhead seems crazy.
What is the real impact of the overhead of including it everywhere (like what order of magnitude is it)? On Thu, Aug 11, 2016 at 12:46 AM, <michaelpg@chromium.org> wrote: > On 2016/08/11 07:15:23, noms (inactive) wrote: > > On 2016/08/11 06:09:33, Dan Beam wrote: > > > +noms cuz dragons > > > > (audible, but understandable, sigh) > > > > side note: if you include `iron-flex-layout/iron-flex- > layout-classes.html` > > everywhere, that should define this rule for you, sans dragons. > > Right, we would have to include that *and* do <style > include="iron-positioning"> > in every. single. component. > > Maybe that's the right thing to do, because any component that needs to > hide > content is, by definition, styling stuff, but the overhead seems crazy. > > https://codereview.chromium.org/2231593003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/11 21:10:52, chromium-reviews wrote: > What is the real impact of the overhead of including it everywhere (like > what order of magnitude is it)? According to the Polymer team, ¯\_(ツ)_/¯ I'll try doing it that way, and see what happens. > > On Thu, Aug 11, 2016 at 12:46 AM, <mailto:michaelpg@chromium.org> wrote: > > > On 2016/08/11 07:15:23, noms (inactive) wrote: > > > On 2016/08/11 06:09:33, Dan Beam wrote: > > > > +noms cuz dragons > > > > > > (audible, but understandable, sigh) > > > > > > side note: if you include `iron-flex-layout/iron-flex- > > layout-classes.html` > > > everywhere, that should define this rule for you, sans dragons. > > > > Right, we would have to include that *and* do <style > > include="iron-positioning"> > > in every. single. component. > > > > Maybe that's the right thing to do, because any component that needs to > > hide > > content is, by definition, styling stuff, but the overhead seems crazy. > > > > https://codereview.chromium.org/2231593003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2016/08/12 18:37:04, michaelpg wrote: > On 2016/08/11 21:10:52, chromium-reviews wrote: > > What is the real impact of the overhead of including it everywhere (like > > what order of magnitude is it)? > > According to the Polymer team, ¯\_(ツ)_/¯ > > I'll try doing it that way, and see what happens. > > > > > On Thu, Aug 11, 2016 at 12:46 AM, <mailto:michaelpg@chromium.org> wrote: > > > > > On 2016/08/11 07:15:23, noms (inactive) wrote: > > > > On 2016/08/11 06:09:33, Dan Beam wrote: > > > > > +noms cuz dragons > > > > > > > > (audible, but understandable, sigh) > > > > > > > > side note: if you include `iron-flex-layout/iron-flex- > > > layout-classes.html` > > > > everywhere, that should define this rule for you, sans dragons. > > > > > > Right, we would have to include that *and* do <style > > > include="iron-positioning"> > > > in every. single. component. > > > > > > Maybe that's the right thing to do, because any component that needs to > > > hide > > > content is, by definition, styling stuff, but the overhead seems crazy. > > > > > > https://codereview.chromium.org/2231593003/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. Ack :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
