Closed Bug 1359603 Opened 3 years ago Closed 3 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(5 files)

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 on attachment 8862097 [details]
Bug 1359603 - Correct typo about pseudo_element macros.

https://reviewboard.mozilla.org/r/134062/#review137104
Attachment #8862097 - Flags: review?(cam) → review+
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/
Attachment #8862098 - Flags: review?(cam) → review+
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
Attachment #8862099 - Flags: review?(cam) → review+
Comment on attachment 8862100 [details]
Bug 1359603 - Extra spec comments for style adjustments.

https://reviewboard.mozilla.org/r/134068/#review137112
Attachment #8862100 - Flags: review?(cam) → review+
Comment on attachment 8862101 [details]
Bug 1359603 - Tweak style adjust ordering to better match Gecko.

https://reviewboard.mozilla.org/r/134070/#review137114
Attachment #8862101 - Flags: review?(cam) → review+
Pushed by jryans@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/0ecb5d9dae25
https://hg.mozilla.org/mozilla-central/rev/4b1568ebb953
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I'm surprised this patch works at all, we resolve text styles using Servo_ComputedValues_Inherit, that doesn't go through the cascade() function.
Flags: needinfo?(jryans)
(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.
Flags: needinfo?(jryans)
You need to log in before you can comment on or make changes to this bug.