1383869 - Stylo: Mac bidi reftests fail in Gecko vs. Stylo mode, font sizing seems different

Status: RESOLVED FIXED

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

Description

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

In bug 1380053, we are working on enabling Stylo tests for more desktop platforms like macOS.  (You may want the patches in that bug to test this issue on try.)

Many of the bidi reftests in layout/reftests/bidi/dirAuto fail in Gecko vs. Stylo mode on macOS only.

From squinting at the reftest images, it looks like Stylo might be choosing a different font or sizing it differently than Gecko.

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=115423600&repo=try&lineNumber=7592
Reftest analyzer: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/HYC3wrKFRp-7YikkQiBr8w/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1 (scroll past the writing mode failures)

Comment 1

[Jonathan Kew [:jfkthame]]

The stylo tests show a different font being chosen for the Hebrew characters. It seems that the form element styling, which calls for things like

    font: -moz-field;

or

    font: -moz-button;

(for the <input type="text"> and <button> elements, respectively) is not handled the same by the gecko vs stylo engines.

Inspecting the <button> in layout/reftests/bidi/dirAuto/dynamicDirAuto-setLTR-Auto1.html, for example, and looking at the Computed Styles panel (with Browser styles checked) when stylo is ENABLED shows:

    font-family: "-apple-system";
     button> -moz-button                 forms.css:637

while if stylo is DISABLED, I see:

    font-family: -apple-system;
     button> -moz-use-system-font        forms.css:637

Note that the -apple-system family (which resolves to the hidden .SF NS Text font) does not support Hebrew characters, so font fallback should be taking effect here, but apparently it is behaving differently in the two cases. I suspect this related to the -moz-button vs -moz-use-system-font difference we see in the Inspector, although I don't fully understand what is going on.

Note also that what is actually in forms.css here is "font: -moz-button;", which is a system value for the font property shorthand, while -moz-use-system-font is a special value for the font-family property (not the font shorthand).

The same applies to other system font shorthands (-moz-field is also found in this reftest; non-prefixed values such as message-box or menu exhibit the same behavior).

So a reduced testcase is:

  data:text/html,<div style="font: menu">abc &%23x5d0;&%23x5d1;&%23x5d2;

With stylo, the Inspector reveals that the fonts that end up getting used here are .SF NS Text (aka "System Font"), used for the Latin characters, and Times New Roman for the Hebrew. With Gecko, we get .SF NS Text (Latin) and Arial (Hebrew).

Comment 2

[Manish Goregaokar [:manishearth]]

Wait, what?

-moz-use-system-font should never show in the computed styles. It definitely will never do so in Stylo, and it shouldn't in Gecko either.

This may be a weird gecko bug.

Comment 3

[Manish Goregaokar [:manishearth]]

Oh, sorry, I misread that output.

Comment 4

[Manish Goregaokar [:manishearth]]

I *think* the inspector output is a red herring; Stylo and Gecko handle system fonts differently; and as a side effect Stylo produces better results in the inspector.

But the computed value is the same. It's possible we're not producing *exactly* the same nsFont struct.

Comment 5

[Manish Goregaokar [:manishearth]]

I think I've figured it out, the system font stuff calls aFont->fontlist.SetDefaultFontType(eFamily_none), but we don't replicate that on our side, and FixupNoneGeneric overwrites it.

(haven't yet confirmed this, but making FixupNoneGeneric always set eFamily_none makes them both the same)

Comment 6

[Manish Goregaokar [:manishearth]]

FixupNoneGeneric gets called for Servo but not Gecko. This "fixes" it by replacing the default font type with the default document generic instead of none.

Comment 7

[Manish Goregaokar [:manishearth]]

Aha. Yes.

In Servo we call fixup_none_generic whenever a family was specified *somehow*.

In Gecko we call it *only* for a specified non-system font value. Not even CSS-wide keywords.

Comment 8

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1383869 - stylo: Don't call fixup_generic_font for system or CSS wide specified fonts ;

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

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8896060 [details]
Bug 1383869 - stylo: Don't call fixup_generic_font for system or CSS wide specified fonts ;

https://reviewboard.mozilla.org/r/167328/#review172564

::: servo/components/style/properties/gecko.mako.rs:2184
(Diff revision 1)
>      pub fn reset_font_variation_settings(&mut self, other: &Self) {
>          self.copy_font_variation_settings_from(other)
>      }
>  
>      pub fn fixup_none_generic(&mut self, device: &Device) {
> +        self.gecko.mFont.systemFont = false;

Another option would be to move this assignment into nsRuleNode::FixupNoneGeneric itself.

::: servo/components/style/properties/gecko.mako.rs:2191
(Diff revision 1)
>              bindings::Gecko_nsStyleFont_FixupNoneGeneric(&mut self.gecko, device.pres_context())
>          }
>      }
>  
> +    pub fn fixup_system(&mut self) {
> +        self.gecko.mFont.systemFont = true;

Do we correctly set mFont.systemFont for initial/inherit/unset?

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8896060 [details]
Bug 1383869 - stylo: Don't call fixup_generic_font for system or CSS wide specified fonts ;

https://reviewboard.mozilla.org/r/167328/#review172646

::: servo/components/style/properties/gecko.mako.rs:2184
(Diff revision 1)
>      pub fn reset_font_variation_settings(&mut self, other: &Self) {
>          self.copy_font_variation_settings_from(other)
>      }
>  
>      pub fn fixup_none_generic(&mut self, device: &Device) {
> +        self.gecko.mFont.systemFont = false;

I'm trying to mirror the Gecko code as closely as possible here.

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8896060 [details]
Bug 1383869 - stylo: Don't call fixup_generic_font for system or CSS wide specified fonts ;

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

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8896060 [details]
Bug 1383869 - stylo: Don't call fixup_generic_font for system or CSS wide specified fonts ;

https://reviewboard.mozilla.org/r/167328/#review172646

> I'm trying to mirror the Gecko code as closely as possible here.

Right, but if you move the assignment into nsRuleNode::FixupNonGeneric then Gecko and Stylo will still be mirrored. :-)  Anyway, don't worry, since you're ready to land.

Comment 13

[Manish Goregaokar [:manishearth]]

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

Comment 14

[Mozilla Autoland]

We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae888eb0e5a8
Update reftest expectation.

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/ae888eb0e5a8