1358688 - stylo: handle text-zoom

Status: RESOLVED FIXED

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

Description

[Manish Goregaokar [:manishearth]]

Text-zoom is where text is zoomed, preserving other sizes, aside from the sizes that are in em units (and other font relative things).

We handle this with an -x-text-zoom property that can be true or false, which gets set to false for SVG text nodes.

When inheriting a font-size, all three of mFont.mSize, mSize, and mScriptUnconstrainedSize are zoomed or unzoomed if the text-zoom value changed. Explicit specification of pixel/etc values also respects the text zoom

See sizeIsZoomedAccordingToParent and SetFontSizeCalcOps. This does some custom handling of calcs since they could contain mixed em and px units.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

This is probably more than a P3, and should definitely block releasing Stylo, I think...

Comment 3

[Manish Goregaokar [:manishearth]]

This is a bit tricky to do nicely in Stylo, but my only solace is that the code is worse in Gecko :p

Taking for now, have a plan.

Comment 4

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1358688 - Part 1: Don't unzoom text for font-size:larger/smaller ;

This used to be necessary when larger/smaller did a table lookup.
However as of bug 1361550 these are simple ratios and should be treated
the same as em units.

Review commit: https://reviewboard.mozilla.org/r/162950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162950/

Comment 5

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1358688 - Part 2: stylo: Handle text-zoom for font-size ;

Review commit: https://reviewboard.mozilla.org/r/162952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162952/

Comment 6

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1358688 - Part 3: stylo: Handle text-zoom for scriptminsize;

Review commit: https://reviewboard.mozilla.org/r/162954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162954/

Comment 7

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1358688 - Part 4: stylo: Handle text-zoom for line-height;

Review commit: https://reviewboard.mozilla.org/r/162956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162956/

Comment 8

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1358688 - Part 5: stylo: Disable text-zoom for <svg:text>;

Review commit: https://reviewboard.mozilla.org/r/162958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162958/

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8891926 [details]
Bug 1358688 - Part 1: Don't unzoom text for font-size:larger/smaller ;

https://reviewboard.mozilla.org/r/162950/#review168236

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8891927 [details]
Bug 1358688 - Part 2: stylo: Handle text-zoom for font-size ;

https://reviewboard.mozilla.org/r/162952/#review168240

r=me, with the duplication removed, and ideally a few more docs :).

::: servo/components/style/gecko/media_queries.rs:189
(Diff revision 1)
>      /// Returns the default background color.
>      pub fn default_background_color(&self) -> RGBA {
>          convert_nscolor_to_rgba(self.pres_context().mBackgroundColor)
>      }
> +
> +    /// Applies text zoom to a font-size or line-height value (see nsStyleFont::ZoomText)

nit: missing a period at the end, also in a few other places.

::: servo/components/style/values/computed/length.rs:233
(Diff revision 1)
>      }
>  }
>  
> +impl specified::CalcLengthOrPercentage {
> +    /// Compute font-size or line-height taking into account text-zoom if necessary
> +    pub fn to_computed_value_zoomed(&self, context: &Context) -> CalcLengthOrPercentage {

Can we instead share most of the code, instead of duplicating this?

IIUC we only need to scale absolute units, right? If so, what about just making this function take a closure or some generic parameter? Rust is good at this kind of stuff :-)

::: servo/components/style/values/computed/mod.rs:138
(Diff revision 1)
>      pub fn style(&self) -> &StyleBuilder {
>          &self.builder
>      }
> +
> +
> +    /// Apply text-zoom if enabled

A bit more detail in the `mAllowZoom` check would be nice.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8891928 [details]
Bug 1358688 - Part 3: stylo: Handle text-zoom for scriptminsize;

https://reviewboard.mozilla.org/r/162954/#review168244

::: servo/components/style/properties/gecko.mako.rs:1977
(Diff revision 1)
>      /// Returns true if the inherited keyword size was actually used
>      pub fn inherit_font_size_from(&mut self, parent: &Self,
>                                    kw_inherited_size: Option<Au>,
>                                    device: &Device) -> bool {
>          let (adjusted_size, adjusted_unconstrained_size)
> -            = self.calculate_script_level_size(parent);
> +            = self.calculate_script_level_size(parent, device);

nit: while you're here, I think the `=` should be in the previous line, here and above. Tidy checks for this, but not in the mako files.

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8891929 [details]
Bug 1358688 - Part 4: stylo: Handle text-zoom for line-height;

https://reviewboard.mozilla.org/r/162956/#review168248

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8891930 [details]
Bug 1358688 - Part 5: stylo: Disable text-zoom for <svg:text>;

https://reviewboard.mozilla.org/r/162958/#review168250

Looks fine.

Now the question... Are there tests for this stuff? :-)

::: servo/components/style/gecko/wrapper.rs:1457
(Diff revision 1)
>                  hints.push(TH_RULE.clone());
>              } else if self.get_local_name().as_ptr() == atom!("table").as_ptr() &&
>                        self.as_node().owner_doc().mCompatMode == structs::nsCompatibility::eCompatibility_NavQuirks {
>                  hints.push(TABLE_COLOR_RULE.clone());
>              }
>          }

nit: You can use `else if` here, since you've already checked the namespace above. Not a big deal I guess though.

::: servo/components/style/properties/gecko.mako.rs:1840
(Diff revision 1)
> +        self.gecko.mSize = device.unzoom_text(Au(self.gecko.mSize)).0;
> +        self.gecko.mScriptUnconstrainedSize = device.unzoom_text(Au(self.gecko.mScriptUnconstrainedSize)).0;
> +        self.gecko.mFont.size = device.unzoom_text(Au(self.gecko.mFont.size)).0;
> +    }
> +
> +

