Hard code autofill preview font for select and date/time inputs [chromium/src : main]
David Baron (Gerrit)
David Baron added 4 comments![]( "Open in Gerrit")
input:-internal-autofill-previewed::-webkit-datetime-edit-text-fields-wrapper {```suggestion
input:-internal-autofill-previewed::-webkit-datetime-edit-fields-wrapper {
```
(and same below)
font: monospace !important;
font-family: monospace !important;`font: monospace` isn't valid syntax because the `font` shorthand requires both size and family. I suspect you want a fixed font size here although I'm not sure. So you probably want either `medium monospace` (fixed) or `1em monospace` (inherits the size). Or maybe actually you want what the old code was doing which was `font: -webkit-small-control` and then `font-family: monospace` which leaves all the aspects of `-webkit-small-control` except for the family, and then changes the family. (This might also suggest going back to 2 rules rather than 3, though maybe with a better comment.)
Although I'm also a little surprised by wanting `monospace` rather than a system font though I guess it's the existing style for textarea.
/* Prevent that overflow affects the scrollable area. Without this,```suggestion
/* Prevent overflow from affecting the scrollable area. Without this,
```
}
input::-internal-input-suggested,
textarea::-internal-input-suggested {No need to split these into separate rules, I think...
```suggestion
```
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 4 comments![]( "Open in Gerrit")
input:-internal-autofill-previewed::-webkit-datetime-edit-text-fields-wrapper {```suggestion
input:-internal-autofill-previewed::-webkit-datetime-edit-fields-wrapper {
```
(and same below)
Done
font: monospace !important;
font-family: monospace !important;`font: monospace` isn't valid syntax because the `font` shorthand requires both size and family. I suspect you want a fixed font size here although I'm not sure. So you probably want either `medium monospace` (fixed) or `1em monospace` (inherits the size). Or maybe actually you want what the old code was doing which was `font: -webkit-small-control` and then `font-family: monospace` which leaves all the aspects of `-webkit-small-control` except for the family, and then changes the family. (This might also suggest going back to 2 rules rather than 3, though maybe with a better comment.)
Although I'm also a little surprised by wanting `monospace` rather than a system font though I guess it's the existing style for textarea.
Whoops, yeah this shouldn't be set twice in a row like this, that was clearly a mistake.
Considering the vulnerability has to do with loading a custom font that the author specifies, I'm not sure that setting a specific font size helps at all. Do you think we should be?
And yes I chose monospace because thats the default for these elements. I'm leaning just doing font-family:monospace!important here and nothing else, but do you feel strongly that we should also be doing font:-webkit-small-control!important?
/* Prevent that overflow affects the scrollable area. Without this,```suggestion
/* Prevent overflow from affecting the scrollable area. Without this,
```
Done
}
input::-internal-input-suggested,
textarea::-internal-input-suggested {No need to split these into separate rules, I think...
```suggestion
```
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
font: monospace !important;
font-family: monospace !important;Joey Arhar`font: monospace` isn't valid syntax because the `font` shorthand requires both size and family. I suspect you want a fixed font size here although I'm not sure. So you probably want either `medium monospace` (fixed) or `1em monospace` (inherits the size). Or maybe actually you want what the old code was doing which was `font: -webkit-small-control` and then `font-family: monospace` which leaves all the aspects of `-webkit-small-control` except for the family, and then changes the family. (This might also suggest going back to 2 rules rather than 3, though maybe with a better comment.)
Although I'm also a little surprised by wanting `monospace` rather than a system font though I guess it's the existing style for textarea.
Whoops, yeah this shouldn't be set twice in a row like this, that was clearly a mistake.
Considering the vulnerability has to do with loading a custom font that the author specifies, I'm not sure that setting a specific font size helps at all. Do you think we should be?
And yes I chose monospace because thats the default for these elements. I'm leaning just doing font-family:monospace!important here and nothing else, but do you feel strongly that we should also be doing font:-webkit-small-control!important?
So I think the old code did explicitly set the size for both input and textarea. I'd be inclined to keep that aspect of it since I don't think you need to *remove* it to fix the security bug and there may well be a reason for it.
So maybe it makes sense to:
* continue to have 3 rules
* replace the `select:-internal-autofill-previewed` selector in the first rule with `textarea::-internal-input-suggested`
* add a `select:-internal-autofill-previewed` selector to the second rule
* the above pair of changes mean that all three rules will now apply to `textarea::-internal-input-suggested`
* just do `font-family: monospace !important` in the second rule
This would mean the code continues to set the size for the input and textarea previews (as it has done), continues to set the same font family that it was for both of them (which has been different for input versus textarea), and then starts setting font family (but not size) for all the other pieces you're adding (select and the input date parts).
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
font: monospace !important;
font-family: monospace !important;Joey Arhar`font: monospace` isn't valid syntax because the `font` shorthand requires both size and family. I suspect you want a fixed font size here although I'm not sure. So you probably want either `medium monospace` (fixed) or `1em monospace` (inherits the size). Or maybe actually you want what the old code was doing which was `font: -webkit-small-control` and then `font-family: monospace` which leaves all the aspects of `-webkit-small-control` except for the family, and then changes the family. (This might also suggest going back to 2 rules rather than 3, though maybe with a better comment.)
Although I'm also a little surprised by wanting `monospace` rather than a system font though I guess it's the existing style for textarea.
David BaronWhoops, yeah this shouldn't be set twice in a row like this, that was clearly a mistake.
Considering the vulnerability has to do with loading a custom font that the author specifies, I'm not sure that setting a specific font size helps at all. Do you think we should be?
And yes I chose monospace because thats the default for these elements. I'm leaning just doing font-family:monospace!important here and nothing else, but do you feel strongly that we should also be doing font:-webkit-small-control!important?
So I think the old code did explicitly set the size for both input and textarea. I'd be inclined to keep that aspect of it since I don't think you need to *remove* it to fix the security bug and there may well be a reason for it.
So maybe it makes sense to:
* continue to have 3 rules
* replace the `select:-internal-autofill-previewed` selector in the first rule with `textarea::-internal-input-suggested`
* add a `select:-internal-autofill-previewed` selector to the second rule
* the above pair of changes mean that all three rules will now apply to `textarea::-internal-input-suggested`
* just do `font-family: monospace !important` in the second ruleThis would mean the code continues to set the size for the input and textarea previews (as it has done), continues to set the same font family that it was for both of them (which has been different for input versus textarea), and then starts setting font family (but not size) for all the other pieces you're adding (select and the input date parts).
- David Baron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted Code-Review+1![]( "Open in Gerrit")
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
1 comment
font: monospace !important;
font-family: monospace !important;Joey Arhar`font: monospace` isn't valid syntax because the `font` shorthand requires both size and family. I suspect you want a fixed font size here although I'm not sure. So you probably want either `medium monospace` (fixed) or `1em monospace` (inherits the size). Or maybe actually you want what the old code was doing which was `font: -webkit-small-control` and then `font-family: monospace` which leaves all the aspects of `-webkit-small-control` except for the family, and then changes the family. (This might also suggest going back to 2 rules rather than 3, though maybe with a better comment.)
Although I'm also a little surprised by wanting `monospace` rather than a system font though I guess it's the existing style for textarea.
David BaronWhoops, yeah this shouldn't be set twice in a row like this, that was clearly a mistake.
Considering the vulnerability has to do with loading a custom font that the author specifies, I'm not sure that setting a specific font size helps at all. Do you think we should be?
And yes I chose monospace because thats the default for these elements. I'm leaning just doing font-family:monospace!important here and nothing else, but do you feel strongly that we should also be doing font:-webkit-small-control!important?
Joey ArharSo I think the old code did explicitly set the size for both input and textarea. I'd be inclined to keep that aspect of it since I don't think you need to *remove* it to fix the security bug and there may well be a reason for it.
So maybe it makes sense to:
* continue to have 3 rules
* replace the `select:-internal-autofill-previewed` selector in the first rule with `textarea::-internal-input-suggested`
* add a `select:-internal-autofill-previewed` selector to the second rule
* the above pair of changes mean that all three rules will now apply to `textarea::-internal-input-suggested`
* just do `font-family: monospace !important` in the second ruleThis would mean the code continues to set the size for the input and textarea previews (as it has done), continues to set the same font family that it was for both of them (which has been different for input versus textarea), and then starts setting font family (but not size) for all the other pieces you're adding (select and the input date parts).
Done
Marked as unresolved.It looks like applying font-family:monospace!important to select:-internal-autofill-previewed regresses menulist-field-sizing-autofill.html, which makes sense since select elements don't have monospace by default.
If you still think that we should be hard coding a font-family for select elements, then maybe I should make it be the same as whatever -webkit-small-control is, right?
Based on what I read in LayoutThemeFontProvider::DefaultGUIFont it looks like -webkit-small-control always has Arial, so i set it to that. Based on https://chromium-review.git.corp.google.com/c/chromium/src/+/7930489 I'm not sure that its always Arial though... I guess I might find out after CQ+1
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 2 comments![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
2 comments
font-family: Arial !important;Could you add a comment saying this matches `LayoutThemeFontProvider::DefaultGUIFont()` which is the `font-family` that `font: -webkit-small-control` produces?
font: monospace !important;
font-family: monospace !important;Joey Arhar`font: monospace` isn't valid syntax because the `font` shorthand requires both size and family. I suspect you want a fixed font size here although I'm not sure. So you probably want either `medium monospace` (fixed) or `1em monospace` (inherits the size). Or maybe actually you want what the old code was doing which was `font: -webkit-small-control` and then `font-family: monospace` which leaves all the aspects of `-webkit-small-control` except for the family, and then changes the family. (This might also suggest going back to 2 rules rather than 3, though maybe with a better comment.)
Although I'm also a little surprised by wanting `monospace` rather than a system font though I guess it's the existing style for textarea.
David BaronWhoops, yeah this shouldn't be set twice in a row like this, that was clearly a mistake.
Considering the vulnerability has to do with loading a custom font that the author specifies, I'm not sure that setting a specific font size helps at all. Do you think we should be?
And yes I chose monospace because thats the default for these elements. I'm leaning just doing font-family:monospace!important here and nothing else, but do you feel strongly that we should also be doing font:-webkit-small-control!important?
Joey ArharSo I think the old code did explicitly set the size for both input and textarea. I'd be inclined to keep that aspect of it since I don't think you need to *remove* it to fix the security bug and there may well be a reason for it.
So maybe it makes sense to:
* continue to have 3 rules
* replace the `select:-internal-autofill-previewed` selector in the first rule with `textarea::-internal-input-suggested`
* add a `select:-internal-autofill-previewed` selector to the second rule
* the above pair of changes mean that all three rules will now apply to `textarea::-internal-input-suggested`
* just do `font-family: monospace !important` in the second ruleThis would mean the code continues to set the size for the input and textarea previews (as it has done), continues to set the same font family that it was for both of them (which has been different for input versus textarea), and then starts setting font family (but not size) for all the other pieces you're adding (select and the input date parts).
Joey ArharDone
Marked as unresolved.It looks like applying font-family:monospace!important to select:-internal-autofill-previewed regresses menulist-field-sizing-autofill.html, which makes sense since select elements don't have monospace by default.
If you still think that we should be hard coding a font-family for select elements, then maybe I should make it be the same as whatever -webkit-small-control is, right?
Based on what I read in LayoutThemeFontProvider::DefaultGUIFont it looks like -webkit-small-control always has Arial, so i set it to that. Based on https://chromium-review.git.corp.google.com/c/chromium/src/+/7930489 I'm not sure that its always Arial though... I guess I might find out after CQ+1
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
1 comment
font-family: Arial !important;Could you add a comment saying this matches `LayoutThemeFontProvider::DefaultGUIFont()` which is the `font-family` that `font: -webkit-small-control` produces?
Ok so apparently we can't use Arial for this because it can leak the text via @font-face: https://chromium-review.git.corp.google.com/c/chromium/src/+/7930489/comment/85a58c0f_100803c8/
I'm changing this to sans-serif in order to match that other change. What do you think of this?
I'm also going to have to rebase this change on theirs soon, which will probably require rewriting some of this stuff.
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
1 comment
font-family: Arial !important;Joey ArharCould you add a comment saying this matches `LayoutThemeFontProvider::DefaultGUIFont()` which is the `font-family` that `font: -webkit-small-control` produces?
Ok so apparently we can't use Arial for this because it can leak the text via @font-face: https://chromium-review.git.corp.google.com/c/chromium/src/+/7930489/comment/85a58c0f_100803c8/
I'm changing this to sans-serif in order to match that other change. What do you think of this?
I'm also going to have to rebase this change on theirs soon, which will probably require rewriting some of this stuff.
Hmmm. I wonder if at this point it's going to be more straightforward to expose an internal CSS syntax for setting a `CSSPendingSystemFontValue` as the value of any of the longhands of the `font` shorthand. (Probably functional, taking as an argument which system font.)
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
1 comment
font-family: Arial !important;Joey ArharCould you add a comment saying this matches `LayoutThemeFontProvider::DefaultGUIFont()` which is the `font-family` that `font: -webkit-small-control` produces?
David BaronOk so apparently we can't use Arial for this because it can leak the text via @font-face: https://chromium-review.git.corp.google.com/c/chromium/src/+/7930489/comment/85a58c0f_100803c8/
I'm changing this to sans-serif in order to match that other change. What do you think of this?
I'm also going to have to rebase this change on theirs soon, which will probably require rewriting some of this stuff.
Hmmm. I wonder if at this point it's going to be more straightforward to expose an internal CSS syntax for setting a `CSSPendingSystemFontValue` as the value of any of the longhands of the `font` shorthand. (Probably functional, taking as an argument which system font.)
So it would look like this?
```
font: -internal-pending-system-font(-webkit-small-control) !important;
```
Wouldn't we still need to set the font-family explicitly too?
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
font-family: Arial !important;Joey ArharCould you add a comment saying this matches `LayoutThemeFontProvider::DefaultGUIFont()` which is the `font-family` that `font: -webkit-small-control` produces?
David BaronOk so apparently we can't use Arial for this because it can leak the text via @font-face: https://chromium-review.git.corp.google.com/c/chromium/src/+/7930489/comment/85a58c0f_100803c8/
I'm changing this to sans-serif in order to match that other change. What do you think of this?
I'm also going to have to rebase this change on theirs soon, which will probably require rewriting some of this stuff.
Joey ArharHmmm. I wonder if at this point it's going to be more straightforward to expose an internal CSS syntax for setting a `CSSPendingSystemFontValue` as the value of any of the longhands of the `font` shorthand. (Probably functional, taking as an argument which system font.)
So it would look like this?
```
font: -internal-pending-system-font(-webkit-small-control) !important;
```
Wouldn't we still need to set the font-family explicitly too?
I was thinking of the ability to do `font-family: -internal-system-font(-webkit-small-control) ! important` so that it's just setting the family but also (I assume, based on the discussion above) bypassing `@font-face`.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |