Bug 37244 – thead in table without tbody causes table height doubling

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Elliott Sprehn]

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.

[Elliott Sprehn]

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.

[Elliott Sprehn]

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)

[Julien Chaffraix]

Created attachment 124734 [details]
Proposed fix: An empty table has no section, not no tbody.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Julien Chaffraix]

Created attachment 124796 [details]
Fixed another issue uncovered by the test case.

[Julien Chaffraix]

Created attachment 124962 [details]
Rebased patch.

[WebKit Review Bot]

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.

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 125420 [details]
Patch for landing

[Adam Barth]

Comment on attachment 125420 [details]
Patch for landing

This patch is crashing the commit-queue for reasons I don't understand.

[Julien Chaffraix]

(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?

[Adam Barth]

I'd just land it manually.  I'll investigate the cq issue.

[Julien Chaffraix]

Created attachment 126526 [details]
Patch for landing

[WebKit Review Bot]

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

[Julien Chaffraix]

Committed r107971: <http://trac.webkit.org/changeset/107971>

[Julien Chaffraix]

More Chromium rebaseline in http://trac.webkit.org/changeset/108088.

[Csaba Osztrogonác]

Qt rebaseline landed in http://trac.webkit.org/changeset/108160

[Alexander Pavlov (apavlov)]

@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?

[Julien Chaffraix]

(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.

[Alexander Pavlov (apavlov)]

Filed https://bugs.webkit.org/show_bug.cgi?id=93006. Re-closing this bug.