1360508 - stylo: Adjust text-combine-upright properly.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

Bug 1359603 added the fixup, but to the wrong place.

Style for text is not resolved during the traversal.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1360508: Adjust text-combine properly.

Text style is always resolved via ServoStyleSet::ResolveStyleForText, either
from the frame constructor initially, or from
ServoRestyleManager::ProcessPostTraversalForText.

So text-only adjustments should go there instead. Since that doesn't call into
cascade(), all the code that passes `pseudo` there is dead code we can remove.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1360508: Use the parent's frame style context when handling text-combine.

Instead of the parent style context itself. This also fixes bug 1360530.

It's not clear what should we use in this case, it depends on the resolution in
[1] and [2].

While those get resolved, this is probably ok, and gets rid of the only
styleContext->GetParent() outside nsRuleNode.

[1]: https://github.com/w3c/csswg-drafts/issues/1249
[2]: https://github.com/w3c/csswg-drafts/issues/1281

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1360508: Allow fixups on text styles to be reflected.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8862831 [details]
Bug 1360508: Use the parent's frame style context when handling text-combine.

https://reviewboard.mozilla.org/r/134746/#review137678

I guess it's fine for now... It should be a rare case that people hit this difference.

Comment 5

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

Comment on attachment 8862797 [details]
Bug 1360508: Adjust text-combine properly.

https://reviewboard.mozilla.org/r/134718/#review137660

Thanks for cleaning this up, I didn't realize text had a different path.  Overall, looks good, but a few notes for clarity.

::: layout/style/ServoBindingList.h:336
(Diff revision 1)
>                     nsIAtom* pseudoTag, bool skip_display_fixup,
>                     RawServoStyleSetBorrowed set)
>  SERVO_BINDING_FUNC(Servo_ComputedValues_Inherit, ServoComputedValuesStrong,
>                     RawServoStyleSetBorrowed set,
> -                   ServoComputedValuesBorrowedOrNull parent_style)
> +                   ServoComputedValuesBorrowedOrNull parent_style,
> +                   bool for_text)

Could we use an enum, similar to what I did with `LengthParsingMode`[1]?  Bools are harder to read at a call site, especially if people forget to add annotating comments to explain them.  Enums make it much more obvious.

[1]: http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/layout/style/ServoTypes.h#75

::: servo/components/style/style_adjuster.rs:95
(Diff revision 1)
>      }
>  
> -    /// Change writing mode of text frame for text-combine-upright.
> -    /// It is safe to look at the parent's style because we are looking at
> -    /// inherited properties, and ::-moz-text never matches any rules.
> -    #[cfg(feature = "gecko")]
> +    /// Adjust the style for text style.
> +    ///
> +    /// The adjustments here are a subset of the adjustments generally, because
> +    /// text only inherits properties.

Could you extend this comment to note that text nodes are coming through a different path (not the usual cascade)?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e264da11dfbe
Adjust text-combine properly. r=jryans
https://hg.mozilla.org/integration/autoland/rev/7ac1b8bcb88f
Use the parent's frame style context when handling text-combine. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/766e80198c61
Allow fixups on text styles to be reflected. rpending=heycam a=orange
https://hg.mozilla.org/integration/autoland/rev/043cdedf4903
Test expectation updates. r=emilio

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e264da11dfbe
https://hg.mozilla.org/mozilla-central/rev/7ac1b8bcb88f
https://hg.mozilla.org/mozilla-central/rev/766e80198c61
https://hg.mozilla.org/mozilla-central/rev/043cdedf4903

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8862832 [details]
Bug 1360508: Allow fixups on text styles to be reflected.

https://reviewboard.mozilla.org/r/134748/#review137848