1438234 - stylo: Interaction of shorthands and prefs isn't great.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

See bug 1435692 comment 28.

The gist of it:

Stylo assumes that if a given shorthand is available, all its longhands are. That seems a reasonable assumption off-hand, but it has terrible implications, specially in presence of the `all` shorthand.

In particular, if you go with nightly with stylo enabled and do something like the following in the web console:

> document.body.style.all = "inherit";
> document.body.style.fontFamily = "10px"; // Doesn't matter, just any property so that we need to expand "all" to its longhands.
> document.body.style.cssText

You'll see disabled properties in there, including, for example, font-variation-settings.

Now, regarding how to fix this... I thought it'd be enough to check whether the longhands are available for content at parse time, and avoid expanding those longhands. That'd fix that particular test...

But that is obviously not enough, because then, when serializing, we stash all the longhands in a struct, and call ToCss on that... Which means we'll never serialize the longhand!

Comment 1

[Emilio Cobos Álvarez (:emilio)]

So, I don't have a clear idea of how to fix this with the current shorthand serialization model, which is unfortunate...

We'd need to somehow change LonghandsToSerialize / Longhands make some longhands optional, I guess...

Comment 2

[Emilio Cobos Álvarez (:emilio)]

And we'd also need to special-case `all` to avoid currently-disabled properties on serialization... Gah.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I submitted a couple preliminar cleanups here: 

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

And here:

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

Our serialization code isn't as simple as it used to be, heh :)

The idea is to shuffle things around so that shorthands() and longhands() stop being arrays, but iterators that check whether the properties are enabled.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/20078 is most of the fix, now I just need to write the iterators after that.

Comment 5

[Jet Villegas (inactive)]

[ Triage 2017/02/20: P3 ]

Comment 6

[Emilio Cobos Álvarez (:emilio)]

So I had something, but it terribly broke UA / chrome-only shorthands. I put up a patch to avoid serializing preffd-off properties and was green, but then I realised that that's totally unfortunate, because there's CSSStyleDeclaration::Length and such, so we cannot just handle them during serialization.

Slightly different approach: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffbd2c25b089072d820cdb0952d675f3eb537242

Comment 7

[Jonathan Kew [:jfkthame]]

I took the patches from your try push, together with the font-optical-sizing patches in bug 1435692, and tried a local build.

Unfortunately, when a testcase tries to read the 'font' shorthand after setting all its subproperties (except the preffed-off font-optical-sizing one), it panics:

TEST-START | layout/style/test/test_bug377947.html
TEST-PASS | layout/style/test/test_bug377947.html | list-style shorthand should start off empty 
TEST-PASS | layout/style/test/test_bug377947.html | list-style shorthand should be empty when some subproperties specified 
TEST-PASS | layout/style/test/test_bug377947.html | list-style shorthand should produce value when all subproperties set 
TEST-PASS | layout/style/test/test_bug377947.html | list-style shorthand be empty after removal 
TEST-PASS | layout/style/test/test_bug377947.html | list-style shorthand should produce value when shorthand set 
TEST-PASS | layout/style/test/test_bug377947.html | list-style shorthand be empty after removal 
TEST-PASS | layout/style/test/test_bug377947.html | font shorthand should start off empty 
TEST-PASS | layout/style/test/test_bug377947.html | font shorthand should be empty when some subproperties specified 
GECKO(1850) | thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Error', src/libcore/result.rs:906:4
GECKO(1850) | stack backtrace:
GECKO(1850) |    0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
GECKO(1850) |    1: std::sys_common::backtrace::_print
GECKO(1850) |    2: std::panicking::default_hook::{{closure}}
GECKO(1850) |    3: std::panicking::default_hook
GECKO(1850) |    4: std::panicking::rust_panic_with_hook
GECKO(1850) |    5: std::panicking::begin_panic
GECKO(1850) |    6: std::panicking::begin_panic_fmt
GECKO(1850) |    7: rust_begin_unwind
GECKO(1850) |    8: core::panicking::panic_fmt
GECKO(1850) | Redirecting call to abort() to mozalloc_abort

(When the new subproperty is preffed on, the same test runs happily.)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> I took the patches from your try push, together with the font-optical-sizing
> patches in bug 1435692, and tried a local build.
> 
> Unfortunately, when a testcase tries to read the 'font' shorthand after
> setting all its subproperties (except the preffed-off font-optical-sizing
> one), it panics:
> 
> TEST-START | layout/style/test/test_bug377947.html
> TEST-PASS | layout/style/test/test_bug377947.html | list-style shorthand
> should start off empty 
> TEST-PASS | layout/style/test/test_bug377947.html | list-style shorthand
> should be empty when some subproperties specified 
> TEST-PASS | layout/style/test/test_bug377947.html | list-style shorthand
> should produce value when all subproperties set 
> TEST-PASS | layout/style/test/test_bug377947.html | list-style shorthand be
> empty after removal 
> TEST-PASS | layout/style/test/test_bug377947.html | list-style shorthand
> should produce value when shorthand set 
> TEST-PASS | layout/style/test/test_bug377947.html | list-style shorthand be
> empty after removal 
> TEST-PASS | layout/style/test/test_bug377947.html | font shorthand should
> start off empty 
> TEST-PASS | layout/style/test/test_bug377947.html | font shorthand should be
> empty when some subproperties specified 
> GECKO(1850) | thread '<unnamed>' panicked at 'called `Result::unwrap()` on
> an `Err` value: Error', src/libcore/result.rs:906:4
> GECKO(1850) | stack backtrace:
> GECKO(1850) |    0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
> GECKO(1850) |    1: std::sys_common::backtrace::_print
> GECKO(1850) |    2: std::panicking::default_hook::{{closure}}
> GECKO(1850) |    3: std::panicking::default_hook
> GECKO(1850) |    4: std::panicking::rust_panic_with_hook
> GECKO(1850) |    5: std::panicking::begin_panic
> GECKO(1850) |    6: std::panicking::begin_panic_fmt
> GECKO(1850) |    7: rust_begin_unwind
> GECKO(1850) |    8: core::panicking::panic_fmt
> GECKO(1850) | Redirecting call to abort() to mozalloc_abort
> 
> (When the new subproperty is preffed on, the same test runs happily.)

Yes, that's expected, and it's the last bit remaining to fix, which is making it understand that font-optical-sizing may be missing.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/20138 contains the fix for that, and should be ready to land, just fyi.

Sorry for the lag here, turned out to be tricky as heck :)

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Just fyi, you can apply patches directly from github like:

curl -L https://github.com/servo/servo/pull/20138.patch | git am - --directory=servo

(or the relevant hg command equivalent to git am)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=399baeb32221c620fef56c438144f11272bdba21 looks green enough (orange is for it being on top of yesterday's autoland)w

Comment 12

[Emilio Cobos Álvarez (:emilio)]

The fix for this landed in https://github.com/servo/servo/pull/20138 / https://hg.mozilla.org/mozilla-central/rev/08a85550fb3b9eaae2974e53594600f9b5eb02eb.