Make font-size: larger/smaller a simple ratio

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
4 months ago
12 days ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

Created attachment 8863933 [details]
testcase.html

We currently implement font-size: larger and font-size: smaller by basically mapping back to the keyword sizes and bumping the keyword up or down by 1.

This means, for example, that font-size:larger when the parent is 16px on a serif font will map to font-size: large (18px). Something like 17px will take its position between font-size:medium and font-size:large (50%) and map it to the same position between font-size:large and font-size:x-large.

This is what the spec says too: https://drafts.csswg.org/css-fonts-3/#relative-size-value

However, Chrome/Safari just multiply by 1.2 or 0.85, as does Servo. IE seems to also multiply, but by different (close) ratios. It doesn't seem to handle the generic font change behavior.

Gecko's behavior in itself is not hard to implement in stylo, however it becomes nontrivial when we need to deal with generic font family changes, because we need to persist the keyword it mapped to. This makes it trickier to implement in Servo's model, which does not walk up the rule tree and instead keeps track of keyword sizes and ratios (this is something I want to eventually do in Gecko too). It's possible to implement, but after discussing in #layout the consensus seemed to be that we should just remove this behavior and use simple ratios like other browsers.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7529a19c7881a899639a65758fc5b85ebf9cc40 shows that Gecko tests don't fail due to this change. One stylo test is an unexpected pass. There are multiple other tests affected by this, but they also fail due to scrollbars and other things so they won't show up.
Comment hidden (mozreview-request)
So looking at this a little more -- I find it a little disturbing that the ratios aren't the inverse of each other.  I'd have hoped that larger and then smaller (or vice-versa) would get you back to where you started.

Do you happen to know if the IE ratios produce that result?  Maybe it would be better to use them...
Flags: needinfo?(manishearth)
Chromium seems to use multiplication and division by 1.2, actually:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/fonts/FontDescription.cpp?rcl=29b751a9ca7189238f4ad3703fd9db726698af91&l=154
... and that appears to have been true both before and after https://chromium.googlesource.com/chromium/src/+/17b5d686f5066fd209815697ca8c9e1419018133

Comment 6

4 months ago
mozreview-review
Comment on attachment 8863934 [details]
Bug 1361550 - Make font-size: larger/smaller a simple ratio; add stylo support;

https://reviewboard.mozilla.org/r/135674/#review139382

r=dbaron if you change both occurrences of 0.85 to 0.833333 (which seems equivalent to what Chromium does)... or convince me why 0.85 is better.
Attachment #8863934 - Flags: review?(dbaron) → review+
Flags: needinfo?(manishearth)
Created attachment 8864647 [details]
IE font sizes
Created attachment 8864648 [details]
Chrome font sizes
Created attachment 8864649 [details]
Firefox font sizes
Comment hidden (mozreview-request)
yeah, I guess I got confused between the numbers there. I put 1 / 1.2 in both places (clearer that way)


I looked at IE and I have no idea what it's doing here. Firstly, font sizes aren't affected by the generic. Ok. That saves them a bunch of trouble.

The font sizes themselves are arranged differently than the 9/10/13/16/18/24/32 that Firefox and Chrome do. Ok. I expected that. Interestingly they're not whole numbers, but whatever.

font-size: larger and font-size: smaller do take you to the next keyword size. In most cases. Except when apply smaller on x-small, in which case it becomes even smaller than xx-small. Wat. This seems to be the worst of both worlds? Perhaps they have some weird scaling function here.


Anyway, changed to 1.2^-1, so we can land this I guess.

Comment 12

4 months ago
mozreview-review
Comment on attachment 8863934 [details]
Bug 1361550 - Make font-size: larger/smaller a simple ratio; add stylo support;

https://reviewboard.mozilla.org/r/135674/#review139384

::: servo/components/style/properties/longhand/font.mako.rs:774
(Diff revision 2)
>                      }
>                  }
> +            } else if let SpecifiedValue::Larger = *self {
> +                return Some(1.2)
> +            } else if let SpecifiedValue::Smaller = *self {
> +                return Some(1./1.2)

(Just two drive-by comments, feel free to not address them if you don't feel the same way)

First, this could use a `match` expression, which could perhaps be a bit more idiomatic.

Second, it'd be nice if the `1.2` would be moved to a constant with a descriptive name, and perhaps some rationale of why it was chosen.

Comment 13

4 months ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/60c719808cf4
Make font-size: larger/smaller a simple ratio; add stylo support; r=dbaron
Addressed in the servo side, at https://github.com/servo/servo/pull/16730

(Landing separately since they're pretty independent)

Comment 15

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/60c719808cf4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attachment #8863933 - Attachment mime type: text/plain → text/html
You need to log in before you can comment on or make changes to this bug.