Closed Bug 1361550 Opened 8 years ago Closed 8 years ago

Make font-size: larger/smaller a simple ratio

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(5 files)

Attached file testcase.html (deleted) —
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.
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+
Flags: needinfo?(manishearth)
Attached image IE font sizes (deleted) —
Attached image Chrome font sizes (deleted) —
Attached image Firefox font sizes (deleted) —
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 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.
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)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
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.

Attachment

General

Created:
Updated:
Size: