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

Implement gecko glue for clip property #15710

Merged
merged 1 commit into from Feb 24, 2017

Conversation

Projects
None yet
5 participants
@canaltinova
Copy link
Member

commented Feb 23, 2017

Implemented gecko glue for clip property.

Clip property looks slightly different in gecko. auto top and left values are preserved as auto in gecko but converted to 0 in servo. Gecko is setting NS_STYLE_CLIP_TOP_AUTO and NS_STYLE_CLIP_LEFT_AUTO flags with that information. But I tried this property in stylo build and it is working correctly like this. It looks like it doesn't change the outcome of the property.
Do we really need to set these flags?
Manishearth and bz said that auto and 0 values are same.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix Bug 1341728
  • These changes do not require tests because this is stylo side change.

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Feb 23, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/gecko.mako.rs, components/style/properties/longhand/effects.mako.rs
  • @emilio: components/style/properties/gecko.mako.rs, components/style/properties/longhand/effects.mako.rs
@highfive

This comment has been minimized.

Copy link

commented Feb 23, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!

@canaltinova canaltinova force-pushed the canaltinova:clip branch from a12c207 to 34aa9ee Feb 23, 2017

@emilio

emilio approved these changes Feb 23, 2017

Copy link
Member

left a comment

Looks good to me with that, thanks! :)

self.gecko.mClip.x = rect.left.0;
self.gecko.mClip.y = rect.top.0;

if rect.bottom.is_none() {

This comment has been minimized.

Copy link
@emilio

emilio Feb 23, 2017

Member

if let Some(bottom) = self.bottom, or match instead of `is_none() and then unwrap? here and below.

@canaltinova canaltinova force-pushed the canaltinova:clip branch from 34aa9ee to c3da55d Feb 24, 2017

@canaltinova

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2017

Fixed that. Thanks!

@emilio

This comment has been minimized.

Copy link
Member

commented Feb 24, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

📌 Commit c3da55d has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

⌛️ Testing commit c3da55d with merge 050d9d9...

bors-servo added a commit that referenced this pull request Feb 24, 2017

Auto merge of #15710 - canaltinova:clip, r=emilio
Implement gecko glue for clip property

<!-- Please describe your changes on the following line: -->
Implemented gecko glue for clip property.

Clip property looks slightly different in gecko. `auto` top and left values are preserved as auto in [gecko](http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/layout/style/nsRuleNode.cpp#10294,10316) but converted to 0 in [servo](https://dxr.mozilla.org/servo/rev/65624dbfc28442b58145215f524eb13aeb2cadf6/components/style/values/specified/mod.rs#942). Gecko is setting `NS_STYLE_CLIP_TOP_AUTO` and `NS_STYLE_CLIP_LEFT_AUTO` flags with that information. But I tried this property in stylo build and it is working correctly like this. It looks like it doesn't change the outcome of the property.
~Do we really need to set these flags?~
Manishearth and bz said that auto and 0 values are same.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [Bug 1341728](https://bugzilla.mozilla.org/show_bug.cgi?id=1341728)

<!-- Either: -->
- [X] These changes do not require tests because this is stylo side change.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/15710)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

@bors-servo bors-servo merged commit c3da55d into servo:master Feb 24, 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

@canaltinova canaltinova deleted the canaltinova:clip branch Feb 24, 2017

@bzbarsky

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

@canaltinova So what behavior does this implement? If the specified value is clip(auto, auto, auto, auto) what is the computed value?

Per spec, the computed value is:

'auto' if specified as 'auto', otherwise a rectangle with four values, each of which is 'auto' if specified as 'auto' and the computed length otherwise

which means the auto needs to be preserved in the computed value; it only becomes 0 in the used value.

This is observable using getComputedStyle.

@bzbarsky

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

Oh, servo messes this up for the specified value too! It's actually broken at the parser level. :(

@bzbarsky

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

I filed #15728 on the broken parsing.

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.