New issue
Advanced search Search tips

Issue 745376 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 651762



Sign in to add a comment

Correct reflection behavior of colgroup.span, col.span, td/th.colSpan, and td/th.rowSpan

Project Member Reported by tkent@chromium.org, Jul 18 2017

Issue description

Chrome Version: 61 canary
OS: All but iOS

What steps will reproduce the problem?
(1) Open http://w3c-test.org/html/dom/reflection-tabular.html

What is the expected result?
No Fail tests

What happens instead?
21 Fails about colgroup.span, col.span, td/th.colSpan, and td/th.rowSpan.

Please use labels and text to provide additional information.
https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#clamped-to-the-range



For graphics-related bugs, please copy/paste the contents of the about:gpu
page at the end of this report.

 

Comment 1 by tkent@chromium.org, Jul 18 2017

Our rowSpan lower limit is 1, instead of 0 defined by the standard. It's an intentional behavior for now.

Comment 2 by tkent@chromium.org, Jul 18 2017

Owner: tkent@chromium.org
Status: Started (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e043c4c05e0a2c9629edc2c5512e67e23d7f9b6

commit 8e043c4c05e0a2c9629edc2c5512e67e23d7f9b6
Author: Kent Tamura <tkent@chromium.org>
Date: Tue Jul 18 12:12:47 2017

COLGROUP / COL: Setting |span| IDL value should reflect to |span| attribute as is.

We don't need a special handling for 0 value.

https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#clamped-to-the-range
> On setting, it behaves the same as setting a regular reflected unsigned integer.

Bug:  745376 
Change-Id: I6dead5b474324cd19e811c950971e5804532508a
Reviewed-on: https://chromium-review.googlesource.com/575738
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487445}
[modify] https://crrev.com/8e043c4c05e0a2c9629edc2c5512e67e23d7f9b6/third_party/WebKit/LayoutTests/external/wpt/html/dom/reflection-tabular-expected.txt
[modify] https://crrev.com/8e043c4c05e0a2c9629edc2c5512e67e23d7f9b6/third_party/WebKit/Source/core/html/HTMLTableColElement.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6999b61a3be6b84ae0e5f22679d068e57c751ed2

commit 6999b61a3be6b84ae0e5f22679d068e57c751ed2
Author: Kent Tamura <tkent@chromium.org>
Date: Wed Jul 19 04:36:46 2017

TD, TH, COLGROUP, COL: Correct the fallback value when out-of-range value is set via rowSpan, colSpan, span IDL setters.

Their default values are 1, not 0.

https://html.spec.whatwg.org/multipage/tables.html#dom-colgroup-span
https://html.spec.whatwg.org/multipage/tables.html#dom-col-span
https://html.spec.whatwg.org/multipage/tables.html#dom-tdth-colspan
https://html.spec.whatwg.org/multipage/tables.html#dom-tdth-rowspan
> ... content attribute. It is clamped to the range [...], and its default value is 1.

https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#clamped-to-the-range
> On setting, it behaves the same as setting a regular reflected unsigned integer.

https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:idl-unsigned-long
> On setting, first, if the new value is in the range 0 to 2147483647, then let
> n be the new value, otherwise let n be the default value, or ...

Bug:  745376 
Change-Id: Ibac4d9a7e79c51064a073419e1695f851f7a9df9
Reviewed-on: https://chromium-review.googlesource.com/576010
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487749}
[modify] https://crrev.com/6999b61a3be6b84ae0e5f22679d068e57c751ed2/third_party/WebKit/LayoutTests/external/wpt/html/dom/reflection-tabular-expected.txt
[modify] https://crrev.com/6999b61a3be6b84ae0e5f22679d068e57c751ed2/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/6999b61a3be6b84ae0e5f22679d068e57c751ed2/third_party/WebKit/Source/core/dom/Element.h
[modify] https://crrev.com/6999b61a3be6b84ae0e5f22679d068e57c751ed2/third_party/WebKit/Source/core/html/HTMLTableCellElement.cpp
[modify] https://crrev.com/6999b61a3be6b84ae0e5f22679d068e57c751ed2/third_party/WebKit/Source/core/html/HTMLTableColElement.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/542418665db7ab67bc3d433c429d9c335a34c4e4

commit 542418665db7ab67bc3d433c429d9c335a34c4e4
Author: Kent Tamura <tkent@chromium.org>
Date: Thu Jul 20 05:28:42 2017

Parse clamped unsigned integer attribute values correctly.

https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#clamped-to-the-range

We had a bug that an attribute value greater than 2^32-1 was handled as a parse
error, instead of range overflow. We need to clamp such values to a maximum
value defined for a attribute.

Implementation:
The main change is to introduce blink::ParseHTMLClampedNonNegativeInteger(), and
use it in HTMLTableCellElement and HTMLTableColElement.

This CL introduces WTF::NumberParsingState in order to pass "fail by overflow"
information from platform/wtf/StringToNumber code.

Bug:  745376 
Change-Id: Ie57a0538816f0f508324573cdcda6d96ad51afb2
Reviewed-on: https://chromium-review.googlesource.com/577428
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488143}
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/LayoutTests/external/wpt/html/dom/reflection-tabular-expected.txt
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/LayoutTests/fast/table/incorrect-colgroup-span-values-expected.txt
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/LayoutTests/fast/table/incorrect-colgroup-span-values.html
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/LayoutTests/html/tabular_data/col_span-expected.txt
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/LayoutTests/html/tabular_data/col_span.html
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/LayoutTests/html/tabular_data/td_colspan-expected.txt
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/LayoutTests/html/tabular_data/td_colspan.html
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/LayoutTests/html/tabular_data/td_rowspan-expected.txt
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/LayoutTests/html/tabular_data/td_rowspan.html
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/Source/core/html/BUILD.gn
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/Source/core/html/HTMLTableCellElement.cpp
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/Source/core/html/HTMLTableCellElement.h
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/Source/core/html/HTMLTableColElement.cpp
[add] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/Source/core/html/TableConstants.h
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/Source/core/html/parser/HTMLParserIdioms.cpp
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/Source/core/html/parser/HTMLParserIdioms.h
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/Source/platform/wtf/text/StringToNumber.cpp
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/Source/platform/wtf/text/StringToNumber.h
[modify] https://crrev.com/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/Source/platform/wtf/text/StringToNumberTest.cpp

Comment 6 by tkent@chromium.org, Jul 20 2017

Labels: M-61

Comment 7 by phistuck@gmail.com, Jul 20 2017

Any of those tests can be upstreamed to web platform tests?
I looked at https://chromium.googlesource.com/chromium/src/+/542418665db7ab67bc3d433c429d9c335a34c4e4/third_party/WebKit/LayoutTests/html/tabular_data/col_span.html for example and it does not seem to use any WebKit/Blink specific/internal APIs.

Comment 8 by tkent@chromium.org, Jul 20 2017

Status: Fixed (was: Started)
> Any of those tests can be upstreamed to web platform tests?

Though I haven't looked at all of them yet, I guess WPT already has equivalent tests, and we can remove non-WPT tests.

Sign in to add a comment