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

Fix wrong calculation of inline element's block size #12930

Merged
merged 1 commit into from Aug 22, 2016

Conversation

canova
Copy link
Contributor

@canova canova commented Aug 18, 2016

Border, padding and margin properties' top and bottom values of inline elements were affecting the element's height. It shouldn't affect it normally. Fixed it and added a test.


  • There are tests for these changes

This change is Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 18, 2016
@emilio
Copy link
Member

emilio commented Aug 18, 2016

@bors-servo: try

@bors-servo
Copy link
Contributor

⌛ Trying commit f8a42d5 with merge f998677...

bors-servo pushed a commit that referenced this pull request Aug 18, 2016
Fix wrong calculation of inline element's block size

Border, padding and margin properties' top and bottom values of inline elements were affecting the element's height. It shouldn't affect it normally. Fixed it and added a test.

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

<!-- Either: -->
- [X] There are tests for these changes

<!-- 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/12930)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Aug 18, 2016
@highfive
Copy link

  ▶ PASS [expected FAIL] /css21_dev/html4/border-padding-bleed-001.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/border-padding-bleed-002.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/border-padding-bleed-003.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/border-width-applies-to-008.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/c5506-ipadn-t-000.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/c5506-ipadn-t-002.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/c5508-ipadn-b-000.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/inline-formatting-context-022.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/inline-formatting-context-023.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/inline-non-replaced-height-003.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/inline-non-replaced-height-002.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/margin-bottom-applies-to-008.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/margin-top-applies-to-008.htm

@emilio
Copy link
Member

emilio commented Aug 18, 2016

r=me with those test expectations changed, great catch! :)

@canova
Copy link
Contributor Author

canova commented Aug 18, 2016

Thanks :) I thought I ran the tests but obviously I didn't. So probably I should remove my additional test, right?

@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 18, 2016
@canova
Copy link
Contributor Author

canova commented Aug 18, 2016

Updated the expectations but still I don't know what to do with additional test :)

@emilio
Copy link
Member

emilio commented Aug 18, 2016

Tests don't hurt :-)

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit c39002f has been approved by emilio

@highfive highfive assigned emilio and unassigned metajack Aug 18, 2016
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 18, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit c39002f with merge c383623...

bors-servo pushed a commit that referenced this pull request Aug 20, 2016
Fix wrong calculation of inline element's block size

Border, padding and margin properties' top and bottom values of inline elements were affecting the element's height. It shouldn't affect it normally. Fixed it and added a test.

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

<!-- Either: -->
- [X] There are tests for these changes

<!-- 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/12930)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 20, 2016
@highfive
Copy link

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/reference/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728

@emilio
Copy link
Member

emilio commented Aug 20, 2016

@bors-servo
Copy link
Contributor

⌛ Testing commit c39002f with merge d11d335...

bors-servo pushed a commit that referenced this pull request Aug 20, 2016
Fix wrong calculation of inline element's block size

Border, padding and margin properties' top and bottom values of inline elements were affecting the element's height. It shouldn't affect it normally. Fixed it and added a test.

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

<!-- Either: -->
- [X] There are tests for these changes

<!-- 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/12930)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 20, 2016
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-css

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 22, 2016
@highfive
Copy link

  ▶ PASS [expected FAIL] /css21_dev/html4/c5510-ipadn-000.htm

@emilio
Copy link
Member

emilio commented Aug 22, 2016

Ok, this is sort of annoying, can you mark that test as disabled pointing to #12960? I'll note that in the issue, Thanks!

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 22, 2016
@emilio
Copy link
Member

emilio commented Aug 22, 2016

@bors-servo: r+

@canaltinova realised that #12960 is really an OS-X only pass it seems :)

Thanks once again for the fix, and sorry for the annoyance of the platform-specific test results! :/

@bors-servo
Copy link
Contributor

📌 Commit a14f6cb has been approved by emilio

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 22, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit a14f6cb with merge 078dc53...

bors-servo pushed a commit that referenced this pull request Aug 22, 2016
Fix wrong calculation of inline element's block size

Border, padding and margin properties' top and bottom values of inline elements were affecting the element's height. It shouldn't affect it normally. Fixed it and added a test.

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

<!-- Either: -->
- [X] There are tests for these changes

<!-- 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/12930)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-wpt

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 22, 2016
@highfive
Copy link

  ▶ TIMEOUT [expected PASS] /_mozilla/css/iframe/hide_layers2.html

@KiChjang
Copy link
Contributor

@bors-servo
Copy link
Contributor

⌛ Testing commit a14f6cb with merge 91cee66...

@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 22, 2016
bors-servo pushed a commit that referenced this pull request Aug 22, 2016
Fix wrong calculation of inline element's block size

Border, padding and margin properties' top and bottom values of inline elements were affecting the element's height. It shouldn't affect it normally. Fixed it and added a test.

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

<!-- Either: -->
- [X] There are tests for these changes

<!-- 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/12930)
<!-- Reviewable:end -->
@highfive highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 22, 2016
@bors-servo
Copy link
Contributor

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

@bors-servo bors-servo merged commit a14f6cb into servo:master Aug 22, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 22, 2016
@canova canova deleted the border-bottom branch November 4, 2016 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hover in servo-starters moves the text below it
6 participants