1355402 - stylo: Support prefixed intrinsic size value for {min-,max-,}{width,height,{inline,block}-size}

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

Currently, Stylo supports unprefixed intrinsic size keyword values for {min,max}-*, while Gecko supports (and only supports) -moz-prefixed keyword values.

Also Stylo doesn't support that for width/height/{inline,block}-size.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Xidorn, does this include support for -moz-available, or should that be a separate bug?

Comment 2

[Bobby Holley (:bholley)]

Thanks Shing! bz says this is high priority, so let me know if you get stuck or don't have time to work on it.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

I think this bug should probably include all the prefixed keyword values width-ish properties support, including -moz-{{min,max,fit}-content,available}. Is -moz-available somehow particularly interesting and should be done in a separate bug?

Comment 4

[Boris Zbarsky [:bzbarsky]]

No, it was just me being confused by the summary of this bug, which refers to property _names_, not values.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Stealing.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=420e03526bf15ed676a0b2ee466c2a1dff216567

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1355402 - Combine LengthOrPercentage and Auto into LengthOrPercentageOrAuto for {Min,Max}Length.

MozRevie-Commit-ID: AfE1NgGU9Ov

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

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1355402 - Update test expectations for -moz-{min,max,fit}-content,available}.

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1355402 - Support prefixed intrinsic size value for flex-basis.

auto() and zero() are used in the parser for flex shorthand property.

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1355402 - Update mochitest expectations for prefixed intrinsic size value of flex-basis.

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

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8868280 [details]
Bug 1355402 - Combine LengthOrPercentage and Auto into LengthOrPercentageOrAuto for {Min,Max}Length.

https://reviewboard.mozilla.org/r/139868/#review143222

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8868281 [details]
Bug 1355402 - Update test expectations for -moz-{min,max,fit}-content,available}.

https://reviewboard.mozilla.org/r/139870/#review143224

Comment 13

[Manish Goregaokar [:manishearth]]

Comment on attachment 8868282 [details]
Bug 1355402 - Support prefixed intrinsic size value for flex-basis.

https://reviewboard.mozilla.org/r/139872/#review143226

Comment 14

[Manish Goregaokar [:manishearth]]

Comment on attachment 8868283 [details]
Bug 1355402 - Update mochitest expectations for prefixed intrinsic size value of flex-basis.

https://reviewboard.mozilla.org/r/139874/#review143232

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8868280 [details]
Bug 1355402 - Combine LengthOrPercentage and Auto into LengthOrPercentageOrAuto for {Min,Max}Length.

https://reviewboard.mozilla.org/r/139868/#review143230

::: servo/components/style/properties/longhand/position.mako.rs:157
(Diff revision 1)
>        if logical:
>          spec = "https://drafts.csswg.org/css-logical-props/#propdef-%s"
>      %>
>      // width, height, block-size, inline-size
> +    % if product == "gecko":
> +        ${helpers.predefined_type("%s" % size,

We can't use a predefined type here. The ToComputedValue impl, like that of Min/MaxSize, needs to filter out kw values in the block direction.

We should specify newtypes for each one (like done for Min/MaxSize with `struct SpecifiedValue(MaxLength)`), with custom parse and ToComputedValue impls. They should share the same computed value type (`computed::MozLength`).

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Manish Goregaokar [:manishearth] from comment #15)
> Comment on attachment 8868280 [details]
> Bug 1355402 - Support prefixed intrinsic size value for
> {min-,max-,}{width,height,{inline,block}-size}}.
> 
> https://reviewboard.mozilla.org/r/139868/#review143230
> 
> ::: servo/components/style/properties/longhand/position.mako.rs:157
> (Diff revision 1)
> >        if logical:
> >          spec = "https://drafts.csswg.org/css-logical-props/#propdef-%s"
> >      %>
> >      // width, height, block-size, inline-size
> > +    % if product == "gecko":
> > +        ${helpers.predefined_type("%s" % size,
> 
> We can't use a predefined type here. The ToComputedValue impl, like that of
> Min/MaxSize, needs to filter out kw values in the block direction.
> 
> We should specify newtypes for each one (like done for Min/MaxSize with
> `struct SpecifiedValue(MaxLength)`), with custom parse and ToComputedValue
> impls. They should share the same computed value type
> (`computed::MozLength`).

Oh, thanks. I did not notice it at all. I will check Min/MaxSize. Thanks for the pointer!

Comment 17

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=532d230e4bc694f03d2bc396d083e6bdd03a0cdd

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1355402 - Rename MinLength to MozLength.

So that we can reuse this for non-min-prefixed properties (e.g. width).

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

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1355402 - Factor out implemantations for {min,max} size properties as a macro.

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

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1355402 - Support prefixed intrinsic size value for {width,height,{inline,block}-size}}.

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

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868280 [details]
Bug 1355402 - Combine LengthOrPercentage and Auto into LengthOrPercentageOrAuto for {Min,Max}Length.

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

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868281 [details]
Bug 1355402 - Update test expectations for -moz-{min,max,fit}-content,available}.

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

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868282 [details]
Bug 1355402 - Support prefixed intrinsic size value for flex-basis.

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

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868283 [details]
Bug 1355402 - Update mochitest expectations for prefixed intrinsic size value of flex-basis.

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

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868282 [details]
Bug 1355402 - Support prefixed intrinsic size value for flex-basis.

This one needs to be re-reviewed.

Comment 26

[Manish Goregaokar [:manishearth]]

Comment on attachment 8868280 [details]
Bug 1355402 - Combine LengthOrPercentage and Auto into LengthOrPercentageOrAuto for {Min,Max}Length.

https://reviewboard.mozilla.org/r/139868/#review144792

Comment 27

[Manish Goregaokar [:manishearth]]

Comment on attachment 8868381 [details]
Bug 1355402 - Rename MinLength to MozLength.

https://reviewboard.mozilla.org/r/139960/#review144796

::: servo/components/style/values/computed/length.rs:564
(Diff revision 1)
>  pub type LengthOrNumber = Either<Length, Number>;
>  
>  /// Either a computed `<length>` or the `normal` keyword.
>  pub type LengthOrNormal = Either<Length, Normal>;
>  
>  /// A value suitable for a `min-width` or `min-height` property.

also width and height

Comment 28

[Manish Goregaokar [:manishearth]]

Comment on attachment 8868382 [details]
Bug 1355402 - Factor out implemantations for {min,max} size properties as a macro.

https://reviewboard.mozilla.org/r/139962/#review144798

::: servo/components/style/properties/helpers.mako.rs:1038
(Diff revision 1)
>      }
>  </%def>
> +
> +// Define property that supports prefixed intrinsic size keyword values for gecko.
> +// E.g. -moz-max-content, -moz-min-content, etc.
> +<%def name="gecko_length_type(name, length_type, initial_value, logical, **kwargs)">

I would call this gecko_size_type

Comment 29

[Manish Goregaokar [:manishearth]]

Comment on attachment 8868383 [details]
Bug 1355402 - Support prefixed intrinsic size value for {width,height,{inline,block}-size}}.

https://reviewboard.mozilla.org/r/139964/#review144802

Comment 30

[Manish Goregaokar [:manishearth]]

Comment on attachment 8868282 [details]
Bug 1355402 - Support prefixed intrinsic size value for flex-basis.

https://reviewboard.mozilla.org/r/139872/#review144804

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Thanks for the review!
https://github.com/servo/servo/pull/16962

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868281 [details]
Bug 1355402 - Update test expectations for -moz-{min,max,fit}-content,available}.

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

Comment 33

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868283 [details]
Bug 1355402 - Update mochitest expectations for prefixed intrinsic size value of flex-basis.

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

Comment 34

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 074a5a7d97d8 -d 8a0798c5e404: rebasing 397530:074a5a7d97d8 "Bug 1355402 - Update test expectations for -moz-{min,max,fit}-content,available}. r=manishearth"
merging layout/generic/crashtests/crashtests.list
merging layout/reftests/bugs/reftest.list
merging layout/style/test/stylo-failures.md
warning: conflicts while merging layout/style/test/stylo-failures.md! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868281 [details]
Bug 1355402 - Update test expectations for -moz-{min,max,fit}-content,available}.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139870/diff/3-4/

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868283 [details]
Bug 1355402 - Update mochitest expectations for prefixed intrinsic size value of flex-basis.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139874/diff/3-4/

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab4e65106302
Update test expectations for -moz-{min,max,fit}-content,available}. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/ceaa661a7fdb
Update mochitest expectations for prefixed intrinsic size value of flex-basis. r=manishearth

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef82929381b2
Update reftest expectations. r=me

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ab4e65106302
https://hg.mozilla.org/mozilla-central/rev/ceaa661a7fdb
https://hg.mozilla.org/mozilla-central/rev/ef82929381b2