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

Stylo: Implement column-fill #13764

Closed
Manishearth opened this issue Oct 14, 2016 · 14 comments
Closed

Stylo: Implement column-fill #13764

Manishearth opened this issue Oct 14, 2016 · 14 comments

Comments

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Oct 14, 2016

Please go through the guide if you wish to hack on stylo.

column-fill is a keyword property (implementation guide) with the set of possible values being auto | balance. auto is the default.
The constant prefix is NS_STYLE_COLUMN_FILL, but you should not need to explicitly specify it.

File: https://github.com/servo/servo/blob/master/components/style/properties/longhand/column.mako.rs

Testcase:

<style>

.example {
  -moz-column-count: 4;
  column-count: 4;
  columns: 4;
  -moz-columns: 4;
  height: 105px;
  border: 1px solid rgba(0, 0, 0, 0.1);
  padding: 0 0.5em;
}

.example-balance {
  -webkit-column-fill: balance;
  -moz-column-fill: balance;
  column-fill: balance;
  margin-bottom: 1em;
}

.example-auto {
  -webkit-column-fill: auto;
  -moz-column-fill: auto;
  column-fill: auto;
}

body {
  font-size: 12px;
  font-family: 'Georgia', serif;
  font-weight: 400;
  line-height: 1.45;
  color: #333;
  background: #ecf0f1;
  padding: 1em;
}

code {
  font-family: "Courier New", monospace;
}

</style>
<code>column-fill: balance;</code>
<div class="example example-balance">
  <p> Lorem ipsum dolor sit amet, consectetur adipiscing elit. Phasellus libero libero, placerat at hendrerit nec, vulputate consectetur nisi. Morbi scelerisque lectus id sapien laoreet accumsan. Nunc eget tincidunt ligula, eget suscipit tellus. Sed iaculis nibh gravida faucibus porta. Sed lacinia tristique elementum. Etiam odio sem, dapibus eu tempus vel, consequat non turpis.</p>

</div>
<code>column-fill: auto;</code>
<div class="example example-auto">
  <p> Lorem ipsum dolor sit amet, consectetur adipiscing elit. Phasellus libero libero, placerat at hendrerit nec, vulputate consectetur nisi. Morbi scelerisque lectus id sapien laoreet accumsan. Nunc eget tincidunt ligula, eget suscipit tellus. Sed iaculis nibh gravida faucibus porta. Sed lacinia tristique elementum. Etiam odio sem, dapibus eu tempus vel, consequat non turpis.</p>
</div>

The testcase should look the same in Firefox and Stylo, with the columns having roughly the same amount of text in the first block and an imbalance in the second.

@highfive

This comment has been minimized.

Copy link

@highfive highfive commented Oct 14, 2016

Please make a comment here if you intend to work on this issue. Thank you!

@woop

This comment has been minimized.

Copy link

@woop woop commented Oct 14, 2016

I'd like to work on this issue please.

@Manishearth

This comment has been minimized.

Copy link
Member Author

@Manishearth Manishearth commented Oct 14, 2016

Cool! Let us know if you need help.

@wafflespeanut

This comment has been minimized.

Copy link
Member

@wafflespeanut wafflespeanut commented Oct 20, 2016

@willempienaar Any news on this?

@woop

This comment has been minimized.

Copy link

@woop woop commented Oct 20, 2016

@wafflespeanut No feedback yet. I have next week off so I am planning to finish it then.

@woop

This comment has been minimized.

Copy link

@woop woop commented Oct 29, 2016

Haven't gotten around to this one. If somebody wants to grab it, then it is available. Otherwise I will continue next week.

@KiChjang KiChjang removed the C-assigned label Oct 29, 2016
@KiChjang

This comment has been minimized.

Copy link
Member

@KiChjang KiChjang commented Oct 29, 2016

Let us know if you're working on this again.

@Haggus

This comment has been minimized.

Copy link
Contributor

@Haggus Haggus commented Oct 29, 2016

Do you mind if I hack on this?

@KiChjang

This comment has been minimized.

Copy link
Member

@KiChjang KiChjang commented Oct 29, 2016

@Haggus Go for it!

@KiChjang KiChjang added the C-assigned label Oct 29, 2016
@Haggus Haggus mentioned this issue Oct 29, 2016
0 of 5 tasks complete
@Haggus

This comment has been minimized.

Copy link
Contributor

@Haggus Haggus commented Oct 29, 2016

I went through style guide and added column-fill. Tested and compared it to Firefox. It's not "exactly" the same, but very similar. Let me know if there is more changes I need to do. Also, should the test also be added into the code?

bors-servo added a commit that referenced this issue Oct 29, 2016
Implement column-fill

Implemented column-fill from #13764

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/13978)
<!-- Reviewable:end -->
@Manishearth

This comment has been minimized.

Copy link
Member Author

@Manishearth Manishearth commented Oct 29, 2016

Do you have screenshots?

@Haggus

This comment has been minimized.

Copy link
Contributor

@Haggus Haggus commented Oct 29, 2016

Firefox:
column-fill-firefox

Stylo (after my changes):
column-fill-stylo

@Manishearth

This comment has been minimized.

Copy link
Member Author

@Manishearth Manishearth commented Oct 29, 2016

Yep, this is correct. The difference happens because of that extra line before the lorem ipsum in firefox (might be due to some other unimplemented property). That's ok.

@KiChjang

This comment has been minimized.

Copy link
Member

@KiChjang KiChjang commented Oct 29, 2016

Fixed in #13978.

@KiChjang KiChjang closed this Oct 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.