Skip to content

/ servo

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

Makes layout respect <textarea> rows and cols attributes #4380

Closed
wants to merge 2 commits into from
Closed

Makes layout respect <textarea> rows and cols attributes #4380

wants to merge 2 commits into from

Conversation

@mttr
Copy link
Contributor

mttr commented Dec 15, 2014

...with a bit of a caveat: sizing has the same problem as seen in #4378, and it is significantly more noticeable when using rows.

Fixes #4291
@highfive
Copy link

highfive commented Dec 15, 2014

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Dec 15, 2014

Critic review: https://critic.hoppipolla.co.uk/r/3505

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.
components/style/selector_matching.rs Outdated
name if *name == atom!("textarea") => {
match element.get_integer_attribute(ColsIntegerAttribute) {
Some(value) if value != 0 => {
// TODO ServoCharacterWidth uses the size math for <input type="text">, but

This comment has been minimized.

Sign in to view
Copy link
@pcwalton

pcwalton Dec 15, 2014

Contributor

Can you add your name here after TODO?
components/style/selector_matching.rs Outdated
}
match element.get_integer_attribute(RowsIntegerAttribute) {
Some(value) if value != 0 => {
// TODO This should take scrollbar size into consideration.

This comment has been minimized.

Sign in to view
Copy link
@pcwalton

pcwalton Dec 15, 2014

Contributor

Same here.
@pcwalton
Copy link
Contributor

pcwalton commented Dec 15, 2014

Looks good other than those couple of nits, thanks!
@mttr
Copy link
Contributor Author

mttr commented Dec 15, 2014

Got it!
@pcwalton
Copy link
Contributor

pcwalton commented Dec 15, 2014

Looks good, squash these into one commit and I'll r+
@mttr mttr force-pushed the mttr:textarea_rows_and_cols branch from a7a81e4 to 60ccd9e Dec 15, 2014
@mttr
Copy link
Contributor Author

mttr commented Dec 15, 2014

Squashed away.
bors-servo pushed a commit that referenced this pull request Dec 15, 2014
bors-servo
auto merge of #4380 : mttr/servo/textarea_rows_and_cols, r=pcwalton
Loading status checks…
ef90fdc 
...with a bit of a caveat: sizing has the same problem as seen in #4378, and it is _significantly_ more noticeable when using `rows`.

Fixes #4291
@mttr
Makes layout respect <textarea> cols attribute
2c7f607
@mttr mttr force-pushed the mttr:textarea_rows_and_cols branch from 60ccd9e to c521ba3 Dec 16, 2014
@mttr
Copy link
Contributor Author

mttr commented Dec 16, 2014

Rebased again since some conflicts came up.
@mttr
Makes layout respect <textarea> rows attribute
Loading status checks…
fc0748f 
review addresssing
@mttr mttr force-pushed the mttr:textarea_rows_and_cols branch from c521ba3 to fc0748f Dec 16, 2014
@jdm

This comment has been minimized.

Sign in to view
Copy link

jdm commented on fc0748f Dec 16, 2014

r+
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented on fc0748f Dec 16, 2014

saw approval from jdm
at mttr@fc0748f

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Dec 16, 2014

merging mttr/servo/textarea_rows_and_cols = fc0748f into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Dec 16, 2014

mttr/servo/textarea_rows_and_cols = fc0748f merged ok, testing candidate = fcaa45f

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Dec 16, 2014

all tests pass:
success: http://build.servo.org/builders/linux1/builds/324
success: http://build.servo.org/builders/linux2/builds/323
success: http://build.servo.org/builders/android/builds/391
success: http://build.servo.org/builders/mac1/builds/540
success: http://build.servo.org/builders/mac2/builds/541

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Dec 16, 2014

fast-forwarding master to auto = fcaa45f
bors-servo pushed a commit that referenced this pull request Dec 16, 2014
bors-servo
auto merge of #4380 : mttr/servo/textarea_rows_and_cols, r=jdm
Loading status checks…
fcaa45f 
...with a bit of a caveat: sizing has the same problem as seen in #4378, and it is _significantly_ more noticeable when using `rows`.

Fixes #4291
@bors-servo bors-servo closed this Dec 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Make <textarea> respect cols and rows HTML attributes
6 participants
@mttr @highfive @hoppipolla-critic-bot @pcwalton @bors-servo @jdm
You can’t perform that action at this time.