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

table_wrapper.rs: Fix for percentage widths #4598

Merged
merged 1 commit into from Jan 13, 2015
Merged

table_wrapper.rs: Fix for percentage widths #4598

merged 1 commit into from Jan 13, 2015

Conversation

@mttr
Copy link
Contributor

mttr commented Jan 9, 2015

Fixes #4421
@jdm
Copy link
Member

jdm commented Jan 10, 2015

Note: I know nothing about table layout and I didn't read the changes. Is this change covered by an existing spec?
@mttr
Copy link
Contributor Author

mttr commented Jan 10, 2015

I changed some of the existing code to match with the "Column and caption widths influence the final table width as follows: ..." part toward the end of this section of the CSS spec (the end result should be, as the spec describes here that "if the margins of a table are set to '0' and the width to 'auto', the table will not automatically size to fill its containing block. However, once the calculated value of 'width' for the table is found (using the algorithms given below or, when appropriate, some other UA dependent algorithm) then the other parts of section 10.3 do apply").

I'm not 100% confident about my changes since I can't claim to fully understand the layout code, but it seems to work (and I included a new reftest based on the tests in #4421).
components/layout/table_wrapper.rs Outdated
let available_inline_size = match self.block_flow.fragment.style().content_inline_size() {
LengthOrPercentageOrAuto::Auto => self.block_flow
.get_shrink_to_fit_inline_size(available_inline_size),
// XXX(mttr) This fixes #4421 without breaking our current reftests, but I'm

This comment has been minimized.

Sign in to view
Copy link
@pcwalton

pcwalton Jan 11, 2015

Contributor

nit: I prefer FIXME over XXX
@pcwalton
Copy link
Contributor

pcwalton commented Jan 11, 2015

Looks good modulo that one nit!
@mttr
table_wrapper.rs: Fix for percentage widths
Loading status checks…
182f1a0 
Fixes #4421
@pcwalton
Copy link
Contributor

pcwalton commented Jan 11, 2015

LGTM, squash the two into one and I'll r+. Thanks!
@mttr mttr force-pushed the mttr:table_percentage branch from 4fafbe8 to 182f1a0 Jan 11, 2015
@mttr
Copy link
Contributor Author

mttr commented Jan 11, 2015

Got it!
@pcwalton

This comment has been minimized.

Sign in to view
Copy link

pcwalton commented on 182f1a0 Jan 11, 2015

r+

This comment has been minimized.

Sign in to view
Copy link

jdm replied Jan 13, 2015

@bors: retry
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented on 182f1a0 Jan 11, 2015

saw approval from pcwalton
at mttr@182f1a0

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Jan 11, 2015

merging mttr/servo/table_percentage = 182f1a0 into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Jan 11, 2015

mttr/servo/table_percentage = 182f1a0 merged ok, testing candidate = 94e8079

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Jan 11, 2015

some tests failed:
failure: http://build.servo.org/builders/linux2/builds/513

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Jan 13, 2015

saw approval from pcwalton
at mttr@182f1a0

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Jan 13, 2015

merging mttr/servo/table_percentage = 182f1a0 into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Jan 13, 2015

mttr/servo/table_percentage = 182f1a0 merged ok, testing candidate = 62d1761

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Jan 13, 2015

all tests pass:
success: http://build.servo.org/builders/linux1/builds/526
success: http://build.servo.org/builders/linux2/builds/524
success: http://build.servo.org/builders/android/builds/562
success: http://build.servo.org/builders/mac1/builds/743
success: http://build.servo.org/builders/mac2/builds/743

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Jan 13, 2015

fast-forwarding master to auto = 62d1761
bors-servo pushed a commit that referenced this pull request Jan 11, 2015
bors-servo
auto merge of #4598 : mttr/servo/table_percentage, r=pcwalton
Loading status checks…
94e8079 
Fixes #4421
bors-servo pushed a commit that referenced this pull request Jan 13, 2015
bors-servo
auto merge of #4598 : mttr/servo/table_percentage, r=pcwalton
Loading status checks…
62d1761 
Fixes #4421
@bors-servo bors-servo closed this Jan 13, 2015
@bors-servo bors-servo merged commit 182f1a0 into servo:master Jan 13, 2015
1 check passed
1 check passed
default all tests passed
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.

> 100% width on table mishandled
4 participants
@mttr @jdm @pcwalton @bors-servo
You can’t perform that action at this time.