Skip to content

/ servo

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: Move mask-size outside of mako #19470

Closed
wants to merge 1 commit into from
Closed

style: Move mask-size outside of mako #19470

wants to merge 1 commit into from

Conversation

@CYBAI
Copy link
Collaborator

CYBAI commented Dec 3, 2017 •
edited by emilio

This is a sub-PR of #19015
r? emilio

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #19467
  • These changes do not require tests

This change is Reviewable
@highfive
Copy link

highfive commented Dec 3, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/background.mako.rs, components/style/properties/gecko.mako.rs, components/style/values/computed/background.rs, components/style/properties/longhand/svg.mako.rs, components/style/properties/helpers.mako.rs
  • @canaltinova: components/style/properties/longhand/background.mako.rs, components/style/properties/gecko.mako.rs, components/style/values/computed/background.rs, components/style/properties/longhand/svg.mako.rs, components/style/properties/helpers.mako.rs
  • @emilio: components/style/properties/longhand/background.mako.rs, components/style/properties/gecko.mako.rs, components/style/values/computed/background.rs, components/style/properties/longhand/svg.mako.rs, components/style/properties/helpers.mako.rs
@highfive
Copy link

highfive commented Dec 3, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@emilio
emilio reviewed Dec 3, 2017
View changes
Copy link
Member

emilio left a comment

Code looks good, but let's way for @BorisChiou to ensure that this can be removed or help testing it.
components/style/values/computed/background.rs
fn from_animated_value(animated: Self::AnimatedValue) -> Self {
use values::computed::{Length, Percentage};
let clamp_animated_value = |value: LengthOrPercentageOrAuto| -> LengthOrPercentageOrAuto {
match value {

This comment has been minimized.

Sign in to view
@emilio

emilio Dec 3, 2017

Member

So, as we discussed on IRC, this works, but this removes the clamping behavior on BackgroundSize... I just pushed this to try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7479f22e46b38c6103a46045e1bf83186927b18&selectedJob=149390573) and it looks green.

@BorisChiou, you added this code in ebedea5 (#17783).

Can we remove this? If not, could you write guidance on how to write a test for this?

This comment has been minimized.

Sign in to view
@BorisChiou

BorisChiou Dec 4, 2017 •
edited

Contributor

Unfortunately, Gecko doesn't test this [1]. I added this because we may use a negative cubic timing function [2] for animations/transitions. If you want to verify your patch, you could try to run this: https://jsfiddle.net/borischiou/w5yqs437/. If the console output is "0px 0px", it works. However, if you see something like "calc(-68.3px) calc(-68.3px)", please fix it because the size property should always be 0 or any positive number. :)

[1] https://searchfox.org/mozilla-central/rev/477ac066b565ae0eb3519875581a62dfb1430e98/layout/style/test/test_transitions_per_property.html#109-113
[2] cubic-bezier(0.25, -2, 0.75, 1)

This comment has been minimized.

Sign in to view
@BorisChiou

BorisChiou Dec 4, 2017

Contributor

And I guess you may get the negative value (i.e. unclamped value) if animation_value_type == "ComputedValue" for background-size. mask-size has the similar problem, I think. Therefore, It's better not to remove this.

This comment has been minimized.

Sign in to view
@CYBAI

CYBAI Dec 4, 2017

Author Collaborator

@BorisChiou After trying to access the jsfiddle link, I found it logged with 0px 0px in my local. However, it got an error of ERROR:script::dom::bindings::error: Error at https://jsfiddle.net/js/_dist-editor.js?update_24_11_2017_2:1:228763 Ao(...).getBoundingClientRect is not a function so I cannot see the jsfiddle UI.
Not sure if it means updated in this PR works?

This comment has been minimized.

Sign in to view
@BorisChiou

BorisChiou Dec 5, 2017

Contributor

Really? Oh, I used Firefox to verify this jsfiddle test. Sorry, I will try Servo browser again.

This comment has been minimized.

Sign in to view
@CYBAI

CYBAI Dec 6, 2017

Author Collaborator

Hmm... I got after 500ms: -68.7001px -68.7001px when I ran it locally second time.
I'll check it more detail tomorrow.

This comment has been minimized.

Sign in to view
@emilio

emilio Dec 9, 2017

Member

@BorisChiou mask-size was already bogus here, wasn't it? In particular, it was using animation_value_size="ComputedValue".

Could we get that stuff tested pretty please?

This comment has been minimized.

Sign in to view
@BorisChiou

BorisChiou Dec 11, 2017 •
edited

Contributor

Yes..we have to do that. We have to update the test cases in test_transitions_per_property.html for background-size and mask-size (so at least we test it on Firefox), and fix the problem of mask-size. I have to file a bug for it.

FIled Bug 1424671.

This comment has been minimized.

Sign in to view
@CYBAI

CYBAI Dec 11, 2017

Author Collaborator

@BorisChiou Thanks for the update! I think we can merge this PR and let's fix the issue in another PR?

cc @emilio

This comment has been minimized.

Sign in to view
@BorisChiou

BorisChiou Dec 11, 2017

Contributor

But the problem is: this PR makes background-size be buggy for animation. At least now we have to make sure background-size is correct for animation.
@CYBAI CYBAI force-pushed the CYBAI:mask-size-out-of-mako branch from f833fe4 to 41cbd40 Dec 9, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Jan 23, 2018

☔ The latest upstream changes (presumably #19838) made this pull request unmergeable. Please resolve the merge conflicts.
@CYBAI CYBAI force-pushed the CYBAI:mask-size-out-of-mako branch from 41cbd40 to 9be9ebd Jan 24, 2018
@CYBAI
style: Move mask-size outside of mako
Loading status checks…
10b4a19
@CYBAI CYBAI force-pushed the CYBAI:mask-size-out-of-mako branch from 9be9ebd to 10b4a19 Jan 25, 2018
@bors-servo
Copy link
Contributor

bors-servo commented Mar 7, 2018

☔ The latest upstream changes (presumably #20230) made this pull request unmergeable. Please resolve the merge conflicts.
@emilio
Copy link
Member

emilio commented Jun 1, 2018

I fixed this in https://bugzilla.mozilla.org/show_bug.cgi?id=1462829. Thanks @CYBAI!
@emilio emilio closed this Jun 1, 2018
@CYBAI
Copy link
Collaborator Author

CYBAI commented Jun 1, 2018

@emilio Cheers that this is fixed! Thanks a lot! 😆
@BorisChiou
Copy link
Contributor

BorisChiou commented Jun 1, 2018

Cool! Thanks~
@CYBAI CYBAI deleted the CYBAI:mask-size-out-of-mako branch Jun 2, 2018
@atouchet atouchet mentioned this pull request Jun 23, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

@BorisChiou BorisChiou

Assignees

@emilio emilio

Labels
S-awaiting-review S-needs-rebase
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Move mask-size outside of mako.
5 participants
@CYBAI @highfive @bors-servo @emilio @BorisChiou
You can’t perform that action at this time.