1359603 - Stylo: Port style fixup for text-combine-upright writing mode

Status: RESOLVED FIXED

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

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

As part of ensuring style fixups existing in Servo, we need to port the text-combine-upright writing mode fixup that Gecko performs[1].

[1]: http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/layout/style/nsStyleContext.cpp#781-797

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1359603 - Correct typo about pseudo_element macros.

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

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1359603 - Port text-combine-upright writing mode fixup to Servo.

Ports the Gecko fixup for text-combine-upright writing mode to Servo.  In
addition, this passes the current pseudo element (if any) down to the cascade
for use during the fixup process.

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

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1359603 - Set text combined bit on the context for Servo as well.

We want to set style bits on `nsStyleContext` regardless of which engine is
providing the style source.  The moves the bit set up to `SetStyleBits` which
runs for both sources.

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

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1359603 - Extra spec comments for style adjustments.

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

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1359603 - Tweak style adjust ordering to better match Gecko.

To ease comparison between Gecko's `nsStyleContext::ApplyStyleFixups` and the
Servo equivalent, move them into a similar ordering.

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

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8862097 [details]
Bug 1359603 - Correct typo about pseudo_element macros.

https://reviewboard.mozilla.org/r/134062/#review137104

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8862098 [details]
Bug 1359603 - Port text-combine-upright writing mode fixup to Servo.

https://reviewboard.mozilla.org/r/134064/#review137106

::: servo/components/style/matching.rs:389
(Diff revision 1)
>      fn cascade_with_rules(&self,
>                            shared_context: &SharedStyleContext,
>                            font_metrics_provider: &FontMetricsProvider,
>                            rule_node: &StrongRuleNode,
>                            primary_style: &ComputedStyle,
> -                          is_pseudo: bool)
> +                          pseudo: Option<&PseudoElement>)

I'm making a very similar change in my last patch in bug 1358968. :-)

(That hasn't landed yet since Emilio tells me code around here will change a bit in bug 1331047, so I'm waiting for that to land first.)

::: servo/components/style/style_adjuster.rs:249
(Diff revision 1)
>              box_style.set_overflow_x(overflow_x);
>              box_style.set_overflow_y(overflow_y);
>          }
>      }
>  
> -    /// Adjusts the style to account for display fixups.
> +    /// Adjusts the style to account for various fixups the don't fit naturally into the cascade.

s/the don't/that don't/

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8862099 [details]
Bug 1359603 - Set text combined bit on the context for Servo as well.

https://reviewboard.mozilla.org/r/134066/#review137110

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8862100 [details]
Bug 1359603 - Extra spec comments for style adjustments.

https://reviewboard.mozilla.org/r/134068/#review137112

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8862101 [details]
Bug 1359603 - Tweak style adjust ordering to better match Gecko.

https://reviewboard.mozilla.org/r/134070/#review137114

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ecb5d9dae25
Set text combined bit on the context for Servo as well. r=heycam
https://hg.mozilla.org/integration/autoland/rev/4b1568ebb953
Update test expectations. r=me

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ecb5d9dae25
https://hg.mozilla.org/mozilla-central/rev/4b1568ebb953

Comment 15

[Emilio Cobos Álvarez (:emilio)]

I'm surprised this patch works at all, we resolve text styles using Servo_ComputedValues_Inherit, that doesn't go through the cascade() function.

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> I'm surprised this patch works at all, we resolve text styles using
> Servo_ComputedValues_Inherit, that doesn't go through the cascade() function.

It sounds like it "worked" by setting the style bits only, from chatting on IRC, at least that's probably what triggered a few tests to pass.

Thanks for the pointer and fixing it up in bug 1360508, I wasn't aware of the special path for text.