Bug 102802

Summary:

REGRESSION (r128633): td changes size during re-layout of table although it shouldn't

Product:

WebKit

Reporter:

jochen

Component:

Tables

Assignee:

Julian Pastarmov <pastarmovj>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, dglazkov, eric, jchaffraix, mitz, mkwst, ojan, pastarmovj, tony, webkit.review.bot

Priority:

Keywords:

Regression

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
test case	none
Patch	none
reduced testcase	none
Patch	none
Patch for landing	none

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	Details Formatted Diff Diff
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	Details Formatted Diff Diff
Patch for landing (7.46 KB, patch) 2012-11-23 01:09 PST, Julian Pastarmov	no flags	Details Formatted Diff Diff
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.