Closed Bug 1342710 Opened 7 years ago Closed 7 years ago

stylo: Assertion failure: aCoord.IsCoordPercentCalcUnit() in a few css-grid reftests

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: emilio, Assigned: manishearth)

References

Details

Attachments

(1 file)

After landing https://github.com/servo/servo/pull/15730 one of these appeared, will investigate more when I have a stylo build with that patch.
Concretely layout/reftests/css-grid/grid-repeat-auto-fill-fit-011.html
I guess this may be the same as what Manish is investigating in bug 1342596.
That sounds likely.
Depends on: 1342596
Even more likely given grid-auto-min-sizing-min-content-min-size-004.html is also affected by this.
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/830c88f032fc
Bug 1324633: Update reftests and crashtests expectations.
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Comment on attachment 8841288 [details]
Bug 1342710 - stylo: Disallow keyword values in min/max-size properties;

I think xidorn can probably review this faster than I can.
Attachment #8841288 - Flags: review?(bobbyholley) → review?(xidorn+moz)
Also, thanks for fixing this! Especially on a weekend. :-)
Comment on attachment 8841288 [details]
Bug 1342710 - stylo: Disallow keyword values in min/max-size properties;

https://reviewboard.mozilla.org/r/115540/#review116978

r=me with the following issues addressed.

::: servo/components/style/properties/longhand/position.mako.rs:229
(Diff revision 1)
> -                                  "computed::MinLength::LengthOrPercentage(" +
> -                                  "computed::LengthOrPercentage::Length(Au(0)))",
> +            // Gecko cannot handle keyword values for these in the block direction
> +            // so we must filter them out

It is not "Gecko cannot handle ...". It doesn't make sense to have those keyword value on block direction size, and the spec also says they should be computed to `auto` when not applied for inline-axis (although there is no `auto` defined at all for {min,max}-{width,height}... which is a spec issue... I filed to w3c/csswg-drafts#1062.)

::: servo/components/style/properties/longhand/position.mako.rs:246
(Diff revision 1)
> +                        self.0.has_viewport_percentage()
> +                    }
> +                }
> +
> +                pub mod computed_value {
> +

This blank line doesn't seem necessary.

::: servo/components/style/properties/longhand/position.mako.rs:285
(Diff revision 1)
> -                                  "LengthOrPercentage",
> -                                  "computed::LengthOrPercentage::Length(Au(0))",
> +                                <% is_height = "true" if "height" in size else "false" %>
> +                                if ${is_height} ^ context.style().writing_mode.is_vertical() {

I find xor makes the code harder to understand at the first glance... Probably consider changing it to verbose form, e.g. `(is_height && !is_vertical) || (!is_height && is_vertical)` and hope that the compiler can optimize it... or at least, `is_height != is_vertical` which is probably mentally easier to understand than xor.

::: servo/components/style/properties/longhand/position.mako.rs:303
(Diff revision 1)
> -                                  "LengthOrPercentageOrNone",
> -                                  "computed::LengthOrPercentageOrNone::None",
> +                                      "LengthOrPercentage",
> +                                      "computed::LengthOrPercentage::Length(Au(0))",

You need different type and initial value for min and max, no?
Attachment #8841288 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8841288 [details]
Bug 1342710 - stylo: Disallow keyword values in min/max-size properties;

https://reviewboard.mozilla.org/r/115540/#review116978

> You need different type and initial value for min and max, no?

This is Servo mode, Servo doesn't support auto/none/keywords yet.
Comment on attachment 8841288 [details]
Bug 1342710 - stylo: Disallow keyword values in min/max-size properties;

https://reviewboard.mozilla.org/r/115540/#review116978

> This is Servo mode, Servo doesn't support auto/none/keywords yet.

But the original code use `LengthOrPercentageOrNone` for max in Servo mode.
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0a3f512c8451
stylo: Disallow keyword values in min/max-size properties; r=xidorn
https://hg.mozilla.org/mozilla-central/rev/830c88f032fc
https://hg.mozilla.org/mozilla-central/rev/0a3f512c8451
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8841288 [details]
Bug 1342710 - stylo: Disallow keyword values in min/max-size properties;

https://reviewboard.mozilla.org/r/115540/#review118346
Attachment #8841288 - Flags: review?(bobbyholley)
You need to log in before you can comment on or make changes to this bug.