1887627 - CSS zoom property change should affect all explicitly and implicitly inherited lengths

Status: NEW

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

Description

[Emilio Cobos Álvarez [:emilio]]

See the discussion in https://github.com/w3c/csswg-drafts/issues/9397

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Main concern here is performance of having to cascade all these properties explicitly...

Comment 4

[Daniel Holbert [:dholbert]]

This bug is the reason for most of our interop-2025-webcompat test-failures here, btw:
https://wpt.fyi/results/css/css-viewport/zoom?label=master&label=experimental&aligned&view=interop&q=label%3Ainterop-2025-webcompat%20firefox%3Afail

Comment 5

[Daniel Holbert [:dholbert]]

Relevant code snippet, where we do something like this for font-size:
https://searchfox.org/mozilla-central/rev/dd8b5213e4e7760b5fe5743fbc313398b85f8a14/servo/components/style/properties/cascade.rs#772-782,1277-1287

    if apply!(Zoom) {
        context.builder.effective_zoom = context
            .builder
            .inherited_effective_zoom()
            .compute_effective(context.builder.specified_zoom());
        // NOTE(emilio): This is a bit of a hack, but matches the shipped WebKit and Blink
        // behavior for now. Ideally, in the future, we have a pass over all
        // implicitly-or-explicitly-inherited properties that can contain lengths and
        // re-compute them properly, see https://github.com/w3c/csswg-drafts/issues/9397.
        self.recompute_font_size_for_zoom_change(&mut context.builder);
    }
...
fn recompute_font_size_for_zoom_change(&self, builder: &mut StyleBuilder) {
    debug_assert!(self.seen.contains(LonghandId::Zoom));
    // NOTE(emilio): Intentionally not using the effective zoom here, since all the inherited
    // zooms are already applied.
    let old_size = builder.get_font().clone_font_size();
    let new_size = old_size.zoom(builder.resolved_specified_zoom());
    if old_size == new_size {
        return;
    }
    builder.mutate_font().set_font_size(new_size);
}

emilio: is the work here essentially to repeat a version of that pattern for every length-valued property? (Maybe/hopefully not by-hand in an ad-hoc way... Maybe there's an ~elegant way to do this by looping across all such properties somehow?)

Comment 6

[Emilio Cobos Álvarez [:emilio]]

emilio: is the work here essentially to repeat a version of that pattern for every length-valued property?

Yes and no. As you mentioned we don't want to do that manually for all properties. Also, another quirk of this is that we want to do this on explicit inherit too, which doesn't work right now:

data:text/html,Should be a square below<div style="width: 16px"><div style="width: inherit; zoom: 2; height: 16px; background: green">

So yes, the right thing to do is building a LonghandIdSet (or maybe just array) of properties that can contain lengths. We can hopefully reuse some of the SpecifiedValueInfo stuff for that which already tracks it.

When you inherit one of those, there are two ways of going around it... One would be manually zooming somehow as we're doing for font, that code could be autogenerated using mako for all relevant properties I guess, but you still need some extra plumbing.

But something that would be a bit more generic and have all the relevant code there would be to call to_resolved_value() on the computed size (which unzooms) and then re-apply it (which would zoom with the correct zoom). Or something like that.

Comment 7

[Daniel Holbert [:dholbert]]

[ni=me to either take this or find an owner for it. Tentatively planning to complete this early in H2, to mostly finish off interop-2025-webcompat.]

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Bug 1968431 does something like what's described in comment 6, without the SpecifiedValueInfo stuff.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

Bug 1968431 does something like what's described in comment 6, without the SpecifiedValueInfo stuff.

Looks like jkew built on top of this in bug 1911195 comment 8, which addressed all (I think?) of the test failures that were due to this (all the interop-relevant ones at least).

emilio, do you know if there's more to be done here, or can we close this out? (maybe duping to bug 1911195, with dependencies fixed up to allow for that)

Comment 10

[Emilio Cobos Álvarez [:emilio]]

The big remaining thing is to autogenerate this list or add more extensive tests to apply it consistently on all properties: https://searchfox.org/mozilla-central/rev/932e5cded30452c4f009b4f9254021f8b9895217/servo/components/style/properties/data.py#523

Comment 11

[Jonathan Kew [:jfkthame]]

Yeah - I had a bit of a look at this, but not sure how to proceed.

It seems easy enough to add a LENGTH flag to SpecifiedValueInfo::SUPPORTED_TYPES for all properties that include a length value; there are just a couple of types where we need to explicitly specify it, and then #[derive(SpecifiedValueInfo)] will deal with cases where there's a length component as part of a more complex type.

The usage of is_zoom_dependent() in cascade_property() can then be replaced with a direct check of SUPPORTED_TYPES.

I pushed a try job with patches to do that much, just to check it doesn't break anything yet... https://treeherder.mozilla.org/jobs?repo=try&revision=08e3ba84784abbc82d23cb53453a20cf6fe17def.

But I'm not seeing how to deal with the construction of the LonghandIdSets in properties.mako.rs, or alternatively how to eliminate those sets and rely on SpecifiedValueInfo directly at the use sites.

I think this needs someone with more of a grasp of Mako/Rust/stylo and how all the pieces play together....

Comment 12

[Daniel Holbert [:dholbert]]

Thanks for that additional context.

In any case this doesn't block interop-2025-webcompat anymore (bug 1948302), since we addressed the related test failures there with bug 1911195, so I'll remove that bug-relationship and mark this as depending on bug 1911195.

(I'll also remove bug 1951003 -- a WPT test-failure bug -- from the blocking list here, since that test passes now. That bug can be closed as fixed by bug 1911195. And this bug's duplicates seem to have been fixed by bug 1911195, too, so I'll adjust them to be duplicates of that bug instead of this one.)