chrome/browser/resources/security_warnings/interstitial_v2.html - Issue 830743003: Directly inline shared WebUI CSS declarations into the security and supervised user interstitials.

Unified Diff: chrome/browser/resources/security_warnings/interstitial_v2.html

Issue 830743003: Directly inline shared WebUI CSS declarations into the security and supervised user interstitials. (Closed) Base URL: https://chromium.googlesource.com/chromium/src@master

Patch Set: Created 5 years, 11 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« no previous file with comments | « chrome/browser/interstitials/security_interstitial_page.cc ('k') | chrome/browser/resources/supervised_user_block_interstitial.html » ('j') | chrome/browser/resources/supervised_user_block_interstitial.html » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/resources/security_warnings/interstitial_v2.html

diff --git a/chrome/browser/resources/security_warnings/interstitial_v2.html b/chrome/browser/resources/security_warnings/interstitial_v2.html

index cc81404920fc4634f7a7b335f35ed04aa0a89883..1fb16d1e8201ce46b152ba7a49f66ff8686dbe56 100644

--- a/chrome/browser/resources/security_warnings/interstitial_v2.html

+++ b/chrome/browser/resources/security_warnings/interstitial_v2.html

@@ -1,5 +1,5 @@

<!doctype html>

-<html i18n-values="dir:textdirection;">

jungshik at Google 2015/01/10 00:54:04 You have to keep this because 'dir' attribute in H

Bernhard Bauer 2015/01/12 15:47:17 Oh, ok. That does mean a few things though: 1. W

Bernhard Bauer 2015/01/14 18:14:23 Jungshik, ping?

Dan Beam 2015/01/21 19:21:41 this pattern is common: default styles { ma

Bernhard Bauer 2015/01/21 22:47:50 We could still do: [dir='rtl'] { direction:

We could still do: [dir='rtl'] { direction: rtl; } Right?

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">/ ?

Dan Beam 2015/01/22 00:58:48 ^ how does this selector match?

^ how does this selector match?

that is more or less what we'd want to do, yes

Dan Beam 2015/01/22 01:14:05 to be more specific (and less rhetorical), current

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?

Bernhard Bauer 2015/01/22 10:12:35 Sorry, I meant moving the `direction: $1;` line fr

Dan Beam 2015/01/22 17:08:41 some things I think are clear but I just want to m

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_...

Dan Beam 2015/01/22 17:09:29 which you *were*

which you *were*

Bernhard Bauer 2015/01/22 17:36:17 Gotcha, thanks for the explanation! Once we have

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.

+<html>

<head>

@@ -11,9 +11,7 @@

</head>

-<!-- TODO(jshin): Get rid of i18n-values and import default_text.css instead

- once interstitial page loading does not use data url any more. -->

-<body id="body" i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize">

+<body id="body">