RESOLVED FIXED Bug 102802
REGRESSION (r128633): td changes size during re-layout of table although it shouldn't 
https://bugs.webkit.org/show_bug.cgi?id=102802
Summary REGRESSION (r128633): td changes size during re-layout of table although it s...
jochen
Reported 2012-11-20 04:54:25 PST
Created attachment 175192 [details] test case In the attached test case, there are essentially two table rows. The first has a 100px high empty td element in it, the other one has a cell with the word "test" in it. If you hover "test" cell (on a large enough window), the size of the 100px high empty td will change. Note that in the test case, this "placeholder" tag is actually important to reproduce. The test case is derived from a sharepoint page. In Chrome, the bug reproduces on M23, but not M22.
Attachments
test case (1.53 KB, text/html)
2012-11-20 04:54 PST, jochen 		no flags
Details
Patch (8.74 KB, patch)
2012-11-21 06:06 PST, Julian Pastarmov 		no flags
DetailsFormatted DiffDiff
reduced testcase (540 bytes, text/html)
2012-11-21 10:20 PST, Ojan Vafai 		no flags
Details
Patch (7.49 KB, patch)
2012-11-22 09:25 PST, Julian Pastarmov 		no flags
DetailsFormatted DiffDiff
Patch for landing (7.46 KB, patch)
2012-11-23 01:09 PST, Julian Pastarmov 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2012-11-20 12:33:16 PST
Regressed in <http://trac.webkit.org/changeset/128633>. Ojan, can you take a look?
Julian Pastarmov
Comment 2 2012-11-21 06:06:32 PST
Created attachment 175422 [details] Patch
Julian Pastarmov
Comment 3 2012-11-21 06:07:31 PST
Hey I uploaded the fix for that but I got some trouble creating a proper test for that. I have uploaded the frame for the test but it seems to not reproduce as it does in Chrome. I don't know why.
Mike West
Comment 4 2012-11-21 06:11:01 PST
Ojan, I worked a bit with Julian to get a test for this change, but I don't know enough about the effected code to be at all helpful. :) In short, the test works the way we'd expect it to when run manually in a browser, but fails in DumpRenderTree. It would be excellent if you could have a look, and suggest either a better test case for the bug Julian found, or suggest mechanisms for improving the test to actually test the bug. Thanks!
WebKit Review Bot
Comment 5 2012-11-21 06:56:25 PST
Comment on attachment 175422 [details] Patch Attachment 175422 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14945029 New failing tests: fast/dom/jumping-box-test.html
jochen
Comment 6 2012-11-21 07:01:04 PST
Comment on attachment 175422 [details] Patch I would recommend turning this into a reftest. Also, it should be possible to find a smaller test case. View in context: https://bugs.webkit.org/attachment.cgi?id=175422&action=review > LayoutTests/fast/dom/jumping-box-test.html:2 > + <head> please format the html file according the the webkit-style as well > LayoutTests/fast/dom/jumping-box-test.html:83 > + eventSender.mouseMoveTo(10,10); i don't think using the event sender is necessary, you could just change the css in the onload handler. Also, you should write the test such that it is possible to verify the result when you run it in a browser (i.e. include a textual description of what it does, and make sure you don't use objects like eventSender without first checking whether they exist)
Ojan Vafai
Comment 7 2012-11-21 09:57:24 PST
This regression only happens in quirks mode. Please mention that in the ChangeLog description.
Ojan Vafai
Comment 8 2012-11-21 10:20:29 PST
Created attachment 175475 [details] reduced testcase Does this solve your issue? It now no longer depends on a layout, so I'd be surprised if there were a difference between Chrome and DRT here. Also, if you can, please make the test case a check-layout.js test. See http://trac.webkit.org/browser/trunk/LayoutTests/css3/flexbox/percent-margins.html as an example. You can use data-offset-y to assert the location of the innermost TD.
Ojan Vafai
Comment 9 2012-11-21 10:24:15 PST
On tricky bit using check-layout.js here is that the position depends on the height of the window. I couldn't think of a way to change that. So, you'll just need some text in the test saying that the pages needs to be loaded with the window 600px tall for the test to pass (we run DRT at 800x600). An alternative would be to make this a reftest where you position an element absolutely using a percentage. Then it wouldn't depend on the window height. Not sure how tricky it'll be to write a reference for this test.
Julian Pastarmov
Comment 10 2012-11-22 09:25:56 PST
Created attachment 175688 [details] Patch
Julian Pastarmov
Comment 11 2012-11-22 09:28:26 PST
I managed to create a test case that failes without the patch and passes with it. I couldn't make it smaller and still failing without the patch. Also I moved the test case to the fast/table directory because it is mostly concerning nested tables.
Ojan Vafai
Comment 12 2012-11-22 11:45:43 PST
Comment on attachment 175688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175688&action=review Looks great. Please fix the doctype issue before committing. > LayoutTests/fast/table/nested-tables-with-div-offset.html:1 > +<!DOCTYPE xhtml> No need to include the doctype here. We only include a doctype when we're trying to get standards mode. Even then we only use the html doctype of <!DOCTYPE html>.
Julian Pastarmov
Comment 13 2012-11-23 01:09:22 PST
Created attachment 175748 [details] Patch for landing
Mike West
Comment 14 2012-11-23 01:42:07 PST
Comment on attachment 175748 [details] Patch for landing Thanks, Julian!
WebKit Review Bot
Comment 15 2012-11-23 03:00:30 PST
Comment on attachment 175748 [details] Patch for landing Clearing flags on attachment: 175748 Committed r135578: <http://trac.webkit.org/changeset/135578>
WebKit Review Bot
Comment 16 2012-11-23 03:00:35 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.