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 keyword values in border-width #13869

Closed
Manishearth opened this issue Oct 21, 2016 · 8 comments
Closed

Implement keyword values in border-width #13869

Manishearth opened this issue Oct 21, 2016 · 8 comments

Comments

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Oct 21, 2016

Please see the property hacking guide first. Familiarity with Rust required. This is not implementing a new property, just extending an existing one.

For border-foo-width (e.g. border-top-width), we currently support length values. However, this struct supports keywords too.

We currently parse it as <length>, but the full syntax is <length> | thin | medium | thick. Now, the keyword values map to integer lengths (Firefox maps them to 1px, 3px and 5px respectively, you can use Length::Absolute(1).to_computed_value(context) to get these in Au). So you will have to change the SpecifiedValue struct so that it can store thin/medium/thick keyword values (as well as lengths), and change to_computed_value so that it replaces the keyword values with the stuff specified.

You should be able to test this in Servo directly, just create a div and give it different keyword borders.

@highfive
Copy link

@highfive highfive commented Oct 21, 2016

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

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Oct 21, 2016

@heycam Gecko stores the constants 1/3/5 in the pres context. Should we be storing them in the computed value context (and fetching them from Gecko in stylo mode)? For this bug I'm just hardcoding them, but we might eventually want to do something else.

@aethanyc
Copy link
Contributor

@aethanyc aethanyc commented Oct 21, 2016

I would like to work on this.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Oct 21, 2016

Great! Let us know if you have questions!

@heycam
Copy link
Member

@heycam heycam commented Oct 21, 2016

Hard coding is fine for now. For Gecko, we probably should just change them to be constants defined somewhere rather than exposed through the nsPresContext.

@aethanyc
Copy link
Contributor

@aethanyc aethanyc commented Oct 21, 2016

Any bug filed on gecko for moving those border-width constants from nsPresContext to elsewhere?

@heycam
Copy link
Member

@heycam heycam commented Oct 21, 2016

There isn't AFAIK. Please feel free to file one!

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Oct 22, 2016

aethanyc added a commit to aethanyc/servo that referenced this issue Oct 24, 2016
Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed servo#13869.
@aethanyc aethanyc mentioned this issue Oct 24, 2016
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Oct 24, 2016
Use enum BorderWidth as SpecifiedValue

Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed #13869.

---
<!-- 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
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes fix #13869 (github issue number if applicable).

<!-- Either: -->
- [x] 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/13908)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 25, 2016
Use enum BorderWidth as SpecifiedValue

Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed #13869.

---
<!-- 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
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes fix #13869 (github issue number if applicable).

<!-- Either: -->
- [x] 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/13908)
<!-- Reviewable:end -->
aethanyc added a commit to aethanyc/servo that referenced this issue Oct 25, 2016
Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed servo#13869.
bors-servo added a commit that referenced this issue Oct 25, 2016
Use enum BorderWidth as SpecifiedValue

Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed #13869.

---
<!-- 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
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes fix #13869 (github issue number if applicable).

<!-- Either: -->
- [x] 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/13908)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 25, 2016
Use enum BorderWidth as SpecifiedValue

Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed #13869.

---
<!-- 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
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes fix #13869 (github issue number if applicable).

<!-- Either: -->
- [x] 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/13908)
<!-- Reviewable:end -->
aethanyc added a commit to aethanyc/servo that referenced this issue Oct 26, 2016
Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed servo#13869.
aethanyc added a commit to aethanyc/servo that referenced this issue Oct 26, 2016
Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed servo#13869.
bors-servo added a commit that referenced this issue Oct 27, 2016
Use enum BorderWidth as SpecifiedValue

Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed #13869.

---
<!-- 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 #13869 (github issue number if applicable).

<!-- Either: -->
- [x] 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/13908)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 27, 2016
Use enum BorderWidth as SpecifiedValue

Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed #13869.

---
<!-- 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 #13869 (github issue number if applicable).

<!-- Either: -->
- [x] 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/13908)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 27, 2016
Use enum BorderWidth as SpecifiedValue

Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed #13869.

---
<!-- 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 #13869 (github issue number if applicable).

<!-- Either: -->
- [x] 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/13908)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 29, 2016
Use enum BorderWidth as SpecifiedValue

Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed #13869.

---
<!-- 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 #13869 (github issue number if applicable).

<!-- Either: -->
- [x] 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/13908)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 29, 2016
Use enum BorderWidth as SpecifiedValue

Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed #13869.

---
<!-- 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 #13869 (github issue number if applicable).

<!-- Either: -->
- [x] 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/13908)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 30, 2016
Use enum BorderWidth as SpecifiedValue

Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in `specified` module.

Fixed #13869.

---
<!-- 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 #13869 (github issue number if applicable).

<!-- Either: -->
- [x] 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/13908)
<!-- 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.

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