Closed Bug 1358688 Opened 7 years ago Closed 7 years ago

stylo: handle text-zoom

Categories

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

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

Tracking

()

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

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(5 files)

Bug 1358688 - Part 1: Don't unzoom text for font-size:larger/smaller ;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1358688 - Part 2: stylo: Handle text-zoom for font-size ;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1358688 - Part 3: stylo: Handle text-zoom for scriptminsize;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1358688 - Part 4: stylo: Handle text-zoom for line-height;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1358688 - Part 5: stylo: Disable text-zoom for <svg:text>;
59 bytes, text/x-review-board-request
emilio
: review+
Details 
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.
Blocks: 1357296
This is probably more than a P3, and should definitely block releasing Stylo, I think...
Blocks: stylo-release
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.
Assignee: nobody → manishearth
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
Attachment #8891926 - Flags: review?(emilio+bugs) → review+
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.
Attachment #8891927 - Flags: review?(emilio+bugs) → review+
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.
Attachment #8891928 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8891929 [details]
Bug 1358688 - Part 4: stylo: Handle text-zoom for line-height;

https://reviewboard.mozilla.org/r/162956/#review168248
Attachment #8891929 - Flags: review?(emilio+bugs) → review+
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`.
Attachment #8891930 - Flags: review?(emilio+bugs) → review+
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 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.
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)
Flags: needinfo?(emilio+bugs)
Priority: P3 → --
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/47709bd65292
Part 1: Don't unzoom text for font-size:larger/smaller ; r=emilio
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/47709bd65292
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1404057
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: