Closed Bug 1355402 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

Bug 1355402 - Update test expectations for -moz-{min,max,fit}-content,available}.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1355402 - Update mochitest expectations for prefixed intrinsic size value of flex-basis.
59 bytes, text/x-review-board-request
manishearth
: review+
Details 
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.
Priority: -- → P2
Blocks: 1356087
Assignee: nobody → slyu
Priority: P2 → P1
Xidorn, does this include support for -moz-available, or should that be a separate bug?
Flags: needinfo?(xidorn+moz)
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.
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?
Flags: needinfo?(xidorn+moz)
No, it was just me being confused by the summary of this bug, which refers to property _names_, not values.
Stealing.
Assignee: shing.lyu → hikezoe
Comment on attachment 8868280 [details]
Bug 1355402 - Combine LengthOrPercentage and Auto into LengthOrPercentageOrAuto for {Min,Max}Length.

https://reviewboard.mozilla.org/r/139868/#review143222
Attachment #8868280 - Flags: review?(manishearth) → review+
Comment on attachment 8868281 [details]
Bug 1355402 - Update test expectations for -moz-{min,max,fit}-content,available}.

https://reviewboard.mozilla.org/r/139870/#review143224
Attachment #8868281 - Flags: review?(manishearth) → review+
Comment on attachment 8868282 [details]
Bug 1355402 - Support prefixed intrinsic size value for flex-basis.

https://reviewboard.mozilla.org/r/139872/#review143226
Attachment #8868282 - Flags: review?(manishearth) → review+
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
Attachment #8868283 - Flags: review?(manishearth) → review+
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`).
Attachment #8868280 - Flags: review+ → review-
(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 on attachment 8868282 [details]
Bug 1355402 - Support prefixed intrinsic size value for flex-basis.

This one needs to be re-reviewed.
Attachment #8868282 - Flags: review+ → review?(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
Attachment #8868280 - Flags: review?(manishearth) → review+
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
Attachment #8868381 - Flags: review?(manishearth) → review+
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
Attachment #8868382 - Flags: review?(manishearth) → review+
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
Attachment #8868383 - Flags: review?(manishearth) → review+
Comment on attachment 8868282 [details]
Bug 1355402 - Support prefixed intrinsic size value for flex-basis.

https://reviewboard.mozilla.org/r/139872/#review144804
Attachment #8868282 - Flags: review?(manishearth) → review+
Thanks for the review!
https://github.com/servo/servo/pull/16962
Attachment #8868280 - Attachment is obsolete: true
Attachment #8868381 - Attachment is obsolete: true
Attachment #8868382 - Attachment is obsolete: true
Attachment #8868383 - Attachment is obsolete: true
Attachment #8868282 - Attachment is obsolete: true
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)
Pushed by hikezoe@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: