Make font-size: larger/smaller a simple ratio

RESOLVED FIXED in Firefox 55

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments)

Posted file 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.
(Assignee)

Comment 1

2 years ago
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.
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)
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+
(Assignee)

Comment 7

2 years ago
Posted image IE font sizes
(Assignee)

Comment 8

2 years ago
Posted image Chrome font sizes
(Assignee)

Comment 9

2 years ago
Posted image Firefox font sizes
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
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

2 years 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

2 years 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
(Assignee)

Comment 14

2 years ago
Addressed in the servo side, at https://github.com/servo/servo/pull/16730

(Landing separately since they're pretty independent)

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/60c719808cf4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

2 years ago
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.