Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
842973002:
Restore security warning font size to correct size (Closed)
|
Created:
5 years, 11 months ago by felt Modified:
5 years, 11 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, arv+watch_chromium.org, edwardjung Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRestore security warning font size to correct size
A regression broke both the font face and size. The "fix" fixed the
font face, but not the font size. This restores the size.
BUG=443203
Committed: https://crrev.com/51e49b0faba37e3dc20809f10c49fd922c39d581
Cr-Commit-Position: refs/heads/master@{#310839}
Patch Set 1 #Patch Set 2 : Now using important keyword #Patch Set 3 : Semicolon #Patch Set 4 : Math #Patch Set 5 : math is hard #Patch Set 6 : Moved to interstitial wrapper #Patch Set 7 : Moved to html #Messages
Total messages: 20 (2 generated)
felt@chromium.org changed reviewers: + bauerb@chromium.org
bernhard, PTAL.
Oh, hm, this is going to be a problem when we move back to the shared WebUI CSS styles, because those define font-size on the <body>. So, is what we really want to do here not to make the WebUI default font size apply to this interstitial? Could we do that by giving the custom font-size a higher precedence?
On 2015/01/09 17:41:44, Bernhard Bauer wrote: > Oh, hm, this is going to be a problem when we move back to the shared WebUI CSS > styles, because those define font-size on the <body>. > > So, is what we really want to do here not to make the WebUI default font size > apply to this interstitial? Could we do that by giving the custom font-size a > higher precedence? The designer asked me to make it 125% of the standard font-size, whatever the standard font-size is for that OS. We can achieve this by importing the standard font size on the <html> tag and then boosting it in the <body> tag to 125%. Is there another way to accomplish this?
edward, how have you set the font size for the neterror pages?
On 2015/01/09 17:41:44, Bernhard Bauer wrote: > Oh, hm, this is going to be a problem when we move back to the shared WebUI CSS > styles, because those define font-size on the <body>. > > So, is what we really want to do here not to make the WebUI default font size > apply to this interstitial? Could we do that by giving the custom font-size a > higher precedence? I think the easiest way to do that would be to add !important (and a comment why it's necessary).
OK, so I did that and it works...ish. The font is now bigger... but it's also now bigger than it was before, pre-original-regression. Somehow this 125% is different from the 125% you get with the fontsize on the html tag. I am baffled.
OK, I figured this out. Here's what's going on.
The different font sizes...
Original: font-size 15px (base is 12px)
Now, without !important: font-size 12px (base is 12px)
Now, with !important: font-size 20px (base is 16px)
So the question is, why is the !important keyword changing the base?
Answer: if you take a look at the font_defaults.css file, it has:
body {
font-size: 75%;
}
Which resets the base from 16px to 12px.
When I use the !important keyword, it removes that so 125% is computed from the
original 16px instead of the altered 12px.
In the latest patchset, I solved this by doing
body {
font-size: .9375% !important;
}
Which is the equivalent of .75*1.25, but this is kind of a hack.
On 2015/01/09 18:32:14, felt wrote:
> OK, I figured this out. Here's what's going on.
>
> The different font sizes...
> Original: font-size 15px (base is 12px)
> Now, without !important: font-size 12px (base is 12px)
> Now, with !important: font-size 20px (base is 16px)
>
> So the question is, why is the !important keyword changing the base?
>
> Answer: if you take a look at the font_defaults.css file, it has:
> body {
> font-size: 75%;
> }
> Which resets the base from 16px to 12px.
>
> When I use the !important keyword, it removes that so 125% is computed from
the
> original 16px instead of the altered 12px.
>
> In the latest patchset, I solved this by doing
> body {
> font-size: .9375% !important;
> }
> Which is the equivalent of .75*1.25, but this is kind of a hack.
Hm... would it work to add a big <div> around everything in the <body> and set
the font-size on that?
Also, interestingly enough, IDS_WEB_FONT_SIZE is 75% on all platforms... except
under Windows XP in Bengali. o_O
On 2015/01/09 18:40:37, Bernhard Bauer wrote:
> On 2015/01/09 18:32:14, felt wrote:
> > OK, I figured this out. Here's what's going on.
> >
> > The different font sizes...
> > Original: font-size 15px (base is 12px)
> > Now, without !important: font-size 12px (base is 12px)
> > Now, with !important: font-size 20px (base is 16px)
> >
> > So the question is, why is the !important keyword changing the base?
> >
> > Answer: if you take a look at the font_defaults.css file, it has:
> > body {
> > font-size: 75%;
> > }
> > Which resets the base from 16px to 12px.
> >
> > When I use the !important keyword, it removes that so 125% is computed from
> the
> > original 16px instead of the altered 12px.
> >
> > In the latest patchset, I solved this by doing
> > body {
> > font-size: .9375% !important;
> > }
> > Which is the equivalent of .75*1.25, but this is kind of a hack.
>
> Hm... would it work to add a big <div> around everything in the <body> and set
> the font-size on that?
Or, even simpler: Just set the 125% on the <html> tag! :) For the factors the
order doesn't seem to matter anyway.
On 2015/01/09 18:40:37, Bernhard Bauer wrote:
> On 2015/01/09 18:32:14, felt wrote:
> > OK, I figured this out. Here's what's going on.
> >
> > The different font sizes...
> > Original: font-size 15px (base is 12px)
> > Now, without !important: font-size 12px (base is 12px)
> > Now, with !important: font-size 20px (base is 16px)
> >
> > So the question is, why is the !important keyword changing the base?
> >
> > Answer: if you take a look at the font_defaults.css file, it has:
> > body {
> > font-size: 75%;
> > }
> > Which resets the base from 16px to 12px.
> >
> > When I use the !important keyword, it removes that so 125% is computed from
> the
> > original 16px instead of the altered 12px.
> >
> > In the latest patchset, I solved this by doing
> > body {
> > font-size: .9375% !important;
> > }
> > Which is the equivalent of .75*1.25, but this is kind of a hack.
>
> Hm... would it work to add a big <div> around everything in the <body> and set
> the font-size on that?
>
Ah yes, that does work too. Moved it to a wrapper.
> Also, interestingly enough, IDS_WEB_FONT_SIZE is 75% on all platforms...
except
> under Windows XP in Bengali. o_O
I wonder if they should just make it 12px instead of 16px to start with then...
On 2015/01/09 18:57:20, felt wrote:
> On 2015/01/09 18:40:37, Bernhard Bauer wrote:
> > On 2015/01/09 18:32:14, felt wrote:
> > > OK, I figured this out. Here's what's going on.
> > >
> > > The different font sizes...
> > > Original: font-size 15px (base is 12px)
> > > Now, without !important: font-size 12px (base is 12px)
> > > Now, with !important: font-size 20px (base is 16px)
> > >
> > > So the question is, why is the !important keyword changing the base?
> > >
> > > Answer: if you take a look at the font_defaults.css file, it has:
> > > body {
> > > font-size: 75%;
> > > }
> > > Which resets the base from 16px to 12px.
> > >
> > > When I use the !important keyword, it removes that so 125% is computed
from
> > the
> > > original 16px instead of the altered 12px.
> > >
> > > In the latest patchset, I solved this by doing
> > > body {
> > > font-size: .9375% !important;
> > > }
> > > Which is the equivalent of .75*1.25, but this is kind of a hack.
> >
> > Hm... would it work to add a big <div> around everything in the <body> and
set
> > the font-size on that?
> >
>
> Ah yes, that does work too. Moved it to a wrapper.
>
> > Also, interestingly enough, IDS_WEB_FONT_SIZE is 75% on all platforms...
> except
> > under Windows XP in Bengali. o_O
>
> I wonder if they should just make it 12px instead of 16px to start with
then...
I think 16px is the user-agent default, which means changing it would change it
for _all_ websites.
On 2015/01/09 18:59:47, Bernhard Bauer wrote:
> On 2015/01/09 18:57:20, felt wrote:
> > On 2015/01/09 18:40:37, Bernhard Bauer wrote:
> > > On 2015/01/09 18:32:14, felt wrote:
> > > > OK, I figured this out. Here's what's going on.
> > > >
> > > > The different font sizes...
> > > > Original: font-size 15px (base is 12px)
> > > > Now, without !important: font-size 12px (base is 12px)
> > > > Now, with !important: font-size 20px (base is 16px)
> > > >
> > > > So the question is, why is the !important keyword changing the base?
> > > >
> > > > Answer: if you take a look at the font_defaults.css file, it has:
> > > > body {
> > > > font-size: 75%;
> > > > }
> > > > Which resets the base from 16px to 12px.
> > > >
> > > > When I use the !important keyword, it removes that so 125% is computed
> from
> > > the
> > > > original 16px instead of the altered 12px.
> > > >
> > > > In the latest patchset, I solved this by doing
> > > > body {
> > > > font-size: .9375% !important;
> > > > }
> > > > Which is the equivalent of .75*1.25, but this is kind of a hack.
> > >
> > > Hm... would it work to add a big <div> around everything in the <body> and
> set
> > > the font-size on that?
> > >
> >
> > Ah yes, that does work too. Moved it to a wrapper.
> >
> > > Also, interestingly enough, IDS_WEB_FONT_SIZE is 75% on all platforms...
> > except
> > > under Windows XP in Bengali. o_O
> >
> > I wonder if they should just make it 12px instead of 16px to start with
> then...
>
> I think 16px is the user-agent default, which means changing it would change
it
> for _all_ websites.
Are you fine with this approach (125% on the pre-existing wrapper)? I'd like to
land this before branch if I can to avoid needing a merge next week.
On 2015/01/09 19:01:46, felt wrote:
> On 2015/01/09 18:59:47, Bernhard Bauer wrote:
> > On 2015/01/09 18:57:20, felt wrote:
> > > On 2015/01/09 18:40:37, Bernhard Bauer wrote:
> > > > On 2015/01/09 18:32:14, felt wrote:
> > > > > OK, I figured this out. Here's what's going on.
> > > > >
> > > > > The different font sizes...
> > > > > Original: font-size 15px (base is 12px)
> > > > > Now, without !important: font-size 12px (base is 12px)
> > > > > Now, with !important: font-size 20px (base is 16px)
> > > > >
> > > > > So the question is, why is the !important keyword changing the base?
> > > > >
> > > > > Answer: if you take a look at the font_defaults.css file, it has:
> > > > > body {
> > > > > font-size: 75%;
> > > > > }
> > > > > Which resets the base from 16px to 12px.
> > > > >
> > > > > When I use the !important keyword, it removes that so 125% is computed
> > from
> > > > the
> > > > > original 16px instead of the altered 12px.
> > > > >
> > > > > In the latest patchset, I solved this by doing
> > > > > body {
> > > > > font-size: .9375% !important;
> > > > > }
> > > > > Which is the equivalent of .75*1.25, but this is kind of a hack.
> > > >
> > > > Hm... would it work to add a big <div> around everything in the <body>
and
> > set
> > > > the font-size on that?
> > > >
> > >
> > > Ah yes, that does work too. Moved it to a wrapper.
> > >
> > > > Also, interestingly enough, IDS_WEB_FONT_SIZE is 75% on all platforms...
> > > except
> > > > under Windows XP in Bengali. o_O
> > >
> > > I wonder if they should just make it 12px instead of 16px to start with
> > then...
> >
> > I think 16px is the user-agent default, which means changing it would change
> it
> > for _all_ websites.
>
> Are you fine with this approach (125% on the pre-existing wrapper)? I'd like
to
> land this before branch if I can to avoid needing a merge next week.
Yes, this LGTM (the 125% on the <html> approach as well, FWIW).
On 2015/01/09 19:03:53, Bernhard Bauer wrote:
> On 2015/01/09 19:01:46, felt wrote:
> > On 2015/01/09 18:59:47, Bernhard Bauer wrote:
> > > On 2015/01/09 18:57:20, felt wrote:
> > > > On 2015/01/09 18:40:37, Bernhard Bauer wrote:
> > > > > On 2015/01/09 18:32:14, felt wrote:
> > > > > > OK, I figured this out. Here's what's going on.
> > > > > >
> > > > > > The different font sizes...
> > > > > > Original: font-size 15px (base is 12px)
> > > > > > Now, without !important: font-size 12px (base is 12px)
> > > > > > Now, with !important: font-size 20px (base is 16px)
> > > > > >
> > > > > > So the question is, why is the !important keyword changing the base?
> > > > > >
> > > > > > Answer: if you take a look at the font_defaults.css file, it has:
> > > > > > body {
> > > > > > font-size: 75%;
> > > > > > }
> > > > > > Which resets the base from 16px to 12px.
> > > > > >
> > > > > > When I use the !important keyword, it removes that so 125% is
computed
> > > from
> > > > > the
> > > > > > original 16px instead of the altered 12px.
> > > > > >
> > > > > > In the latest patchset, I solved this by doing
> > > > > > body {
> > > > > > font-size: .9375% !important;
> > > > > > }
> > > > > > Which is the equivalent of .75*1.25, but this is kind of a hack.
> > > > >
> > > > > Hm... would it work to add a big <div> around everything in the <body>
> and
> > > set
> > > > > the font-size on that?
> > > > >
> > > >
> > > > Ah yes, that does work too. Moved it to a wrapper.
> > > >
> > > > > Also, interestingly enough, IDS_WEB_FONT_SIZE is 75% on all
platforms...
> > > > except
> > > > > under Windows XP in Bengali. o_O
> > > >
> > > > I wonder if they should just make it 12px instead of 16px to start with
> > > then...
> > >
> > > I think 16px is the user-agent default, which means changing it would
change
> > it
> > > for _all_ websites.
> >
> > Are you fine with this approach (125% on the pre-existing wrapper)? I'd like
> to
> > land this before branch if I can to avoid needing a merge next week.
>
> Yes, this LGTM (the 125% on the <html> approach as well, FWIW).
Can you update the description though?
Updated description and moved it to the html tag.
The CQ bit was checked by felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/842973002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/51e49b0faba37e3dc20809f10c49fd922c39d581 Cr-Commit-Position: refs/heads/master@{#310839} |
