Bug 37244 - thead in table without tbody causes table height doubling
Summary: thead in table without tbody causes table height doubling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2010-04-07 18:27 PDT by Elliott Sprehn
Modified: 2012-08-02 10:25 PDT (History)
7 users (show)

See Also:


Attachments
Height doubling test covering font sizes and heights (1.71 KB, text/html)
2010-04-07 18:37 PDT, Elliott Sprehn
no flags Details
Proposed fix: An empty table has no section, not no tbody. (5.89 KB, patch)
2012-01-31 08:07 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Fixed another issue uncovered by the test case. (41.59 KB, patch)
2012-01-31 12:30 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Rebased patch. (41.70 KB, patch)
2012-02-01 09:37 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (41.82 KB, patch)
2012-02-03 14:47 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (36.66 KB, patch)
2012-02-10 09:36 PST, Julien Chaffraix
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2010-04-07 18:27:16 PDT
When thead is used in a table without a tbody the height of the table will be doubled.

Particularly weird is that if the height is below a certain amount the table height is not doubled. For instance if height: 23px is used the problem does not appear, but if height: 24px it does. The specific height that causes it to first appear is different depending on the page.

Adding a tbody to the table fixes the problem and it renders as expected.

Using tbody { display: table-row-group; } fixes this problem, though I'm not sure if there's other side effects of that change.
Comment 1 Elliott Sprehn 2010-04-07 18:37:30 PDT
Created attachment 52818 [details]
Height doubling test covering font sizes and heights

Apparently the bug relates to the font size of the page. Depending on the font size, the height necessary to cause the height doubling changes.
Comment 2 Elliott Sprehn 2011-05-10 01:01:53 PDT
This is still reproducing on the latest nightly a year later. Could someone comment on what's up with the table layout algorithm?

Version 5.0.5 (6533.21.1, r86072)
Comment 3 Julien Chaffraix 2012-01-31 08:07:21 PST
Created attachment 124734 [details]
Proposed fix: An empty table has no section, not no tbody.
Comment 4 WebKit Review Bot 2012-01-31 08:09:23 PST
Attachment 124734 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 WebKit Review Bot 2012-01-31 08:41:35 PST
Comment on attachment 124734 [details]
Proposed fix: An empty table has no section, not no tbody.

Attachment 124734 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11378035

New failing tests:
fast/table/double-height-table-no-tbody.html
Comment 6 Julien Chaffraix 2012-01-31 12:30:14 PST
Created attachment 124796 [details]
Fixed another issue uncovered by the test case.
Comment 7 Julien Chaffraix 2012-02-01 09:37:41 PST
Created attachment 124962 [details]
Rebased patch.
Comment 8 WebKit Review Bot 2012-02-01 09:41:06 PST
Attachment 124962 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 826, in process
    checker.check(lines)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 119, in check
    overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 96, in check_test_expectations
    is_lint_mode=True, overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py", line 726, in __init__
    self._add_skipped_tests(port.skipped_tests(tests))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 376, in skipped_tests
    return self.skipped_layout_tests(test_list)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 372, in skipped_layout_tests
    tests_to_skip.update(self._skipped_tests_for_unsupported_features(test_list))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 319, in _skipped_tests_for_unsupported_features
    if self._has_test_in_directories(self._missing_feature_to_skipped_tests().values(), test_list):
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 311, in _has_test_in_directories
    for directory, test in itertools.product(directories, test_list):
TypeError: 'NoneType' object is not iterable


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Julien Chaffraix 2012-02-03 14:41:56 PST
Comment on attachment 124962 [details]
Rebased patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=124962&action=review

> Source/WebCore/rendering/RenderTable.cpp:382
>              // FIXME: Distribute extra height between all table body sections instead of giving it all to the first one.

This comment is stale now and does not match FF or Opera: we need to spread the extra logical height on all the sections. I will update it before landing.
Comment 10 Julien Chaffraix 2012-02-03 14:47:15 PST
Created attachment 125420 [details]
Patch for landing
Comment 11 Adam Barth 2012-02-03 23:54:39 PST
Comment on attachment 125420 [details]
Patch for landing

This patch is crashing the commit-queue for reasons I don't understand.
Comment 12 Julien Chaffraix 2012-02-04 13:41:48 PST
(In reply to comment #11)
> (From update of attachment 125420 [details])
> This patch is crashing the commit-queue for reasons I don't understand.

Could it be related to comment #8? Should I file a bug about it or is the patch not passing the cq and I should investigate instead of landing it manually?
Comment 13 Adam Barth 2012-02-04 14:23:11 PST
I'd just land it manually.  I'll investigate the cq issue.
Comment 14 Julien Chaffraix 2012-02-10 09:36:03 PST
Created attachment 126526 [details]
Patch for landing
Comment 15 WebKit Review Bot 2012-02-10 14:24:36 PST
Comment on attachment 126526 [details]
Patch for landing

Rejecting attachment 126526 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ebKit/chromium/third_party/yasm/source/patched-yasm --revision 73761 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
47>At revision 73761.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/11484808
Comment 16 Julien Chaffraix 2012-02-16 12:50:23 PST
Committed r107971: <http://trac.webkit.org/changeset/107971>
Comment 17 Julien Chaffraix 2012-02-17 10:06:05 PST
More Chromium rebaseline in http://trac.webkit.org/changeset/108088.
Comment 18 Csaba Osztrogonác 2012-02-18 05:05:43 PST
Qt rebaseline landed in http://trac.webkit.org/changeset/108160
Comment 19 Alexander Pavlov (apavlov) 2012-08-02 01:20:23 PDT
@jchaffraix: The related tests have been consistently failing lately, which has gone unnoticed due to the stale

BUGWK37244 : tables/mozilla/bugs/bug27038-1.html = IMAGE+TEXT
BUGWK37244 : tables/mozilla/bugs/bug27038-2.html = IMAGE+TEXT

found in TestExpectations for Chromium, which were not removed when the bug fix went in (see http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=tables%2Fmozilla%2Fbugs%2Fbug27038-1.html%2Ctables%2Fmozilla%2Fbugs%2Fbug27038-2.html). Should I file a new bug on this and close this one?
Comment 20 Julien Chaffraix 2012-08-02 10:15:25 PDT
(In reply to comment #19)
> @jchaffraix: The related tests have been consistently failing lately, which has gone unnoticed due to the stale
> 
> BUGWK37244 : tables/mozilla/bugs/bug27038-1.html = IMAGE+TEXT
> BUGWK37244 : tables/mozilla/bugs/bug27038-2.html = IMAGE+TEXT
> 
> found in TestExpectations for Chromium, which were not removed when the bug fix went in (see http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=tables%2Fmozilla%2Fbugs%2Fbug27038-1.html%2Ctables%2Fmozilla%2Fbugs%2Fbug27038-2.html). Should I file a new bug on this and close this one?

That's unfortunate, please file a follow-up bug and assign it to me. Thanks, Alexander.
Comment 21 Alexander Pavlov (apavlov) 2012-08-02 10:25:59 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=93006. Re-closing this bug.