Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

style: Make numbers keep track of whether they were specified as calc(). #16144

Merged
merged 13 commits into from Mar 27, 2017

Conversation

Projects
None yet
8 participants
@emilio
Copy link
Member

commented Mar 26, 2017

Fixes #15960


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Mar 26, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/gecko.mako.rs, components/style/values/specified/mod.rs, components/style/properties/shorthand/position.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/longhand/border.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/gecko_bindings/sugar/ns_timing_function.rs
@emilio

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

r? @upsuper or @Manishearth or @heycam

This is like a kinda big refactor, and prone to bitrot, so I'd appreciate fast review.

@highfive highfive assigned upsuper and unassigned KiChjang Mar 26, 2017

@emilio

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2017

⌛️ Trying commit a14128a with merge db5e385...

bors-servo added a commit that referenced this pull request Mar 26, 2017

Auto merge of #16144 - emilio:number-calc, r=<try>
style: Make numbers keep track of whether they were specified as calc().

Fixes #15960

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16144)
<!-- Reviewable:end -->
@emilio

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

Note that Gecko fails the second test (it preserves the full expression), and Blink fails both (it has the same behavior as Servo without this patch).

I believe we want to adopt this behavior, but if we want to keep Gecko's behavior I'd prefer to do it as a followup, given how big is this patch already.

@emilio

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

Servo's behavior is the correct one per https://drafts.csswg.org/css-values/#calc-serialize

@emilio emilio force-pushed the emilio:number-calc branch from ba1956e to c381ae2 Mar 26, 2017

@emilio

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2017

⌛️ Trying commit c381ae2 with merge 0435933...

bors-servo added a commit that referenced this pull request Mar 26, 2017

Auto merge of #16144 - emilio:number-calc, r=<try>
style: Make numbers keep track of whether they were specified as calc().

Fixes #15960

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16144)
<!-- Reviewable:end -->
@emilio

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2017

💔 Test failed - linux-rel-wpt

@emilio

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2017

⌛️ Trying commit 3de3a77 with merge dee3a2c...

bors-servo added a commit that referenced this pull request Mar 26, 2017

Auto merge of #16144 - emilio:number-calc, r=<try>
style: Make numbers keep track of whether they were specified as calc().

Fixes #15960

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16144)
<!-- Reviewable:end -->
@emilio

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

@MatsPalmgren mentioned in the bug linked above that he was aligning Firefox with Blink here for <number> and <integer>. It'd be nice to check what Edge does (I don't have a windows machine so I can't).

Also, note that this PR still doesn't fix Angle and Time units, for which we'd still return the simplified value (see CalcLengthOrPercentage::parse_time and CalcLengthOrPercentage::parse_angle).

Those would need also similar PRs, thought those would likely to be less intrusive because there are less properties abusing angles and time specified values.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2017

💔 Test failed - android

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

☔️ The latest upstream changes (presumably #16142) made this pull request unmergeable. Please resolve the merge conflicts.

emilio added some commits Mar 26, 2017

@emilio

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2017

@bors-servo r=heycam

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

📌 Commit b88c0fd has been approved by heycam

@emilio

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2017

@bors-servo p=1

  • There's other stuff waiting on this, like bug 1336769.
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

⌛️ Testing commit b88c0fd with merge d0a66d6...

bors-servo added a commit that referenced this pull request Mar 27, 2017

Auto merge of #16144 - emilio:number-calc, r=heycam
style: Make numbers keep track of whether they were specified as calc().

Fixes #15960

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16144)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 27, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

⌛️ Testing commit b88c0fd with merge b6bc492...

bors-servo added a commit that referenced this pull request Mar 27, 2017

Auto merge of #16144 - emilio:number-calc, r=heycam
style: Make numbers keep track of whether they were specified as calc().

Fixes #15960

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16144)
<!-- Reviewable:end -->
@stshine

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

I am not sure about this approach. according to https://drafts.csswg.org/css-values/#calc-type-checking:

A math expression has a resolved type, which is one of <length>, <frequency>, <angle>, <time>, <percentage>, <number>, or <integer>. The resolved type must be valid for where the expression is placed; otherwise, the expression is invalid. The resolved type of the expression is determined by the types of the values it contains. s are of type or .

So... if calc(1 + 20%) is a valid number, how can this pull request can handle it properly?

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: heycam
Pushing b6bc492 to master...

@bors-servo bors-servo merged commit b88c0fd into servo:master Mar 27, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@emilio

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2017

So... if calc(1 + 20%) is a valid number, how can this pull request can handle it properly?

calc(1 + 20%) is not a valid <number> token, is a <length> | <percentage>.

@emilio emilio deleted the emilio:number-calc branch Mar 27, 2017

@emilio

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2017

calc(1 + 20%) is not a valid token, is a | .

Concretely:

A percentage only has the type if in that context values are not used-value compatible with any other type. If percentages are not normally allowed in place of the calc(), then a calc() expression containing percentages is invalid in that context.

So something like opacity: calc(1 + 20%) is invalid. The main entry point for percentages in calc() expressions is LengthOrPercentage (there's only other place where we accept standalone percentages, which is in -moz-image-rect).

I'm not sure that answers your question, @stshine, but happy to think a bit more through it if you provide a concrete example :)

emilio added a commit to emilio/servo that referenced this pull request Mar 27, 2017

style: Re-introduce the -webkit- prefix for the order property.
I accidentally removed it while converting "order" to a predefined type
in servo#16144.

bors-servo added a commit that referenced this pull request Mar 27, 2017

Auto merge of #16151 - emilio:fixup-webkit-order, r=emilio
style: Re-introduce the -webkit- prefix for the order property.

I accidentally removed it while converting "order" to a predefined type
in #16144.
@stshine

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

Whoops, looks like you are right, I searched through css properties and yet to find a property that allows something like calc(1 + 10%) as a <number> :<

Sorry for the trouble :)

@emilio emilio referenced this pull request Mar 28, 2017

Merged

initial-letter property #16166

4 of 5 tasks complete

clementmiao added a commit to clementmiao/servo that referenced this pull request Apr 7, 2017

style: Re-introduce the -webkit- prefix for the order property.
I accidentally removed it while converting "order" to a predefined type
in servo#16144.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.