1356510 - stylo: support the sideways-{lr,rl} and deprecated SVG 1.1 writing-mode values

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Comment 1

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa0bd0a0f41dc9e3dd62bea9d6494ca2a54200c6

Comment 2

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a67ec9881859031a1f895b65a49fee0f3ec02a7a

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1356510 - stylo: Update test expectations.

For keyword-typed properties with aliases but no extra_specified values,
the storage of the specified and computed values could be the same,
since value aliases are resolved at parse time.  But to prevent
computed_value::T::parse from recognizing these aliases, we keep the
specified and computed value types distinct.

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

Comment 4

[Cameron McCormack (:heycam)]

Attachment: style: Add Gecko-only support for writing-mode:sideways-{lr,rl}.

Fixes #15213.

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

Comment 5

[Cameron McCormack (:heycam)]

Attachment: style: Add Gecko-only support for the deprecated SVG 1.1 writing-mode aliases.

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

Comment 6

[Cameron McCormack (:heycam)]

Attachment: style: Make text-orientation:sideways-right an alias.

Gecko does not preserve this specified value; it converts it to sideways
at parse time.

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

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1356510 - stylo: Update test expectations.

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

Comment 8

[Manish Goregaokar [:manishearth]]

Comment on attachment 8858598 [details]
style: Add Gecko-only support for writing-mode:sideways-{lr,rl}.

https://reviewboard.mozilla.org/r/130594/#review133198

::: servo/components/style/logical_geometry.rs:32
(Diff revision 1)
>      #[cfg_attr(feature = "servo", derive(HeapSizeOf, Serialize))]
>      pub flags WritingMode: u8 {
>          const FLAG_RTL = 1 << 0,
>          const FLAG_VERTICAL = 1 << 1,
>          const FLAG_VERTICAL_LR = 1 << 2,
> -        const FLAG_SIDEWAYS = 1 << 3,
> +        const FLAG_LINE_INVERTED = 1 << 3,

nit: doc comment for this one, it's not immediately obvious what it means.

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8858599 [details]
style: Add Gecko-only support for the deprecated SVG 1.1 writing-mode aliases.

https://reviewboard.mozilla.org/r/130596/#review133200

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8858600 [details]
style: Make text-orientation:sideways-right an alias.

https://reviewboard.mozilla.org/r/130598/#review133202

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8858601 [details]
Bug 1356510 - stylo: Update test expectations.

https://reviewboard.mozilla.org/r/130600/#review133204

Wish we had more wm reftests, but since sideways is a tumultuous bit of the spec right now anyway I guess it doesn't matter.

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8858597 [details]
Bug 1356510 - stylo: Update test expectations.

https://reviewboard.mozilla.org/r/130592/#review133206

Feels a bit like overkill, but it's not the first time we've added special casing for a few properties, so I guess this is fine :

::: servo/components/style/properties/data.py:36
(Diff revision 1)
>  
>  def to_camel_case(ident):
>      return re.sub("(^|_|-)([a-z])", lambda m: m.group(2).upper(), ident.strip("_").strip("-"))
>  
>  
> +def parse_aliases(value):

Elsewhere (see the bitflags stuff for the font-variant properties in gecko.mako.rs) we just pass in a map. Perhaps we should do the same here? Then again, this is much nicer. Up to you :)

Comment 13

[Cameron McCormack (:heycam)]

(In reply to Manish Goregaokar [:manishearth] from comment #11)
> Wish we had more wm reftests, but since sideways is a tumultuous bit of the
> spec right now anyway I guess it doesn't matter.

Turns out a few more failed (including some you wrote!), but yes, would be nice if there were more sideways-{lr,rl} tests.

(In reply to Manish Goregaokar [:manishearth] from comment #12)
> Elsewhere (see the bitflags stuff for the font-variant properties in
> gecko.mako.rs) we just pass in a map. Perhaps we should do the same here?
> Then again, this is much nicer. Up to you :)

Mmm, yeah.  I think the strings are nicer. :)

Comment 14

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ba807e8f6315b703fde9e2a7f2f4c96beeaa9f4

Comment 15

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #13)
> Turns out a few more failed (including some you wrote!), but yes, would be
> nice if there were more sideways-{lr,rl} tests.

(And by that I mean, tests that were previously annotated failing, which now pass, causing the jobs to fail...)

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8858597 [details]
Bug 1356510 - stylo: Update test expectations.

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/045bf7706bb5
stylo: Update test expectations. r=manishearth

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/045bf7706bb5