Closed Bug 1390339 Opened 7 years ago Closed 7 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: canova, Assigned: canova)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

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
See Also: → 1256575
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.
Assignee: nobody → canaltinova
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...
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?
(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.
(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?
Flags: needinfo?(dbaron)
(For an example of a problem that I think is more likely to cause compat problems than this, see bug 1391120, for example)
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.)
Flags: needinfo?(dbaron)
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.
Priority: -- → P2
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.
Attachment #8903385 - Flags: review?(emilio) → review+
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ea78e52393a5
stylo: Adjust test expectations after removing calc support from media queries r=emilio
https://hg.mozilla.org/mozilla-central/rev/ea78e52393a5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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.
Flags: needinfo?(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.
Flags: needinfo?(cmills)
You need to log in before you can comment on or make changes to this bug.