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

Jump to bottom

Move inline-block styling to user-agent and add button #8070

Merged
merged 3 commits into from Oct 25, 2015
Merged

Move inline-block styling to user-agent and add button #8070

merged 3 commits into from Oct 25, 2015

Conversation

badboy
Copy link
Contributor

@badboy badboy commented Oct 18, 2015

First Servo PR for me. \o/

Fixes #8064

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 18, 2015
@highfive
Copy link

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

@pcwalton
Copy link
Contributor

I guess this doesn't really affect any rendering, does it? Would be nice to add a reftest if it does.

@jdm
Copy link
Member

jdm commented Oct 18, 2015

It does affect things like <button style="width: 200px">foo</button>, which doesn't work without this change.

@badboy
Copy link
Contributor Author

badboy commented Oct 18, 2015

I can check out how the tests work and add one, just give me 1-2 days

@jdm
Copy link
Member

jdm commented Oct 19, 2015

A reftest that compares <button style="width: 200px">foo</button> against <input type="button" style="width: 200px" value="foo"> should do the trick.

@badboy
Copy link
Contributor Author

badboy commented Oct 19, 2015

Adding a reftest was surprisingly easy.

@frewsxcv
Copy link
Contributor

@badboy Make sure the entry is alphabetized in the basic.list file

https://travis-ci.org/servo/servo/jobs/86299032

@jdm
Copy link
Member

jdm commented Oct 20, 2015

Sorry, there's a moratorium on adding new entries to basic.list. New reftests should be written as part of the WPT harness instead, in tests/wpt/mozilla/tests/css/.

@badboy
Copy link
Contributor Author

badboy commented Oct 20, 2015

Oops, okay

@badboy
Copy link
Contributor Author

badboy commented Oct 20, 2015

Force-pushed the new test now. I hope I got it right this time (I still couldn't run the test locally though)

@badboy
Copy link
Contributor Author

badboy commented Oct 20, 2015

I now also updated the test manifest and ran the test locally.

For reference:

# updating
./mach test-wpt --include _mozilla/css --manifest-update
# running the single test
./mach test-wpt --include _mozilla/css/button_css_width.html

@jdm jdm self-assigned this Oct 25, 2015
@jdm
Copy link
Member

jdm commented Oct 25, 2015

Sorry for letting this slide. If you rebase it, I'll merge :)

@jdm jdm added S-needs-rebase There are merge conflict errors. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 25, 2015
@badboy
Copy link
Contributor Author

badboy commented Oct 25, 2015

Happy to do that. Just one thing: How to correctly update the manifest? Is the above correct? When I tried to rebase yesterday and then regenerate it with the above command it changed more than just adding the new test, so I'm unsure whether this is correct or not.

@jdm
Copy link
Member

jdm commented Oct 25, 2015

Yeah, it appears the manifest is not quite correct on master at the moment. If possible, just surgically add the changes that directly apply to your test to the manifest for now.

@badboy
Copy link
Contributor Author

badboy commented Oct 25, 2015

surgery it is then

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 25, 2015
@badboy
Copy link
Contributor Author

badboy commented Oct 25, 2015

Rebased on top of latest master & pushed. :)

@frewsxcv frewsxcv removed the S-needs-rebase There are merge conflict errors. label Oct 25, 2015
@jdm
Copy link
Member

jdm commented Oct 25, 2015

@bors-servo: r+
Thanks!

@bors-servo
Copy link
Contributor

📌 Commit 4153291 has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit 4153291 with merge 41df37c...

@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 Oct 25, 2015
bors-servo pushed a commit that referenced this pull request Oct 25, 2015
Auto merge of #8070 - badboy:inline-block-for-button, r=jdm
41df37c 
Move inline-block styling to user-agent and add button

First Servo PR for me. \o/

Fixes #8064

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8070)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit 4153291 into servo:master Oct 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@badboy @highfive @pcwalton @jdm @frewsxcv @bors-servo