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

Serialization of specified length and time value should perserve their original unit #15346

Closed
upsuper opened this issue Feb 2, 2017 · 13 comments
Closed

Comments

@upsuper
Copy link
Member

@upsuper upsuper commented Feb 2, 2017

e.g. width: 1in should be serialized as 1in rather than 96px. The units should dropped during computing, not parsing.

(I'm not sure whether we have a spec for this behavior... but this behavior seems to match all major browsers. Probably worth a spec issue.)

@dpyro
Copy link
Contributor

@dpyro dpyro commented Feb 15, 2017

Ok, I will do this!

@jdm jdm added the C-assigned label Feb 15, 2017
@emilio
Copy link
Member

@emilio emilio commented Apr 3, 2017

Hi @dpyro, did you manage to make any progress on this?

@dpyro
Copy link
Contributor

@dpyro dpyro commented Apr 3, 2017

Nope @emilio, I am not able to do it right now. Please unassign me. Hopefully I will be able to contribute again later!

@jdm jdm removed the C-assigned label Apr 3, 2017
@emilio
Copy link
Member

@emilio emilio commented Apr 3, 2017

No worries, and thanks for replying!

@upsuper
Copy link
Member Author

@upsuper upsuper commented May 24, 2017

This is still a problem for time unit.

@emilio
Copy link
Member

@emilio emilio commented May 24, 2017

@luisbg, you mentioned you were interested on stylo, would you like to take this one?

@luisbg
Copy link
Contributor

@luisbg luisbg commented May 26, 2017

@emilio sure! I am currently traveling for work but I can start looking into this issue on Sunday.

Any advice on how to reproduce this bug?

@upsuper
Copy link
Member Author

@upsuper upsuper commented May 26, 2017

<!DOCTYPE html>
<style>
div { animation-delay: 1ms; }
</style>
<script>
alert(document.styleSheets[0].cssRules[0].style.animationDelay);
</script>

Other browsers show "1ms", while Stylo and Servo would show "0.001s".

@luisbg
Copy link
Contributor

@luisbg luisbg commented May 26, 2017

@upsuper I just reproduced it, thanks for sharing. Will investigate soon.

@upsuper
Copy link
Member Author

@upsuper upsuper commented May 26, 2017

I think what you basically need to do is to extend values::specified::Time to include the original unit, rather than converting to seconds directly.

@canova
Copy link
Member

@canova canova commented Jun 21, 2017

@luisbg Hi! Have you made any progress here? Is there anything that we can help?

@luisbg
Copy link
Contributor

@luisbg luisbg commented Jun 21, 2017

@canaltinova Hi! I got very busy at work. But I think I will have time this weekend to play with this code and figure out a solution.

Apologies for the delay. Give me a few more days and if not somebody else can take it 😄

Thanks for offering to help

@canova
Copy link
Member

@canova canova commented Jun 21, 2017

@luisbg No problem, thanks for letting us know. You can ping me or probably other team members on IRC if you need to ask anything!

bors-servo added a commit that referenced this issue Aug 4, 2017
Preserve unit in specified time value

This fixes #15346.

<!-- 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/17970)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants
You can’t perform that action at this time.