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

emilio · 2017-03-26T19:50:08Z

Fixes #15960

This change is

highfive · 2017-03-26T19:50:11Z

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 · 2017-03-26T19:50:53Z

r? @upsuper or @Manishearth or @heycam

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

emilio · 2017-03-26T19:51:44Z

@bors-servo try

bors-servo · 2017-03-26T19:51:49Z

⌛ Trying commit a14128a with merge db5e385...

style: Make numbers keep track of whether they were specified as calc(). Fixes #15960  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16144)

emilio · 2017-03-26T19:59:59Z

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 · 2017-03-26T20:02:31Z

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

emilio · 2017-03-26T20:49:13Z

@bors-servo try

bors-servo · 2017-03-26T20:49:16Z

⌛ Trying commit c381ae2 with merge 0435933...

style: Make numbers keep track of whether they were specified as calc(). Fixes #15960  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16144)

emilio · 2017-03-26T21:00:23Z

See https://bugzilla.mozilla.org/show_bug.cgi?id=1350739 for more fun.

bors-servo · 2017-03-26T21:15:45Z

💔 Test failed - linux-rel-wpt

emilio · 2017-03-26T22:28:42Z

@bors-servo try

bors-servo · 2017-03-26T22:28:46Z

⌛ Trying commit 3de3a77 with merge dee3a2c...

style: Make numbers keep track of whether they were specified as calc(). Fixes #15960  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16144)

emilio · 2017-03-26T22:32:15Z

@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 · 2017-03-26T22:35:59Z

💔 Test failed - android

bors-servo · 2017-03-27T00:22:10Z

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

emilio · 2017-03-27T10:50:00Z

@bors-servo r=heycam

bors-servo · 2017-03-27T10:50:01Z

📌 Commit b88c0fd has been approved by heycam

emilio · 2017-03-27T11:03:24Z

@bors-servo p=1

There's other stuff waiting on this, like bug 1336769.

bors-servo · 2017-03-27T11:26:53Z

⌛ Testing commit b88c0fd with merge d0a66d6...

style: Make numbers keep track of whether they were specified as calc(). Fixes #15960  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16144)

bors-servo · 2017-03-27T11:54:58Z

💔 Test failed - linux-rel-wpt

jdm · 2017-03-27T13:13:36Z

@bors-servo: retry
#14323

bors-servo · 2017-03-27T13:13:40Z

⌛ Testing commit b88c0fd with merge b6bc492...

style: Make numbers keep track of whether they were specified as calc(). Fixes #15960  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16144)

stshine · 2017-03-27T13:45:46Z

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 · 2017-03-27T13:57:28Z

☀️ 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...

emilio · 2017-03-27T14:37:55Z

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 · 2017-03-27T14:41:54Z

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 :)

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

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

stshine · 2017-03-27T18:42:44Z

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 :)

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

highfive assigned KiChjang Mar 26, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 26, 2017

highfive assigned upsuper and unassigned KiChjang Mar 26, 2017

emilio force-pushed the number-calc branch from ba1956e to c381ae2 Compare March 26, 2017 20:49

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-tests-failed The changes caused existing tests to fail. labels Mar 26, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Mar 26, 2017

highfive added S-needs-rebase There are merge conflict errors. and removed S-tests-failed The changes caused existing tests to fail. labels Mar 27, 2017

emilio added 2 commits March 27, 2017 03:56

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

c7ce2ff

style: Test for calc integer expressions.

3ab6a20

highfive removed the S-tests-failed The changes caused existing tests to fail. label Mar 27, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Mar 27, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Mar 27, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Mar 27, 2017

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

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 27, 2017

emilio deleted the number-calc branch March 27, 2017 14:38

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.

17f055c

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

emilio mentioned this pull request Mar 27, 2017

style: Re-introduce the -webkit- prefix for the order property. #16151

Merged

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

Auto merge of #16151 - emilio:fixup-webkit-order, r=emilio

185d31f

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

emilio mentioned this pull request Mar 28, 2017

initial-letter property #16166

Merged

5 tasks

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

style: Re-introduce the -webkit- prefix for the order property.

a186d00

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

emilio commented Mar 26, 2017 •

edited by larsbergstrom

Loading

highfive commented Mar 26, 2017

emilio commented Mar 26, 2017

emilio commented Mar 26, 2017

bors-servo commented Mar 26, 2017

emilio commented Mar 26, 2017

emilio commented Mar 26, 2017

emilio commented Mar 26, 2017

bors-servo commented Mar 26, 2017

emilio commented Mar 26, 2017

bors-servo commented Mar 26, 2017

emilio commented Mar 26, 2017

bors-servo commented Mar 26, 2017

emilio commented Mar 26, 2017

bors-servo commented Mar 26, 2017

bors-servo commented Mar 27, 2017

emilio commented Mar 27, 2017

bors-servo commented Mar 27, 2017

emilio commented Mar 27, 2017

bors-servo commented Mar 27, 2017

bors-servo commented Mar 27, 2017

jdm commented Mar 27, 2017

bors-servo commented Mar 27, 2017

stshine commented Mar 27, 2017

bors-servo commented Mar 27, 2017

emilio commented Mar 27, 2017

emilio commented Mar 27, 2017

stshine commented Mar 27, 2017

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

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

Conversation

emilio commented Mar 26, 2017 • edited by larsbergstrom Loading

highfive commented Mar 26, 2017

emilio commented Mar 26, 2017

emilio commented Mar 26, 2017

bors-servo commented Mar 26, 2017

emilio commented Mar 26, 2017

emilio commented Mar 26, 2017

emilio commented Mar 26, 2017

bors-servo commented Mar 26, 2017

emilio commented Mar 26, 2017

bors-servo commented Mar 26, 2017

emilio commented Mar 26, 2017

bors-servo commented Mar 26, 2017

emilio commented Mar 26, 2017

bors-servo commented Mar 26, 2017

bors-servo commented Mar 27, 2017

emilio commented Mar 27, 2017

bors-servo commented Mar 27, 2017

emilio commented Mar 27, 2017

bors-servo commented Mar 27, 2017

bors-servo commented Mar 27, 2017

jdm commented Mar 27, 2017

bors-servo commented Mar 27, 2017

stshine commented Mar 27, 2017

bors-servo commented Mar 27, 2017

emilio commented Mar 27, 2017

emilio commented Mar 27, 2017

stshine commented Mar 27, 2017

emilio commented Mar 26, 2017 •

edited by larsbergstrom

Loading