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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: xidorn, Assigned: hiro)

Tracking

(Blocks: 4 bugs)

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

4 months ago
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
(Reporter)

Updated

3 months ago
Blocks: 1356087

Updated

3 months ago
Assignee: nobody → slyu
Priority: P2 → P1

Comment 1

3 months ago
Xidorn, does this include support for -moz-available, or should that be a separate bug?
Flags: needinfo?(xidorn+moz)

Updated

3 months ago
Blocks: 1324348
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.
(Reporter)

Comment 3

3 months ago
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)

Comment 4

3 months ago
No, it was just me being confused by the summary of this bug, which refers to property _names_, not values.
(Assignee)

Comment 5

2 months ago
Stealing.
Assignee: shing.lyu → hikezoe
(Assignee)

Comment 6

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=420e03526bf15ed676a0b2ee466c2a1dff216567
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

2 months ago
mozreview-review
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 12

2 months ago
mozreview-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 13

2 months ago
mozreview-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 14

2 months ago
mozreview-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 15

2 months ago
mozreview-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-
(Assignee)

Comment 16

2 months ago
(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!
(Assignee)

Comment 17

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=532d230e4bc694f03d2bc396d083e6bdd03a0cdd
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

2 months ago
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 26

2 months ago
mozreview-review
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 27

2 months ago
mozreview-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 28

2 months ago
mozreview-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 29

2 months ago
mozreview-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 30

2 months ago
mozreview-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+
(Assignee)

Comment 31

2 months ago
Thanks for the review!
https://github.com/servo/servo/pull/16962
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8868280 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8868381 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8868382 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8868383 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8868282 - Attachment is obsolete: true

Comment 34

2 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 37

2 months ago
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

Comment 38

2 months ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef82929381b2
Update reftest expectations. r=me

Comment 39

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab4e65106302
https://hg.mozilla.org/mozilla-central/rev/ceaa661a7fdb
https://hg.mozilla.org/mozilla-central/rev/ef82929381b2
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.