Closed Bug 1380980 Opened 7 years ago Closed 7 years ago

stylo: font_size::SpecifiedValue::as_font_ratio looks super-fishy.

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: emilio, Assigned: manishearth)

References

Details

Attachments

(4 files)

Bug 1380980 - stylo: Add (unused) absolute offset to FontComputationData;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1380980 - stylo: Add support for calcs in base size calculation;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1380980 - stylo: Add test;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1380980 - stylo: Replace FontComputationData with typedef;
59 bytes, text/x-review-board-request
emilio
: review+
Details 
That method reads like:

pub fn as_font_ratio(&self) -> Option<f32> {
    if let SpecifiedValue::Length(ref lop) = *self {
        if let LengthOrPercentage::Percentage(pc) = *lop {
            return Some(pc.0)
        } else if let LengthOrPercentage::Length(ref nocalc) = *lop {
            if let NoCalcLength::FontRelative(FontRelativeLength::Em(em)) = *nocalc {
                return Some(em)
            }
        }
    } else if let SpecifiedValue::Larger = *self {
        return Some(LARGER_FONT_SIZE_RATIO)
    } else if let SpecifiedValue::Smaller = *self {
        return Some(1. / LARGER_FONT_SIZE_RATIO)
    }
    None
}

It only checks LengthOrPercentage::Length and LengthOrPercentage::Percentage, but as far as I can tell, there's nothing preventing us from arriving there with a LengthOrPercentage::Calc, in which case the font ratio won't be detected or honored.

I can see why handling calc is annoying (is it even possible to do so with our current setup?).
Manish, anything I've missed?
Flags: needinfo?(manishearth)
No, this looks like an oversight.

It's not possible to do with the current setup, however, you will have to track an additive bit and a factor in the current setup.
Flags: needinfo?(manishearth)
Priority: -- → P3
taking this.
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Comment on attachment 8906249 [details]
Bug 1380980 - stylo: Add (unused) absolute offset to FontComputationData;

https://reviewboard.mozilla.org/r/178002/#review183136

r=me, assuming it's useful for following commits.

::: servo/components/style/properties/properties.mako.rs:2550
(Diff revision 1)
>      /// The writing mode flags.
>      ///
>      /// TODO(emilio): Make private.
>      pub writing_mode: WritingMode,
>      /// The keyword behind the current font-size property, if any.
> -    pub font_size_keyword: Option<(longhands::font_size::KeywordSize, f32)>,
> +    pub font_size_keyword: Option<(longhands::font_size::KeywordSize, f32, NonNegativeAu)>,

nit: Can we make this a struct, please?
Attachment #8906249 - Flags: review?(emilio) → review+
Comment on attachment 8906250 [details]
Bug 1380980 - stylo: Add support for calcs in base size calculation;

https://reviewboard.mozilla.org/r/178004/#review183138

::: servo/components/style/properties/longhand/font.mako.rs:625
(Diff revision 1)
> -        /// A keyword value, along with a ratio.
> +        /// A keyword value, along with a ratio and absolute offset.
>          /// The ratio in any specified keyword value
> -        /// will be 1, but we cascade keywordness even
> +        /// will be 1 (with offset 0), but we cascade keywordness even
>          /// after font-relative (percent and em) values
>          /// have been applied, which is where the keyword
>          /// comes in. See bug 1355707

Please describe when the offset thingie may be non-zero.

::: servo/components/style/properties/longhand/font.mako.rs:829
(Diff revision 1)
>                          }
> -                        // FIXME(emilio): This looks super fishy!
> -                        LengthOrPercentage::Calc(..) => None,
> +                        LengthOrPercentage::Calc(ref calc) => {
> +                            if calc.em.is_none() && calc.percentage.is_none() {
> +                                return None;
> -                    }
> +                            }
> +                            let ratio = calc.em.unwrap_or(0.) + calc.percentage.map(|pc| pc.0).unwrap_or(0.);

nit: map_or instead of map(..).unwrap_or?

::: servo/components/style/properties/longhand/font.mako.rs:831
(Diff revision 1)
> -                        LengthOrPercentage::Calc(..) => None,
> +                            if calc.em.is_none() && calc.percentage.is_none() {
> +                                return None;
> -                    }
> +                            }
> +                            let ratio = calc.em.unwrap_or(0.) + calc.percentage.map(|pc| pc.0).unwrap_or(0.);
> +                            // Compute it, but shave off the font-relative part
> +                            let abs = calc.to_computed_value_zoomed(context, FontBaseSize::Custom(Au(0).into()))

I'm not convinced this is correct for font-dependent units.

In particular, if we have something like `font-size: calc(10ex + 1em)`, and a style inheriting from it would have another `font-family` wouldn't the `10ex` offset computed with the _old_ font family end up  somewhere in the calculation of the new size? Is that expected? If so, why is that correct?

