RESOLVED FIXED 88116
Change overrideSizes to be content-box instead of border-box
https://bugs.webkit.org/show_bug.cgi?id=88116
Summary Change overrideSizes to be content-box instead of border-box
Ojan Vafai
Reported 2012-06-01 11:53:34 PDT
Change overrideSizes to be content-box instead of border-box
Attachments
Patch (169.85 KB, patch)
2012-06-01 11:59 PDT, Ojan Vafai
no flags
Patch (173.55 KB, patch)
2012-06-01 15:55 PDT, Ojan Vafai
tony: review+
Ojan Vafai
Comment 1 2012-06-01 11:59:30 PDT
Tony Chang
Comment 2 2012-06-01 12:18:22 PDT
Comment on attachment 145349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145349&action=review > Source/WebCore/rendering/RenderBox.cpp:2289 > - return overrideHeight() - borderAndPaddingLogicalWidth(); > + return overrideLogicalContentHeight(); How did this work before? We're subtracting a width from a height? > LayoutTests/ChangeLog:9 > + Change overrideSizes to be content-box instead of border-box > + https://bugs.webkit.org/show_bug.cgi?id=88116 > + > + Reviewed by NOBODY (OOPS!). > + > + * platform/chromium-linux/tables/mozilla/bugs/bug131020-expected.png: > + * platform/chromium-win/tables/mozilla/bugs/bug131020-expected.txt: A sentence about how the test is changing would be helpful. E.g., "We were double counting the border in the height, but now we don't." or something. Should we add a test with padding?
Julien Chaffraix
Comment 3 2012-06-01 12:25:16 PDT
Comment on attachment 145349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145349&action=review > Source/WebCore/rendering/RenderTableCell.cpp:237 > void RenderTableCell::setOverrideHeightFromRowHeight(LayoutUnit rowHeight) While you are renaming all the override functions to include "logical", this one should be renamed too. >> LayoutTests/ChangeLog:9 >> + * platform/chromium-linux/tables/mozilla/bugs/bug131020-expected.png: >> + * platform/chromium-win/tables/mozilla/bugs/bug131020-expected.txt: > > A sentence about how the test is changing would be helpful. E.g., "We were double counting the border in the height, but now we don't." or something. Should we add a test with padding? I agree with adding a sentence (especially since it is a nice progression). The test needs to be rebaselined on other platforms too. I hope you will add the missing test_expectations.txt / Skipped entries before landing.
Ojan Vafai
Comment 4 2012-06-01 14:28:04 PDT
Comment on attachment 145349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145349&action=review >> Source/WebCore/rendering/RenderBox.cpp:2289 >> + return overrideLogicalContentHeight(); > > How did this work before? We're subtracting a width from a height? We were just doing the wrong thing. It happens that we have poor test coverage of the combination of setting overrideHeight on table cells and having a different borderAndPaddingWidth than borderAndPaddingHeight. Sigh, I suppose I should figure out how to write a test for this. >>> LayoutTests/ChangeLog:9 >>> + * platform/chromium-win/tables/mozilla/bugs/bug131020-expected.txt: >> >> A sentence about how the test is changing would be helpful. E.g., "We were double counting the border in the height, but now we don't." or something. Should we add a test with padding? > > I agree with adding a sentence (especially since it is a nice progression). The test needs to be rebaselined on other platforms too. I hope you will add the missing test_expectations.txt / Skipped entries before landing. Will add the description and add it to test_expectations.txt, but adding it to Skipped is not very useful since I need it to run on the bots to get the new expected results, no?
Julien Chaffraix
Comment 5 2012-06-01 15:35:44 PDT
Comment on attachment 145349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145349&action=review >>>> LayoutTests/ChangeLog:9 >>>> + * platform/chromium-win/tables/mozilla/bugs/bug131020-expected.txt: >>> >>> A sentence about how the test is changing would be helpful. E.g., "We were double counting the border in the height, but now we don't." or something. Should we add a test with padding? >> >> I agree with adding a sentence (especially since it is a nice progression). The test needs to be rebaselined on other platforms too. I hope you will add the missing test_expectations.txt / Skipped entries before landing. > > Will add the description and add it to test_expectations.txt, but adding it to Skipped is not very useful since I need it to run on the bots to get the new expected results, no? If you intent to do the rebaselining on the platform then yes you shouldn't touch the Skipped file. That said some maintainers prefer people to use Skipped files over test_expectations.txt which is why I mentioned the 2 files. It's better to disable the test on those platforms (following the maintainers' policy) than to turn the bot red.
Ojan Vafai
Comment 6 2012-06-01 15:55:38 PDT
Tony Chang
Comment 7 2012-06-01 16:43:27 PDT
Comment on attachment 145394 [details] Patch Might want to give Julien a chance to take a look too before landing.
Julien Chaffraix
Comment 8 2012-06-01 16:54:09 PDT
Comment on attachment 145394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145394&action=review The change is fine though I wish there was a way to differentiate between border-box and content-box in our code to avoid such mistakes. > LayoutTests/fast/table/padding-height-and-override-height.html:12 > +// Firefox 12 has the same crazy behavior. Is this a bug or are tables just crazy? You may be calling for the craziness yourself by using an auto-table layout layout in quirks mode. If you want less craziness, the way to go is: doctype + table-layout: fixed on your <table>.
Ojan Vafai
Comment 9 2012-06-05 11:24:42 PDT
(In reply to comment #8) > (From update of attachment 145394 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145394&action=review > > The change is fine though I wish there was a way to differentiate between border-box and content-box in our code to avoid such mistakes. Only thing I can think of to improve this would be to get rid of height/width accessors entirely. It makes the code more verbose, but it's more clear what's going on.
Ojan Vafai
Comment 10 2012-06-05 11:32:24 PDT
Julien Chaffraix
Comment 11 2012-06-05 11:36:35 PDT
> Only thing I can think of to improve this would be to get rid of height/width accessors entirely. It makes the code more verbose, but it's more clear what's going on. That's one way or at least a short term solution. The other would be to stop using plain LayoutRect and have 2 dedicated wrapper classes that avoids being able to convert implicitly between content and border boxes.
Note You need to log in before you can comment on or make changes to this bug.