1390339 - stylo: Stylo supports calc in media queries, gecko doesn't

Status: RESOLVED FIXED

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

Description

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Currently we have some passing tests in web platform tests about these behavior that gecko fails. Specifically:

/css/mediaqueries-3/mq-calc-001.html
/css/mediaqueries-3/mq-calc-002.html
/css/mediaqueries-3/mq-calc-003.html
/css/mediaqueries-3/mq-calc-004.html
/css/mediaqueries-3/mq-calc-005.html

Gecko doesn't support calc values currently in media queries. Chrome and Safari seem to fail on those also. Since no other browsers support that either way seems safe enough. But we might want to unship that just in case in the future.
 
Gecko bug for that is https://bugzilla.mozilla.org/show_bug.cgi?id=1256575

Comment 1

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

In stylo meeting bholley said this might be more problematic since it's not implemented in any browser. Someone might accidentally put calc inside media queries and never test them. Since it's not working on any browser it's not a problem. But if we ship that, that might break some websites. I'm going to unship that.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I don't really think it's a big deal to be honest. I don't think writing a media query accidentally is a big deal, and the length code supports parsing them as anything else.

I think Servo's style system is the most compliant wrt calc(), and I'd rather not unship stuff just-because...

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Yes, it might not be a big deal but I don't think we should just assume it wouldn't cause a problem and move on. There might not be many website with that error but do we really want to leave this up to chance?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Nazım Can Altınova [:canaltinova] from comment #3)
> Yes, it might not be a big deal but I don't think we should just assume it
> wouldn't cause a problem and move on. There might not be many website with
> that error but do we really want to leave this up to chance?

Then why not just unshipping all our behaviour differences? I don't think that's a good argument, this seems like a really unlikely cause of compat problems (I can think on worse ones)... And I rather don't unship something that matches a spec that has no web compat concerns.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> (In reply to Nazım Can Altınova [:canaltinova] from comment #3)
> > Yes, it might not be a big deal but I don't think we should just assume it
> > wouldn't cause a problem and move on. There might not be many website with
> > that error but do we really want to leave this up to chance?
> 
> Then why not just unshipping all our behaviour differences?

It depends on the behavior difference. In many cases the new behavior matches what other UAs do, or is more benign than "affecting what rules match".

>  I don't think
> that's a good argument, this seems like a really unlikely cause of compat
> problems (I can think on worse ones)... And I rather don't unship something
> that matches a spec that has no web compat concerns.

Being in a spec doesn't say much about web-compatibility if no browser ships it. Somebody needs to take the plunge at some point, of course, but ideally not while also shipping an entirely new style system.

Basically, we need to weigh any intentional behavior changes by risk, and err on the very conservative side when evaluating that risk. My assessment was that shipping this has non-negligible risk, but I don't have a ton of experience in CSS web-compat issues. David, what's your opinion here?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(For an example of a problem that I think is more likely to cause compat problems than this, see bug 1391120, for example)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This feels like there's a little risk, but most additions of features (and I feel this falls in that category) are lower risk than certain changes to existing behavior.  It seems reasonable to try to ship it, but it's also reasonable to try to hold it back by a release or two.  (But if we do, we should remember to ship it!)

Have we tested that 'em' units are interpreted relative to the correct things?  If we are going to ship it, it seems like we should be checking that it works correctly, and the tests in comment 0 are far from sufficient.  (Additions to layout/style/test/test_media_queries.html , which there's also a version of in wpt/css/mediaqueries-3/, would be ideal.)

Comment 8

[Bobby Holley (:bholley)]

Thanks dbaron.

So it sounds like either approach is defensible, but we need to do some work either way (either unshipping or writing tests). Given that, I'm going to default to the conservative option. Let's unship for now, assuming it's trivial, and file a followup to turn it back on in 58 or 59.

Nazim is going to take care of this.

Comment 9

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1390339 - stylo: Adjust test expectations after removing calc support from media queries

Gecko currently doesn't support calc inside media queries. We should
also remove the calc support temporarily for parity with gecko. We can
add this support back in next releases.

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8903385 [details]
Bug 1390339 - stylo: Adjust test expectations after removing calc support from media queries

https://reviewboard.mozilla.org/r/175226/#review180466

Looks good, I guess. I think adding test-cases and ship it wouldn't have been that hard fwiw.

With those, r=me

::: servo/components/style/gecko/media_queries.rs:562
(Diff revision 1)
>              let value = match feature.mValueType {
>                  nsMediaFeature_ValueType::eLength => {
> -                    MediaExpressionValue::Length(
> -                        specified::Length::parse_non_negative(context, input)?)
> +                    let length = Length::parse_non_negative(context, input)?;
> +                    // FIXME(canaltinova): Gecko doesn't support calc inside media
> +                    // queries. This check is for temporarily remove it for parity
> +                    // with gecko. We should remove this check when we want to support it.

Please file a bug for this, and mention this bug in the "depends on", or "see also" fields. Also, reference it from this comment.

::: servo/components/style/gecko/media_queries.rs:569
(Diff revision 1)
> +                        return Err(StyleParseError::UnspecifiedError.into())
> +                    }
> +                    MediaExpressionValue::Length(length)
>                  },
>                  nsMediaFeature_ValueType::eInteger => {
>                      let i = input.expect_integer()?;

While you're here, please add a `FIXME(emilio)` here about using `Integer::parse` instead of `expect_integer` to handle `calc` properly in integer expressions.

Comment 11

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8903385 [details]
Bug 1390339 - stylo: Adjust test expectations after removing calc support from media queries

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea78e52393a5
stylo: Adjust test expectations after removing calc support from media queries r=emilio

Comment 13

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

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

Comment 14

[Chris Mills [:cmills]]

Documented:

https://developer.mozilla.org/en-US/docs/Web/CSS/calc#Quantum_CSS_notes
https://developer.mozilla.org/en-US/Firefox/Releases/57#Quantum_CSS_notes

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Hi Chris, note that support for this support was removed in this bug (we'll bring it back in bug 1396057), so this specific bit shouldn't be documented.

Comment 16

[Chris Mills [:cmills]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> Hi Chris, note that support for this support was removed in this bug (we'll
> bring it back in bug 1396057), so this specific bit shouldn't be documented.

OK, thanks Emilio. I've removed the reference to the bug from the support note.