1407701 - stylo: need to honor display:-webkit-box preferentially over later display:-moz-box in the same CSS declaration block

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

tl;dr: we have a weird display:-webkit-box interop hack that seems not to have been preserved in stylo.

Details:
--------
There are some mobile sites out there which have styles like:
.foo {
  display: -webkit-box;
  display: -moz-box;
}

Gecko's CSS parser has a special case for this which ignores the later "-moz-box" (or technically, treats it as a repetition of the earlier "-webkit-box" value):

https://dxr.mozilla.org/mozilla-central/rev/a0488ecc201c04f2617e7b02f039344e8fbf0d9a/layout/style/nsCSSParser.cpp#7036-7062

(We do this because: generally, this sort of pattern indicates that the author thinks [incorrectly] that -webkit-box and -moz-box are interchangeable. And they've probably only tested the -webkit one, since that's the one that actually behaves intuitively on the web. So, we're betting that our -webkit-box emulation [which is pretty good] will be more likely to produce their intended layout.)

Unfortunately, based on some local testing, I think this hack doesn't work when stylo is enabled.

There are [or were at some point] mobile sites that depend on this to behave correctly -- see bug 1107378 comment 15, which has two important-in-Asian-markets sites that have this pattern (m.qunar.com and mogujie.com). So we need to recreate this special case in Stylo, I think.  It might be too late for 57, but we should try to get it back in for 58.

Comment 1

[Daniel Holbert [:dholbert]]

Fortunately, this might not cause too much trouble in 57, because:
 * -webkit-box styling is mostly important on mobile sites.
 * As I recall, stylo isn't enabled on Firefox for Android yet.

I would anticipate we'll need to fix this to avoid regressing some sites when we do ship stylo on Android, though.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (display:-moz-box specified after display:-webkit-box, in various forms)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: (ignore; this testcase is not as relevant as I thought)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: reference case 1 (display:-webkit-box specified after display:-moz-box, in various forms)

The expectation here is that "testcase 1" should show the same alert (and render the same) as this reference case.

Comment 5

[Daniel Holbert [:dholbert]]

(I thought I added a testcase for this behavior, but apparently not...)

Comment 6

[Daniel Holbert [:dholbert]]

For the record, the gecko version of this hack was added in:

* https://hg.mozilla.org/mozilla-central/rev/57410350de4d (where we treat -webkit-box [and later -moz-box] as flex)
* https://hg.mozilla.org/mozilla-central/rev/05b672699c45 (where, now that we [separately] treat -webkit-box as an actual parsed keyword, we just parse -webkit-box directly, and the remaining hack is to just treat later -moz-box as a repetition of the -webkit-box keyword)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> (I thought I added a testcase for this behavior, but apparently not...)

Ah, I did indeed add a testcase for the version of this behavior in the *original* CSSUnprefixingService impl (at which point we converted -webkit-box and subsequent -moz-box keywords to 'flex' under the hood).  But it was part of a larger test that I removed when we ripped out CSSUnprefixingService. :(  That happened here:
  https://hg.mozilla.org/mozilla-central/rev/171262d16e80#l15.79

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Just to confirm, this is done during rule parsing right? (if so, that may be easier)

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Err, meant to ni? Daniel too, plan to fix it ASAP.

Comment 10

[Daniel Holbert [:dholbert]]

Correct. Thanks!

Comment 11

[Daniel Holbert [:dholbert]]

Actually, the reftest webkit-display-values-1.html does kinda serve as a testcase for this (it does have "display:-webkit-box;display:-moz-box"), but it's not a particularly robust test, because it renders the same regardless of which display value we pick, if the default font is small enough.

Fortunately, the default font on Android is large enough to make it render differently, which is how this came to my attention in bug 1407525 (which is an Android+stylo reftest failure that should become fixed when this bug here is fixed).

Anyway -- as part of the patch here, we should probably add a more thorough reftest or mochitest that actually checks the computed style.

Comment 12

[Cameron McCormack (:heycam)]

Does this need to be fixed for 57?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch

I'd really prefer not having to implement this, seems somewhat gross.

There's the patch. I believe we should try to unship it ASAP afterwards.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #12)
> Does this need to be fixed for 57?

Per comment 0 I was assuming not, but if we need to I can prepare a more minimal patch.

Comment 15

[Daniel Holbert [:dholbert]]

Yeah, my risk/benefit thoughts here are that we shouldn't try to get this fixed for 57.  The benefits here are mostly for a long tail of one-off mobile sites, and we aren't shipping stylo on mobile (Android) yet.  So, there's probably a pretty limited real-world benefit to having this fixed in 57.  And a behavior change like this seems risky this late in the release cycle.

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8918323 [details] [diff] [review]
Patch

Review of attachment 8918323 [details] [diff] [review]:
-----------------------------------------------------------------

Please update the commit message to be more commit message like, maybe:

Implement compatibility hack for mobile that prevents display:-moz-box from
overriding display:-webkit-box when parsing style sheets.  See <bugzilla link
from original implementation>.

Also, I'm curious why the existing reftest for this (layout/reftests/webkit-box/webkit-display-values-1.html) does not currently fail.

::: components/style/properties/declaration_block.rs
@@ +46,5 @@
> +pub enum DeclarationSource {
> +    /// The declaration was obtained from CSS parsing of sheets and such.
> +    Parsing,
> +    /// The declaration was obtained from CSSOM.
> +    CssOm,

As an aside, as someone who feels that initial caps for initialism identifiers looks a bit odd, "CssOm" even weirder. :-)

@@ +499,5 @@
> +                            self.declarations_importance.set(i as u32, importance.important());
> +                            return true;
> +                        }
> +                        DeclarationSource::Parsing => {
> +                            // As a compatibility hack, specially on Android,

Nit: s/specially on Android/especially on mobile/

(it's not Android specific, but mobile site specific)

@@ +500,5 @@
> +                            return true;
> +                        }
> +                        DeclarationSource::Parsing => {
> +                            // As a compatibility hack, specially on Android,
> +                            // don't allow to override a prefixed webkit display

s/allow to override/allow overriding/

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #16)
> Also, I'm curious why the existing reftest for this
> (layout/reftests/webkit-box/webkit-display-values-1.html) does not currently
> fail.

It does fail, on Android (bug 1407525), but only subtly.

That test is misleading (sorry) -- it's written with the *intent* that it fail in an obvious way if we mistakenly honor "display:-moz-box" over the earlier "display:-webkit-box". Specifically: the intent was that "-webkit-box-pack:center" wouldn't be honored in that case.

But really, we treat -webkit-box-pack as an alias for -moz-box-pack (which triggers centering both in a -moz-box and in an emulated -webkit-box).  So, in fact, it centers regardless of which display type we settle on. And the test passes (on desktop) because the two display types are similar enough (in this limited scenario) that it unfortunately looks the same regardless.

"Fortunately", on Android, the test does fail because there's a default line-height difference there (maybe a different default font-size), which makes the test render a bit differently if we honor the -moz-box, because -moz-box behaves kinda like a line participant for some reason.

But yeah -- it's not a very good test, unfortunately. :(

Comment 18

[Hiroyuki Ikezoe (:hiro)]

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

Comment 19

[Hiroyuki Ikezoe (:hiro)]

As per comment 15.