Summary: | [chromium] REGRESSION: Incorrect preferred width calculation for table cells | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Emil A Eklund <eae> | ||||||||
Component: | Layout and Rendering | Assignee: | Emil A Eklund <eae> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, jchaffraix, leviw | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Emil A Eklund
2012-09-24 16:47:15 PDT
Created attachment 165471 [details]
Patch
Downstream chromium bug: http://code.google.com/p/chromium/issues/detail?id=147496 Comment on attachment 165471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165471&action=review This LGTM other than coverage for Fixed tables. I'd feel better if Julien can give it a look as well before we land. > Source/WebCore/ChangeLog:12 > + In fixing bug 93911 r125694 caused a problem with the preferred > + width calculations for table cells with a fractional padding > + where the logic to ensure that the cell is wide enough for the > + content returns different results for cells with a fixed width > + and cells with an automatic width. This change will also affect FixedTableLayout when we specify a fractional width, as it will be ceiled. Probably worth mentioning in the ChangeLog, and it wouldn't be a bad idea to have a specific test for that just so we note the behavior and track if it changes. Comment on attachment 165471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165471&action=review This change is fine. I would r+ if we had the proper fixed table layout testing. >> Source/WebCore/ChangeLog:12 >> + and cells with an automatic width. > > This change will also affect FixedTableLayout when we specify a fractional width, as it will be ceiled. Probably worth mentioning in the ChangeLog, and it wouldn't be a bad idea to have a specific test for that just so we note the behavior and track if it changes. Spot on, this should prevent a similar issue with fixed table layout so it should be tested. Cool, I'll add a test for fixed layout and re-upload. Thanks for the quick review! Created attachment 165500 [details]
Patch
Comment on attachment 165500 [details] Patch Attachment 165500 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13992858 New failing tests: fast/sub-pixel/table-cells-have-stable-width.html Created attachment 165625 [details]
Patch
Comment on attachment 165625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165625&action=review Fix that changelog and we're good to go :) > Source/WebCore/ChangeLog:14 > + This also affects Trailed off there ;) (In reply to comment #9) > (From update of attachment 165625 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165625&action=review > > Fix that changelog and we're good to go :) > > > Source/WebCore/ChangeLog:14 > > + This also affects > > Trailed off there ;) Good catch, thanks! Committed r129529: <http://trac.webkit.org/changeset/129529> |