The default bug view has changed. See this FAQ.

stylo: need to do the right thing with "font-family: monospace" in terms of font size

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
28 days ago
26 minutes ago

People

(Reporter: bz, Assigned: manishearth)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(firefox54 affected)

Details

MozReview Requests

()

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

Attachments

(6 attachments)

The default font-size is different for monospace vs non-monospace fonts (13px vs 16px).  Looks like stylo always makes it 16px.  This breaks various reftests, and user expectations.
This should be super easy to fix and will unblock an unknown quantity of tests.
Assignee: nobody → manishearth
Priority: -- → P1
(Assignee)

Comment 2

5 days ago
Prerequisite refactoring for this in https://github.com/servo/servo/pull/16016
(Assignee)

Comment 3

3 days ago
This introduces a more complicated dependence of properties than our "early properties" framework was previously equipped to handle. Previously, we had "early" properties and "late" properties, where the early ones are ones like writing-mode, font-size, font-family, etc which other properties need during computation. Now, font-family is a dependency of font-size, which itself is a dependency of all the late properties. To avoid an extra iteration I'm going to pull out the font-size and family properties during the early computation and compute them separately, in order, before the late computation. I'm pulling out the family property as well since the internal system font property (which I may work on next) is going to have to be early-computed, before family.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

3 days ago
Part 1 is the same as https://github.com/servo/servo/pull/16016, doesn't need to be reviewed here -- review it there first and I'll rebase later.

In part 3 we call nsPresContext::GetDefaultFont, which might not be thread safe -- we need to figure that out.
Comment hidden (mozreview-request)
(Assignee)

Comment 9

3 days ago
This doesn't handle the fact that the initial /computed value/ is "medium".

We could make font-size compute to "Option<Au>" with None as the default and then do the computation of the None case in gecko.mako.rs. But then em units get tricky, and this sounds like it would be expensive, too.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 days ago
Agh, my solution doesn't take into account the fact that the keywordness still needs to be inherited. Fixing.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

2 days ago
mozreview-review
Comment on attachment 8848988 [details]
Bug 1341775 - Part 1: stylo: Make font-size CSS keywords real keyword specified values;

https://reviewboard.mozilla.org/r/121842/#review124254

Seems to be the landed part in Servo. Cancel review here.
Attachment #8848988 - Flags: review?(xidorn+moz)
Attachment #8848989 - Flags: review?(xidorn+moz) → review?(cam)
Attachment #8848990 - Flags: review?(xidorn+moz) → review?(cam)
Attachment #8849002 - Flags: review?(xidorn+moz) → review?(cam)
This seems to be more complicated than what I can review confidently...

Comment 18

2 days ago
mozreview-review
Comment on attachment 8848989 [details]
Bug 1341775 - Part 2: stylo: Compute font-size between font-family and all late properties;

https://reviewboard.mozilla.org/r/121844/#review124286
Attachment #8848989 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8849002 - Flags: review?(xidorn+moz) → review?(cam)
You need to log in before you can comment on or make changes to this bug.