Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
830743003:
Directly inline shared WebUI CSS declarations into the security and supervised user interstitials. (Closed)
|
Created:
5 years, 11 months ago by Bernhard Bauer Modified:
5 years, 11 months ago CC:
chromium-reviews, jam, pam+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, darin-cc_chromium.org, Dan Beam Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDirectly inline shared WebUI CSS declarations into the security and supervised user interstitials.
BUG=446599, 443203
Committed: https://crrev.com/d03991846b9510bb6b3453aa053e292a0aeabc07
Cr-Commit-Position: refs/heads/master@{#313058}
Patch Set 1 #
Total comments: 15
Patch Set 2 : review #
Total comments: 5
Patch Set 3 : review #
Total comments: 4
Patch Set 4 : . #Patch Set 5 : revert stuff #
Messages
Total messages: 29 (7 generated)
bauerb@chromium.org changed reviewers: + estade@chromium.org
bauerb@chromium.org changed reviewers: + felt@chromium.org
Please review. Thanks!
lgtm
jshin@chromium.org changed reviewers: + jshin@chromium.org
Thanks for the fix. Please, add back 'dir' to html. https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... File chrome/browser/resources/security_warnings/interstitial_v2.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html i18n-values="dir:textdirection;"> You have to keep this because 'dir' attribute in HTML plays a different role than 'text-direction:' in CSS. In most cases, the former works better. Anyway, our CSS has a lot of selectors based on HTML's 'dir' ( body['dir=rtl']) so that we should keep 'dir. https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sup... File chrome/browser/resources/supervised_user_block_interstitial.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sup... chrome/browser/resources/supervised_user_block_interstitial.html:2: <html i18n-values="dir:textdirection"> ditto
https://codereview.chromium.org/830743003/diff/1/ui/base/webui/web_ui_util.h File ui/base/webui/web_ui_util.h (right): https://codereview.chromium.org/830743003/diff/1/ui/base/webui/web_ui_util.h#... ui/base/webui/web_ui_util.h:51: // An alternative is to use AppendWebUICSSTextDefaults() below. nit: If this is being deprecated, should it say so here (for ex "Deprecated in favor of AppendWebUICSSTextDefaults")?
https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... File chrome/browser/resources/security_warnings/interstitial_v2.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html i18n-values="dir:textdirection;"> On 2015/01/10 00:54:04, Jungshik at google wrote: > You have to keep this because 'dir' attribute in HTML plays a different role > than 'text-direction:' in CSS. In most cases, the former works better. > > Anyway, our CSS has a lot of selectors based on HTML's 'dir' ( body['dir=rtl']) > so that we should keep 'dir. Oh, ok. That does mean a few things though: 1. We should remove the fontFamily and fontSize keys from the dictionary returned by SetFontAndTextDirection(), and then rename it to SetTextDirection(). There seem to be some cases left though where those keys are used, so we'll need to fix those first. 2. What is the direction: attribute in the CSS file then used for? Shouldn't we automatically set that attribute via a [dir=rtl] selector somewhere in a shared CSS file? 3. For RTL text, the layout will still be somewhat prone to initial flicker, right? Is there a plan to get rid of that as well? https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sup... File chrome/browser/resources/supervised_user_block_interstitial.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sup... chrome/browser/resources/supervised_user_block_interstitial.html:2: <html i18n-values="dir:textdirection"> On 2015/01/10 00:54:04, Jungshik at google wrote: > ditto > Done. https://codereview.chromium.org/830743003/diff/1/ui/base/webui/web_ui_util.h File ui/base/webui/web_ui_util.h (right): https://codereview.chromium.org/830743003/diff/1/ui/base/webui/web_ui_util.h#... ui/base/webui/web_ui_util.h:51: // An alternative is to use AppendWebUICSSTextDefaults() below. On 2015/01/10 01:00:41, felt wrote: > nit: If this is being deprecated, should it say so here (for ex "Deprecated in > favor of AppendWebUICSSTextDefaults")? Per Jungshik's comment in supervised_user_block_interstitial.html, we can't completely deprecate this method. I've renamed it to SetTextDirection() and mentioned the fact that other usage is deprecated.
https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... File chrome/browser/resources/security_warnings/interstitial_v2.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html i18n-values="dir:textdirection;"> On 2015/01/12 15:47:17, Bernhard Bauer wrote: > On 2015/01/10 00:54:04, Jungshik at google wrote: > > You have to keep this because 'dir' attribute in HTML plays a different role > > than 'text-direction:' in CSS. In most cases, the former works better. > > > > Anyway, our CSS has a lot of selectors based on HTML's 'dir' ( > body['dir=rtl']) > > so that we should keep 'dir. > > Oh, ok. That does mean a few things though: > > 1. We should remove the fontFamily and fontSize keys from the dictionary > returned by SetFontAndTextDirection(), and then rename it to SetTextDirection(). > There seem to be some cases left though where those keys are used, so we'll need > to fix those first. > 2. What is the direction: attribute in the CSS file then used for? Shouldn't we > automatically set that attribute via a [dir=rtl] selector somewhere in a shared > CSS file? > 3. For RTL text, the layout will still be somewhat prone to initial flicker, > right? Is there a plan to get rid of that as well? Jungshik, ping?
On 2015/01/14 18:14:23, Bernhard Bauer wrote: > https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... > File chrome/browser/resources/security_warnings/interstitial_v2.html (left): > > https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... > chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html > i18n-values="dir:textdirection;"> > On 2015/01/12 15:47:17, Bernhard Bauer wrote: > > On 2015/01/10 00:54:04, Jungshik at google wrote: > > > You have to keep this because 'dir' attribute in HTML plays a different role > > > than 'text-direction:' in CSS. In most cases, the former works better. > > > > > > Anyway, our CSS has a lot of selectors based on HTML's 'dir' ( > > body['dir=rtl']) > > > so that we should keep 'dir. > > > > Oh, ok. That does mean a few things though: > > > > 1. We should remove the fontFamily and fontSize keys from the dictionary > > returned by SetFontAndTextDirection(), and then rename it to > SetTextDirection(). > > There seem to be some cases left though where those keys are used, so we'll > need > > to fix those first. > > 2. What is the direction: attribute in the CSS file then used for? Shouldn't > we > > automatically set that attribute via a [dir=rtl] selector somewhere in a > shared > > CSS file? > > 3. For RTL text, the layout will still be somewhat prone to initial flicker, > > right? Is there a plan to get rid of that as well? > > Jungshik, ping? <ping>
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... File chrome/browser/resources/security_warnings/interstitial_v2.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html i18n-values="dir:textdirection;"> On 2015/01/14 18:14:23, Bernhard Bauer wrote: > On 2015/01/12 15:47:17, Bernhard Bauer wrote: > > On 2015/01/10 00:54:04, Jungshik at google wrote: > > > You have to keep this because 'dir' attribute in HTML plays a different role > > > than 'text-direction:' in CSS. In most cases, the former works better. > > > > > > Anyway, our CSS has a lot of selectors based on HTML's 'dir' ( > > body['dir=rtl']) > > > so that we should keep 'dir. > > > > Oh, ok. That does mean a few things though: > > > > 1. We should remove the fontFamily and fontSize keys from the dictionary > > returned by SetFontAndTextDirection(), and then rename it to > SetTextDirection(). > > There seem to be some cases left though where those keys are used, so we'll > need > > to fix those first. > > 2. What is the direction: attribute in the CSS file then used for? Shouldn't > we > > automatically set that attribute via a [dir=rtl] selector somewhere in a > shared > > CSS file? this pattern is common: default styles { make a box anchored to the left } if rtl { anchor it to the right by flipping some mutually exclusive styles } [dir='rtl'] is our current way to do "if rtl", which keys off an explicit HTML attribute being set. I don't know a way this can be accomplished via CSS only (as I don't know if CSS has the ability to derive the computed direction, just key off of stuff set by JS or HTML, e.g. classes and attributes). > > 3. For RTL text, the layout will still be somewhat prone to initial flicker, > > right? Is there a plan to get rid of that as well? I'm unsure of how to fix flicker with our current approach other than pre-processing. > > Jungshik, ping? https://codereview.chromium.org/830743003/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/830743003/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:275: .as_string(); i don't really care much, and this could be clant-format's fault, but usually operators (e.g. dot) go at the end in Chrome C++ in my experience (has been through some churn lately...) https://codereview.chromium.org/830743003/diff/20001/ui/base/webui/web_ui_uti... File ui/base/webui/web_ui_util.cc (right): https://codereview.chromium.org/830743003/diff/20001/ui/base/webui/web_ui_uti... ui/base/webui/web_ui_util.cc:140: std::string GetWebUICSSTextDefaults() { nit: GetWebUICssTextDefaults or GetWebUiCssTextDefaults
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
On 2015/01/19 09:37:52, Bernhard Bauer wrote: > On 2015/01/14 18:14:23, Bernhard Bauer wrote: > > > https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... > > File chrome/browser/resources/security_warnings/interstitial_v2.html (left): > > > > > https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... > > chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html > > i18n-values="dir:textdirection;"> > > On 2015/01/12 15:47:17, Bernhard Bauer wrote: > > > On 2015/01/10 00:54:04, Jungshik at google wrote: > > > > You have to keep this because 'dir' attribute in HTML plays a different > role > > > > than 'text-direction:' in CSS. In most cases, the former works better. > > > > > > > > Anyway, our CSS has a lot of selectors based on HTML's 'dir' ( > > > body['dir=rtl']) > > > > so that we should keep 'dir. > > > > > > Oh, ok. That does mean a few things though: > > > > > > 1. We should remove the fontFamily and fontSize keys from the dictionary > > > returned by SetFontAndTextDirection(), and then rename it to > > SetTextDirection(). > > > There seem to be some cases left though where those keys are used, so we'll > > need > > > to fix those first. > > > 2. What is the direction: attribute in the CSS file then used for? > Shouldn't > > we > > > automatically set that attribute via a [dir=rtl] selector somewhere in a > > shared > > > CSS file? > > > 3. For RTL text, the layout will still be somewhat prone to initial > flicker, > > > right? Is there a plan to get rid of that as well? > > > > Jungshik, ping? > > <ping> I'm very sorry that this CL got out of my attention. Dan answered the question about rtl. I'm afraid we have to live with flipping for a while. As for fontFamily/fontSize, we talked about getting rid of them in xdai's mega CL, but punted because there are still some use cases. A couple of more have popped up since. For instance, CrOS OOBE cannot rely on text_default.css because it does not respond to a locale change (i.e. the value in text_default.css is not refreshed upon a locale change while the JS mechanism does) So, we have to live with them until we find a way to resolve that issue. having said that, LGTM
On 2015/01/21 22:01:04, Jungshik at google wrote: > As for fontFamily/fontSize, we talked about getting rid of them in xdai's mega > CL, but punted because there are still some use cases. A couple of more have > popped up since. For instance, CrOS OOBE cannot rely on text_default.css because > it does not respond to a locale change (i.e. the value in text_default.css is > not refreshed upon a locale change while the JS mechanism does) So, we have to > live with them until we find a way to resolve that issue. Hm... I wonder whether we could programmatically change the CSS rules when the locale changes :) Alternatively, just do both: Set the initial values via text_default.css, and then provide overrides via 18n-template? https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... File chrome/browser/resources/security_warnings/interstitial_v2.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html i18n-values="dir:textdirection;"> On 2015/01/21 19:21:41, Dan Beam wrote: > On 2015/01/14 18:14:23, Bernhard Bauer wrote: > > On 2015/01/12 15:47:17, Bernhard Bauer wrote: > > > On 2015/01/10 00:54:04, Jungshik at google wrote: > > > > You have to keep this because 'dir' attribute in HTML plays a different > role > > > > than 'text-direction:' in CSS. In most cases, the former works better. > > > > > > > > Anyway, our CSS has a lot of selectors based on HTML's 'dir' ( > > > body['dir=rtl']) > > > > so that we should keep 'dir. > > > > > > Oh, ok. That does mean a few things though: > > > > > > 1. We should remove the fontFamily and fontSize keys from the dictionary > > > returned by SetFontAndTextDirection(), and then rename it to > > SetTextDirection(). > > > There seem to be some cases left though where those keys are used, so we'll > > need > > > to fix those first. > > > 2. What is the direction: attribute in the CSS file then used for? > Shouldn't > > we > > > automatically set that attribute via a [dir=rtl] selector somewhere in a > > shared > > > CSS file? > > this pattern is common: > > default styles { > make a box anchored to the left > } > > if rtl { > anchor it to the right by flipping some mutually exclusive styles > } > > [dir='rtl'] is our current way to do "if rtl", which keys off an explicit HTML > attribute being set. > > I don't know a way this can be accomplished via CSS only (as I don't know if CSS > has the ability to derive the computed direction, just key off of stuff set by > JS or HTML, e.g. classes and attributes). We could still do: [dir='rtl'] { direction: rtl; } Right? > > > 3. For RTL text, the layout will still be somewhat prone to initial > flicker, > > > right? Is there a plan to get rid of that as well? > > I'm unsure of how to fix flicker with our current approach other than > pre-processing. Yeah, that's unfortunate. So, if we need to do preprocessing, and we need to set an attribute on the <html> tag, could we do that via preprocessing? Essentially, do a s/^<html>$/<html dir="$direction">/ ? > > > > Jungshik, ping? > https://codereview.chromium.org/830743003/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/830743003/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:275: .as_string(); On 2015/01/21 19:21:41, Dan Beam wrote: > i don't really care much, and this could be clant-format's fault, but usually > operators (e.g. dot) go at the end in Chrome C++ in my experience (has been > through some churn lately...) Hm, I think for method calls the dot on the beginning of the line is accepted. It also seems to be more common in existing code (I had to dust off my Chomsky-type-3 knowledge, because codesearch regexps don't do lookahead assertions): compare the 1800 results for https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=%... with the 620 results for https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=l... , which includes false positives. https://codereview.chromium.org/830743003/diff/20001/ui/base/webui/web_ui_uti... File ui/base/webui/web_ui_util.cc (right): https://codereview.chromium.org/830743003/diff/20001/ui/base/webui/web_ui_uti... ui/base/webui/web_ui_util.cc:140: std::string GetWebUICSSTextDefaults() { On 2015/01/21 19:21:41, Dan Beam wrote: > nit: GetWebUICssTextDefaults or GetWebUiCssTextDefaults Ack, will do that tomorrow.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... File chrome/browser/resources/security_warnings/interstitial_v2.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html i18n-values="dir:textdirection;"> On 2015/01/21 22:47:50, Bernhard Bauer wrote: > On 2015/01/21 19:21:41, Dan Beam wrote: > > On 2015/01/14 18:14:23, Bernhard Bauer wrote: > > > On 2015/01/12 15:47:17, Bernhard Bauer wrote: > > > > On 2015/01/10 00:54:04, Jungshik at google wrote: > > > > > You have to keep this because 'dir' attribute in HTML plays a different > > role > > > > > than 'text-direction:' in CSS. In most cases, the former works better. > > > > > > > > > > Anyway, our CSS has a lot of selectors based on HTML's 'dir' ( > > > > body['dir=rtl']) > > > > > so that we should keep 'dir. > > > > > > > > Oh, ok. That does mean a few things though: > > > > > > > > 1. We should remove the fontFamily and fontSize keys from the dictionary > > > > returned by SetFontAndTextDirection(), and then rename it to > > > SetTextDirection(). > > > > There seem to be some cases left though where those keys are used, so > we'll > > > need > > > > to fix those first. > > > > 2. What is the direction: attribute in the CSS file then used for? > > Shouldn't > > > we > > > > automatically set that attribute via a [dir=rtl] selector somewhere in a > > > shared > > > > CSS file? > > > > this pattern is common: > > > > default styles { > > make a box anchored to the left > > } > > > > if rtl { > > anchor it to the right by flipping some mutually exclusive styles > > } > > > > [dir='rtl'] is our current way to do "if rtl", which keys off an explicit HTML > > attribute being set. > > > > I don't know a way this can be accomplished via CSS only (as I don't know if > CSS > > has the ability to derive the computed direction, just key off of stuff set by > > JS or HTML, e.g. classes and attributes). > > We could still do: > > [dir='rtl'] { ^ how does this selector match? > direction: rtl; > } > > Right? > > > > > 3. For RTL text, the layout will still be somewhat prone to initial > > flicker, > > > > right? Is there a plan to get rid of that as well? > > > > I'm unsure of how to fix flicker with our current approach other than > > pre-processing. > > Yeah, that's unfortunate. > > So, if we need to do preprocessing, and we need to set an attribute on the > <html> tag, could we do that via preprocessing? Essentially, do a > s/^<html>$/<html dir="$direction">/ ? that is more or less what we'd want to do, yes > > > > > > > Jungshik, ping? > > >
https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... File chrome/browser/resources/security_warnings/interstitial_v2.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html i18n-values="dir:textdirection;"> On 2015/01/22 00:58:48, Dan Beam wrote: > On 2015/01/21 22:47:50, Bernhard Bauer wrote: > > On 2015/01/21 19:21:41, Dan Beam wrote: > > > On 2015/01/14 18:14:23, Bernhard Bauer wrote: > > > > On 2015/01/12 15:47:17, Bernhard Bauer wrote: > > > > > On 2015/01/10 00:54:04, Jungshik at google wrote: > > > > > > You have to keep this because 'dir' attribute in HTML plays a > different > > > role > > > > > > than 'text-direction:' in CSS. In most cases, the former works better. > > > > > > > > > > > > Anyway, our CSS has a lot of selectors based on HTML's 'dir' ( > > > > > body['dir=rtl']) > > > > > > so that we should keep 'dir. > > > > > > > > > > Oh, ok. That does mean a few things though: > > > > > > > > > > 1. We should remove the fontFamily and fontSize keys from the > dictionary > > > > > returned by SetFontAndTextDirection(), and then rename it to > > > > SetTextDirection(). > > > > > There seem to be some cases left though where those keys are used, so > > we'll > > > > need > > > > > to fix those first. > > > > > 2. What is the direction: attribute in the CSS file then used for? > > > Shouldn't > > > > we > > > > > automatically set that attribute via a [dir=rtl] selector somewhere in a > > > > shared > > > > > CSS file? > > > > > > this pattern is common: > > > > > > default styles { > > > make a box anchored to the left > > > } > > > > > > if rtl { > > > anchor it to the right by flipping some mutually exclusive styles > > > } > > > > > > [dir='rtl'] is our current way to do "if rtl", which keys off an explicit > HTML > > > attribute being set. > > > > > > I don't know a way this can be accomplished via CSS only (as I don't know if > > CSS > > > has the ability to derive the computed direction, just key off of stuff set > by > > > JS or HTML, e.g. classes and attributes). > > > > We could still do: > > > > [dir='rtl'] { > > ^ how does this selector match? to be more specific (and less rhetorical), currently: <html i18n-values="dir:textdirection"> adds dir="rtl" when JavaScript runs and enables [dir='rtl'] selectors. how would only CSS apply RTL-only rules without pre-processing (either at compile or runtime) all our CSS? > > > direction: rtl; > > } > > > > Right? > > > > > > > 3. For RTL text, the layout will still be somewhat prone to initial > > > flicker, > > > > > right? Is there a plan to get rid of that as well? > > > > > > I'm unsure of how to fix flicker with our current approach other than > > > pre-processing. > > > > Yeah, that's unfortunate. > > > > So, if we need to do preprocessing, and we need to set an attribute on the > > <html> tag, could we do that via preprocessing? Essentially, do a > > s/^<html>$/<html dir="$direction">/ ? > > that is more or less what we'd want to do, yes > > > > > > > > > > > Jungshik, ping? > > > > >
https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... File chrome/browser/resources/security_warnings/interstitial_v2.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html i18n-values="dir:textdirection;"> On 2015/01/22 01:14:05, Dan Beam wrote: > to be more specific (and less rhetorical), currently: > > <html i18n-values="dir:textdirection"> > > adds dir="rtl" when JavaScript runs and enables [dir='rtl'] selectors. > > how would only CSS apply RTL-only rules without pre-processing (either at > compile or runtime) all our CSS? Sorry, I meant moving the `direction: $1;` line from text_defaults.css into a [dir='rtl'] block in a CSS file that is not preprocessed. https://codereview.chromium.org/830743003/diff/20001/ui/base/webui/web_ui_uti... File ui/base/webui/web_ui_util.cc (right): https://codereview.chromium.org/830743003/diff/20001/ui/base/webui/web_ui_uti... ui/base/webui/web_ui_util.cc:140: std::string GetWebUICSSTextDefaults() { On 2015/01/21 22:47:50, Bernhard Bauer wrote: > On 2015/01/21 19:21:41, Dan Beam wrote: > > nit: GetWebUICssTextDefaults or GetWebUiCssTextDefaults > > Ack, will do that tomorrow. Done.
lgtm https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... File chrome/browser/resources/security_warnings/interstitial_v2.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html i18n-values="dir:textdirection;"> On 2015/01/22 10:12:35, Bernhard Bauer wrote: > On 2015/01/22 01:14:05, Dan Beam wrote: > > to be more specific (and less rhetorical), currently: > > > > <html i18n-values="dir:textdirection"> > > > > adds dir="rtl" when JavaScript runs and enables [dir='rtl'] selectors. > > > > how would only CSS apply RTL-only rules without pre-processing (either at > > compile or runtime) all our CSS? > > Sorry, I meant moving the `direction: $1;` line from text_defaults.css into a > [dir='rtl'] block in a CSS file that is not preprocessed. some things I think are clear but I just want to make sure: css works by overriding (cascading). so you set up some default styles and then have some overriding styles later based on your environment (which generally changes based on selectors changing, which is generally from JS, HTML, or stuff like :hover). right now an overwhelming majority of chrome users are LTR, so the page layouts are LTR by default. there are some styles that are mutually exclusive (e.g. positioning in CSS is inherently LTR, as it has a top-left origin for its coordinate system). consider: .frobber { right: 100px; } at run-time (currently), we determine that the locale is RTL, so we change the direction of the <html> element by setting dir="rtl" [1][2]. this is done with i18n-values="dir:textdirection" (which you're proposing we remove with your patch). these types of selectors now apply: [dir=rtl] .frobber { right: auto; /* cancels out the previous rule's right property */ left: 100px; } for any element where dir=rtl, built-in user-agent styles apply: [dir=rtl] { direction: rtl; } so there's no reason to have this rule in our application's CSS, as the browser already does it for us. text_defaults.css has `direction: $1` to minimize flicker. JS sets the dir="" attribute (which makes the direction correct), but the page can be painted 1+ times before JS kicks in. [1] https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... [2] https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/webui/web_... https://codereview.chromium.org/830743003/diff/40001/ui/base/webui/web_ui_util.h File ui/base/webui/web_ui_util.h (right): https://codereview.chromium.org/830743003/diff/40001/ui/base/webui/web_ui_uti... ui/base/webui/web_ui_util.h:53: UI_BASE_EXPORT void SetTextDirection(base::DictionaryValue* localized_strings); nit: rename this when it only sets text direction (it still sets font stuff too...)
https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... File chrome/browser/resources/security_warnings/interstitial_v2.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html i18n-values="dir:textdirection;"> On 2015/01/22 17:08:41, Dan Beam wrote: > On 2015/01/22 10:12:35, Bernhard Bauer wrote: > > On 2015/01/22 01:14:05, Dan Beam wrote: > > > to be more specific (and less rhetorical), currently: > > > > > > <html i18n-values="dir:textdirection"> > > > > > > adds dir="rtl" when JavaScript runs and enables [dir='rtl'] selectors. > > > > > > how would only CSS apply RTL-only rules without pre-processing (either at > > > compile or runtime) all our CSS? > > > > Sorry, I meant moving the `direction: $1;` line from text_defaults.css into a > > [dir='rtl'] block in a CSS file that is not preprocessed. > > some things I think are clear but I just want to make sure: > > css works by overriding (cascading). so you set up some default styles and then > have some overriding styles later based on your environment (which generally > changes based on selectors changing, which is generally from JS, HTML, or stuff > like :hover). > > right now an overwhelming majority of chrome users are LTR, so the page layouts > are LTR by default. there are some styles that are mutually exclusive (e.g. > positioning in CSS is inherently LTR, as it has a top-left origin for its > coordinate system). consider: > > .frobber { > right: 100px; > } > > at run-time (currently), we determine that the locale is RTL, so we change the > direction of the <html> element by setting dir="rtl" [1][2]. this is done with > > i18n-values="dir:textdirection" > > (which you're proposing we remove with your patch). these types of selectors which you *were* > now apply: > > [dir=rtl] .frobber { > right: auto; /* cancels out the previous rule's right property */ > left: 100px; > } > > for any element where dir=rtl, built-in user-agent styles apply: > > [dir=rtl] { > direction: rtl; > } > > so there's no reason to have this rule in our application's CSS, as the browser > already does it for us. > > text_defaults.css has `direction: $1` to minimize flicker. JS sets the dir="" > attribute (which makes the direction correct), but the page can be painted 1+ > times before JS kicks in. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/webui/web_...
Adrienne, do you want to look at security_interstitial_page.cc? https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... File chrome/browser/resources/security_warnings/interstitial_v2.html (left): https://codereview.chromium.org/830743003/diff/1/chrome/browser/resources/sec... chrome/browser/resources/security_warnings/interstitial_v2.html:2: <html i18n-values="dir:textdirection;"> On 2015/01/22 17:08:41, Dan Beam wrote: > On 2015/01/22 10:12:35, Bernhard Bauer wrote: > > On 2015/01/22 01:14:05, Dan Beam wrote: > > > to be more specific (and less rhetorical), currently: > > > > > > <html i18n-values="dir:textdirection"> > > > > > > adds dir="rtl" when JavaScript runs and enables [dir='rtl'] selectors. > > > > > > how would only CSS apply RTL-only rules without pre-processing (either at > > > compile or runtime) all our CSS? > > > > Sorry, I meant moving the `direction: $1;` line from text_defaults.css into a > > [dir='rtl'] block in a CSS file that is not preprocessed. > > some things I think are clear but I just want to make sure: > > css works by overriding (cascading). so you set up some default styles and then > have some overriding styles later based on your environment (which generally > changes based on selectors changing, which is generally from JS, HTML, or stuff > like :hover). > > right now an overwhelming majority of chrome users are LTR, so the page layouts > are LTR by default. there are some styles that are mutually exclusive (e.g. > positioning in CSS is inherently LTR, as it has a top-left origin for its > coordinate system). consider: > > .frobber { > right: 100px; > } > > at run-time (currently), we determine that the locale is RTL, so we change the > direction of the <html> element by setting dir="rtl" [1][2]. this is done with > > i18n-values="dir:textdirection" > > (which you're proposing we remove with your patch). these types of selectors > now apply: > > [dir=rtl] .frobber { > right: auto; /* cancels out the previous rule's right property */ > left: 100px; > } > > for any element where dir=rtl, built-in user-agent styles apply: > > [dir=rtl] { > direction: rtl; > } > > so there's no reason to have this rule in our application's CSS, as the browser > already does it for us. > > text_defaults.css has `direction: $1` to minimize flicker. JS sets the dir="" > attribute (which makes the direction correct), but the page can be painted 1+ > times before JS kicks in. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/webui/web_... Gotcha, thanks for the explanation! Once we have a no-flicker solution for setting dir=rtl, we should be able to remove the CSS declaration. https://codereview.chromium.org/830743003/diff/40001/ui/base/webui/web_ui_util.h File ui/base/webui/web_ui_util.h (right): https://codereview.chromium.org/830743003/diff/40001/ui/base/webui/web_ui_uti... ui/base/webui/web_ui_util.h:53: UI_BASE_EXPORT void SetTextDirection(base::DictionaryValue* localized_strings); On 2015/01/22 17:08:41, Dan Beam wrote: > nit: rename this when it only sets text direction (it still sets font stuff > too...) Hm, I did that on purpose now instead of later, so that people wouldn't use it in more places to set the font attributes, when we want to get rid of the existing places :)
https://codereview.chromium.org/830743003/diff/40001/ui/base/webui/web_ui_util.h File ui/base/webui/web_ui_util.h (right): https://codereview.chromium.org/830743003/diff/40001/ui/base/webui/web_ui_uti... ui/base/webui/web_ui_util.h:53: UI_BASE_EXPORT void SetTextDirection(base::DictionaryValue* localized_strings); On 2015/01/22 17:36:17, Bernhard Bauer wrote: > On 2015/01/22 17:08:41, Dan Beam wrote: > > nit: rename this when it only sets text direction (it still sets font stuff > > too...) > > Hm, I did that on purpose now instead of later, so that people wouldn't use it > in more places to set the font attributes, when we want to get rid of the > existing places :) i can see both points of view here, but i've always been told "plan for later, code for now". just a nit.
Adrienne, ping? https://codereview.chromium.org/830743003/diff/40001/ui/base/webui/web_ui_util.h File ui/base/webui/web_ui_util.h (right): https://codereview.chromium.org/830743003/diff/40001/ui/base/webui/web_ui_uti... ui/base/webui/web_ui_util.h:53: UI_BASE_EXPORT void SetTextDirection(base::DictionaryValue* localized_strings); On 2015/01/22 17:47:58, Dan Beam wrote: > On 2015/01/22 17:36:17, Bernhard Bauer wrote: > > On 2015/01/22 17:08:41, Dan Beam wrote: > > > nit: rename this when it only sets text direction (it still sets font stuff > > > too...) > > > > Hm, I did that on purpose now instead of later, so that people wouldn't use it > > in more places to set the font attributes, when we want to get rid of the > > existing places :) > > i can see both points of view here, but i've always been told "plan for later, > code for now". just a nit. Fair enough. Done!
chrome/browser/interstitials/security_interstitial_page.cc lgtm
The CQ bit was checked by bauerb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/830743003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d03991846b9510bb6b3453aa053e292a0aeabc07 Cr-Commit-Position: refs/heads/master@{#313058} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