nit: Extra whitespace.

::: servo/components/style/properties/longhand/font.mako.rs:2409
(Diff revision 1)
> +        }
> +
> +        #[derive(Clone, Debug, PartialEq)]
> +        #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
> +        /// text-zoom. Enable if true, disable if false
> +        pub struct T(pub bool);

I'd just use a binary enum, but sure.

::: servo/components/style/properties/properties.mako.rs:3125
(Diff revision 1)
> +                // <svg:text> is not affected by text zoom, and it uses a preshint to
> +                // disable it. We fix up the struct when this happens by unzooming
> +                // its contained font values, which will have been zoomed in the parent
> +                if seen.contains(LonghandId::XTextZoom) {
> +                    let zoom = context.builder.get_font().gecko().mAllowZoom;
> +                    let parent_zoom = inherited_style.get_font().gecko().mAllowZoom;

use `context.get_parent_font()` instead.

::: servo/components/style/properties/properties.mako.rs:3127
(Diff revision 1)
> +                // its contained font values, which will have been zoomed in the parent
> +                if seen.contains(LonghandId::XTextZoom) {
> +                    let zoom = context.builder.get_font().gecko().mAllowZoom;
> +                    let parent_zoom = inherited_style.get_font().gecko().mAllowZoom;
> +                    if  zoom != parent_zoom {
> +                        debug_assert!(zoom == false,

nit: `!zoom`.

Comment 14

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891927 [details]
Bug 1358688 - Part 2: stylo: Handle text-zoom for font-size ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162952/diff/1-2/

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891928 [details]
Bug 1358688 - Part 3: stylo: Handle text-zoom for scriptminsize;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162954/diff/1-2/

Comment 16

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891929 [details]
Bug 1358688 - Part 4: stylo: Handle text-zoom for line-height;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162956/diff/1-2/

Comment 17

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891930 [details]
Bug 1358688 - Part 5: stylo: Disable text-zoom for <svg:text>;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162958/diff/1-2/

Comment 18

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891930 [details]
Bug 1358688 - Part 5: stylo: Disable text-zoom for <svg:text>;

https://reviewboard.mozilla.org/r/162958/#review168250

> use `context.get_parent_font()` instead.

That doesn't seem to exist?

Comment 19

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891930 [details]
Bug 1358688 - Part 5: stylo: Disable text-zoom for <svg:text>;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162958/diff/2-3/

Comment 20

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8891930 [details]
Bug 1358688 - Part 5: stylo: Disable text-zoom for <svg:text>;

https://reviewboard.mozilla.org/r/162958/#review168250

> That doesn't seem to exist?

context.style.get_parent_font, I mean, to handle the weird first-line inheritance stuff.

Comment 21

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891926 [details]
Bug 1358688 - Part 1: Don't unzoom text for font-size:larger/smaller ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162950/diff/1-2/

Comment 22

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891927 [details]
Bug 1358688 - Part 2: stylo: Handle text-zoom for font-size ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162952/diff/2-3/

Comment 23

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891928 [details]
Bug 1358688 - Part 3: stylo: Handle text-zoom for scriptminsize;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162954/diff/2-3/

Comment 24

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891929 [details]
Bug 1358688 - Part 4: stylo: Handle text-zoom for line-height;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162956/diff/2-3/

Comment 25

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891930 [details]
Bug 1358688 - Part 5: stylo: Disable text-zoom for <svg:text>;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162958/diff/3-4/

Comment 26

[Manish Goregaokar [:manishearth]]

So this works, except for the font-size on the root element and anything which inherits font-size from it. So anything that is not affected by font-size rules will not be zoomed.

I did some poking at the pointers and it turns out that:

 - When a zoomed document is first loaded, default_computed_values is wrong (the font size isn't zoomed)
 - When a document is zoomed, default_computed_values is corrected, however we don't restyle the root so all other elements inherit from the _old_ font-size

If I add an `@media(max-width)` rule to the document and then resize the window, the (incorrect) font size is replaced with the correct zoomed version the moment the window crosses the max-width threshold. So this is a restyle issue.


Weirdly enough, calling MediaFeatureValuesChanged *twice* in nsPresContext::UpdateEffectiveTextZoom() https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/layout/base/nsPresContext.cpp#1378-1379 fixes this, *both* on loading a zoomed document and on progressively zooming an already loaded document.

There's probably something happening in the wrong order there.

(ni? emilio since he said he had some ideas)

Comment 27

[Emilio Cobos Álvarez [:emilio]]

https://github.com/servo/servo/pull/17933

Comment 28

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891926 [details]
Bug 1358688 - Part 1: Don't unzoom text for font-size:larger/smaller ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162950/diff/2-3/

Comment 29

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891927 [details]
Bug 1358688 - Part 2: stylo: Handle text-zoom for font-size ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162952/diff/3-4/

Comment 30

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891928 [details]
Bug 1358688 - Part 3: stylo: Handle text-zoom for scriptminsize;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162954/diff/3-4/

Comment 31

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891929 [details]
Bug 1358688 - Part 4: stylo: Handle text-zoom for line-height;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162956/diff/3-4/

Comment 32

[Manish Goregaokar [:manishearth]]

Comment on attachment 8891930 [details]
Bug 1358688 - Part 5: stylo: Disable text-zoom for <svg:text>;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162958/diff/4-5/

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47709bd65292
Part 1: Don't unzoom text for font-size:larger/smaller ; r=emilio

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/47709bd65292