The commit message could have some love too :)
Attachment #8906250 - Flags: review?(emilio)
> wouldn't the `10ex` offset computed with the _old_ font family end up  somewhere in the calculation of the new size

Yes.

> Is that expected? 

No.


>  If so, why is that correct?

Because it's a super edge case and there's no right answer here. We can match what Gecko does (which is inefficient), or do this. My eventual plan is to change Gecko's code to follow this model too.
Comment on attachment 8906251 [details]
Bug 1380980 - stylo: Add test;

https://reviewboard.mozilla.org/r/178006/#review183140

r=me. It'd be nice to have tests for this with other kinds of units / etc.
Attachment #8906251 - Flags: review?(emilio) → review+
Comment on attachment 8906250 [details]
Bug 1380980 - stylo: Add support for calcs in base size calculation;

https://reviewboard.mozilla.org/r/178004/#review183568

r=me
Attachment #8906250 - Flags: review?(emilio) → review+
Comment on attachment 8906785 [details]
Bug 1380980 - stylo: Replace FontComputationData with typedef;

https://reviewboard.mozilla.org/r/178516/#review183570

::: servo/components/style/properties/properties.mako.rs:113
(Diff revision 1)
> -    /// and is 1 when there was just a keyword and no relative values.
> +/// and is 1 when there was just a keyword and no relative values.
> -    ///
> +///
> -    /// When this is Some, we compute font sizes by computing the keyword against
> +/// When this is Some, we compute font sizes by computing the keyword against
> -    /// the generic font, and then multiplying it by the ratio (as well as adding any
> +/// the generic font, and then multiplying it by the ratio (as well as adding any
> -    /// absolute offset from calcs)
> +/// absolute offset from calcs)
> -    pub font_size_keyword: Option<(longhands::font_size::KeywordSize, f32, NonNegativeAu)>
> +pub type FontComputationData = Option<(longhands::font_size::KeywordSize, f32, NonNegativeAu)>;

Hmm, I meant more like:

```
struct FontComputationData {
    keyword: KeywordSize,
    factor: f32,
    offset: NonNegativeAu,
}
```

Then using `Option<FontComputationData>`.

But I guess this works too?
Attachment #8906785 - Flags: review?(emilio) → review+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bf6b321ede7b
stylo: Add (unused) absolute offset to FontComputationData; r=emilio
https://hg.mozilla.org/integration/autoland/rev/2dc8bf7c38ae
stylo: Add test; r=emilio
This added a word to ComputedValues (264 -> 272). Did we really need to do that, or is there some clever way we could pack this better? Adding a word here costs us half a megabyte on the html spec.
Flags: needinfo?(manishearth)
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/327f2a97631d
followup, mark test as failing on QuantumRender
Keywords: leave-open
Backed out my annotation and the whole test in https://hg.mozilla.org/integration/autoland/rev/24c584c7450a since apparently it fails either everywhere, or a whole lot of wheres, and we just hardly ever run reftests because we're too cheap to run them more than once every dozen or two pushes.
I suspect it fails because the font prefs may be different across platforms. I'll investigate.
Flags: needinfo?(manishearth)
> This added a word to ComputedValues (264 -> 272). Did we really need to do that, or is there some clever way we could pack this better? Adding a word here costs us half a megabyte on the html spec.

Bear in mind this is handling somewhat of an edge case so we could even *not* include the fix. I'm not sure what others feel about this. I'm ambivalent.

Some solutions are:

 - Making it a u16 and somehow getting rustc to pack it with the KeywordSize. This reduces the total range it can go, but this is an edge case.

 - Moving the whole thing to the nsStyleFont. This is where it *should* be, and where I intend to move it once the Gecko style system is removed. However this will bloat nsStyleFont (but that gets shared more, presumably). If it's on nsStyleFont there are other packing opportunities, e.g. we can pack it with the script size multiplier or mathvariant fields.

 - Switching over to Gecko's solution; i.e. walking up the rule tree and recalculating styles as if the base size were different.  I do not recommend this; it's super inefficient and requires us to overcomplicate the code.

 - We only have two unused bits on the Au (max value shaves off one bit, and it's non negative), so not enough to store Option<KeywordSize>. We can reduce the range of the offset to (1<<27 - 1) or something and then bitpack it.


Thoughts?
Per discussion in the meeting, moving to nsStyleFont is preferable. Even better if we can get it to use one word there instead of two.

File a bug? I think this would be nice to do in the 57 timeframe, since it would be a 6% reduction in the size of ComputedValues, which in turn is a large part of our memory overhead.
Blocks: 1399228
I think I can get it done by 57. Probably is straightforward, just potentially tedious.

Filed bug 1399228.
The test failures are because for some reason the base font-size for monospace is 12px instead of 13.

We need to find something that doesn't vary across platforms in the default configuration.
This actually already landed; the test didn't. I'm unsure how to test this reliably since the prefs differ. Perhaps we can only run the test on one platform?
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: