New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec col.span/colgroup.span IDL set quirks #2705

Closed
davidsgrogan opened this Issue May 23, 2017 · 8 comments

Comments

3 participants
@davidsgrogan
Contributor

davidsgrogan commented May 23, 2017

This was discovered while Blink was implementing changes from #1198 (Spec colSpan/rowSpan IDL set quirks)

Now let's standardize col.span and colgroup.span.

No engines currently match but FF and Edge are close. (https://jsfiddle.net/dgrogan/bs01qcs4/)
Chrome 60.0.3108.0 clamps to [1, 8190], except after 2^32 - 1, where it returns 1
EdgeHTML 15.15063 clamps to [1, 1000], even after 2^32
Firefox 55.0a1 (2017-05-23) clamps to [1, 1000], except after 2^31 - 1, where it returns 1
WebKit 10.1 (12603.1.30.0.34, r217272) "clamps" to [1, 2^31 -1], afterward returning 1

The upside is that no engine internally differs between col.span and colgroup.span.

Proposal:
(1) Make col.span and colgroup.span behave the same as td.colspan, i.e. clamp to [1, 1000].
(2) values > 2^32 - 1 return 1.

@davidsgrogan

This comment has been minimized.

Show comment
Hide comment
Contributor

davidsgrogan commented May 23, 2017

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan May 24, 2017

Member

I think EdgeHTML's behavior is consistent with what was specified for colSpan in #1993 , no? Why return 1 here?

(At first I was confused about what this was about, and thought this was a duplicate of the earlier issue. Then I was still confused and thought this was about the setters, rather than the getters. But having taken a closer look my understanding now is that this is about the getters for span on col/colgroup elements, when the content attribute has a given value, which is similar to but not covered by the earlier change.)

Member

zcorpan commented May 24, 2017

I think EdgeHTML's behavior is consistent with what was specified for colSpan in #1993 , no? Why return 1 here?

(At first I was confused about what this was about, and thought this was a duplicate of the earlier issue. Then I was still confused and thought this was about the setters, rather than the getters. But having taken a closer look my understanding now is that this is about the getters for span on col/colgroup elements, when the content attribute has a given value, which is similar to but not covered by the earlier change.)

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan May 30, 2017

Member

So we need to figure out what the setter should do also to fix this.

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5213

EdgeHTML
log: 1000: 1000
log: 1001: Error: Invalid argument.
log: 2147483647: Error: Invalid argument.
log: 2147483648: Error: Invalid argument.
log: 2147483650: Error: Invalid argument.
log: 4294967300: 4
Gecko & WebKit
log: 1000: 1000
log: 1001: 1001
log: 2147483647: 2147483647
log: 2147483648: 1
log: 2147483650: 1
log: 4294967300: 4
Chromium
log: 1000: 1000
log: 1001: 1001
log: 2147483647: 2147483647
log: 2147483648: 0
log: 2147483650: 0
log: 4294967300: 4

The spec is like Gecko & WebKit I believe. The only other reflecting attributes using the "with fallback" thing in the spec are textarea cols and rows.

Given the lack of interop and the fact that EdgeHTML throws, it seems possible to switch to "clamped to the range", like colSpan/rowSpan.

Member

zcorpan commented May 30, 2017

So we need to figure out what the setter should do also to fix this.

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5213

EdgeHTML
log: 1000: 1000
log: 1001: Error: Invalid argument.
log: 2147483647: Error: Invalid argument.
log: 2147483648: Error: Invalid argument.
log: 2147483650: Error: Invalid argument.
log: 4294967300: 4
Gecko & WebKit
log: 1000: 1000
log: 1001: 1001
log: 2147483647: 2147483647
log: 2147483648: 1
log: 2147483650: 1
log: 4294967300: 4
Chromium
log: 1000: 1000
log: 1001: 1001
log: 2147483647: 2147483647
log: 2147483648: 0
log: 2147483650: 0
log: 4294967300: 4

The spec is like Gecko & WebKit I believe. The only other reflecting attributes using the "with fallback" thing in the spec are textarea cols and rows.

Given the lack of interop and the fact that EdgeHTML throws, it seems possible to switch to "clamped to the range", like colSpan/rowSpan.

@davidsgrogan

This comment has been minimized.

Show comment
Hide comment
@davidsgrogan

davidsgrogan May 31, 2017

Contributor

@zcorpan sorry for the confusion and thanks for sticking with it.

And you're right, according to "clamped to the range" there's no reason to make attributes > 2^32 - 1 return 1; Edge already has the desired new getter behavior.

For setters, for the 4294967300 line, WebIDL (or something?) specifies the 2^32 overflow behavior? I'd have thought it should be 1, just according to "On setting, first, if the new value is in the range 1 to 2147483647, then let n be the new value, otherwise let n be the default value" in https://html.spec.whatwg.org/#reflecting-content-attributes-in-idl-attributes:idl-unsigned-long-3

Anyway, I suppose that if we change to clamping, the new correct output for your example would be:

log: 1000: 1000
log: 1001: 1000
log: 2147483647: 1000
log: 2147483648: 1
log: 2147483650: 1
log: 4294967300: 4

Does that look right?

Contributor

davidsgrogan commented May 31, 2017

@zcorpan sorry for the confusion and thanks for sticking with it.

And you're right, according to "clamped to the range" there's no reason to make attributes > 2^32 - 1 return 1; Edge already has the desired new getter behavior.

For setters, for the 4294967300 line, WebIDL (or something?) specifies the 2^32 overflow behavior? I'd have thought it should be 1, just according to "On setting, first, if the new value is in the range 1 to 2147483647, then let n be the new value, otherwise let n be the default value" in https://html.spec.whatwg.org/#reflecting-content-attributes-in-idl-attributes:idl-unsigned-long-3

Anyway, I suppose that if we change to clamping, the new correct output for your example would be:

log: 1000: 1000
log: 1001: 1000
log: 2147483647: 1000
log: 2147483648: 1
log: 2147483650: 1
log: 4294967300: 4

Does that look right?

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Jun 1, 2017

Member

Yes, for the setter the IDL layer will wrap, unless [Clamp] extended attribute is used (which it is not here). See https://heycam.github.io/webidl/#abstract-opdef-converttoint step 10.

For "clamped to the range", the spec says

On setting, it behaves the same as setting a regular reflected unsigned integer.

So I think it would be:

log: 1000: 1000
log: 1001: 1001
log: 2147483647: 2147483647
log: 2147483648: 2147483648
log: 2147483650: 2147483650
log: 4294967300: 4
Member

zcorpan commented Jun 1, 2017

Yes, for the setter the IDL layer will wrap, unless [Clamp] extended attribute is used (which it is not here). See https://heycam.github.io/webidl/#abstract-opdef-converttoint step 10.

For "clamped to the range", the spec says

On setting, it behaves the same as setting a regular reflected unsigned integer.

So I think it would be:

log: 1000: 1000
log: 1001: 1001
log: 2147483647: 2147483647
log: 2147483648: 2147483648
log: 2147483650: 2147483650
log: 4294967300: 4
@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Jun 1, 2017

Member

No wait, regular unsigned integer reflection has a 2147483647 limit...

Member

zcorpan commented Jun 1, 2017

No wait, regular unsigned integer reflection has a 2147483647 limit...

@davidsgrogan

This comment has been minimized.

Show comment
Hide comment
@davidsgrogan

davidsgrogan Jun 1, 2017

Contributor

One more time :) Given that limit, and assuming we change to wording identical to colSpan's:

clamped to the range [1, 1000], and its default value is 1

I think setting would be what Gecko and WebKit do today:

log: 1000: 1000
log: 1001: 1001
log: 2147483647: 2147483647
log: 2147483648: 1
log: 2147483650: 1
log: 4294967300: 4

Other implementors, @ayg, @cdumez, thoughts on aligning with Edge for getting col/colgroup.span from the content attribute?

Contributor

davidsgrogan commented Jun 1, 2017

One more time :) Given that limit, and assuming we change to wording identical to colSpan's:

clamped to the range [1, 1000], and its default value is 1

I think setting would be what Gecko and WebKit do today:

log: 1000: 1000
log: 1001: 1001
log: 2147483647: 2147483647
log: 2147483648: 1
log: 2147483650: 1
log: 4294967300: 4

Other implementors, @ayg, @cdumez, thoughts on aligning with Edge for getting col/colgroup.span from the content attribute?

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Jun 2, 2017

Member

I think setting would be what Gecko and WebKit do today:

Yep.

Member

zcorpan commented Jun 2, 2017

I think setting would be what Gecko and WebKit do today:

Yep.

davidsgrogan added a commit to davidsgrogan/html that referenced this issue Jun 3, 2017

Clamp col.span and colgroup.span to [1,1000]
Change reflection behavior of col/colgroup.span to match that of
td/th.colSpan. Gecko and WebKit already follow the new setting
behavior. Edge already follows the new getting behavior.

Also updated the table algorithm to use these limits.

Fixes #2705.

davidsgrogan added a commit to davidsgrogan/web-platform-tests that referenced this issue Jun 7, 2017

@zcorpan zcorpan closed this in #2734 Jun 7, 2017

zcorpan added a commit that referenced this issue Jun 7, 2017

Clamp col.span and colgroup.span to [1,1000]
Change reflection behavior of col/colgroup.span to match that of
td/th.colSpan. Gecko and WebKit already follow the new setting
behavior. Edge already follows the new getting behavior.

Also updated the table algorithm to use these limits.

Fixes #2705.

hubot pushed a commit to WebKit/webkit that referenced this issue Jun 7, 2017

cdumez@apple.com
Align <col span>/<colgroup span> limits with the latest HTML specific…
…ation

https://bugs.webkit.org/show_bug.cgi?id=173049

Reviewed by Daniel Bates.

LayoutTests/imported/w3c:

Resync web-platform-tests after:
web-platform-tests/wpt#6172

This helps gain coverage for the change in this patch.

* resources/import-expectations.json:
* web-platform-tests/html/dom/elements-tabular.js:
* web-platform-tests/html/dom/reflection-tabular-expected.txt:
* web-platform-tests/html/semantics/tabular-data/processing-model-1/col-span-limits-expected.txt: Added.
* web-platform-tests/html/semantics/tabular-data/processing-model-1/col-span-limits.html: Added.
* web-platform-tests/html/semantics/tabular-data/processing-model-1/w3c-import.log:

Source/WebCore:

Align <col span>/<colgroup span> limits with the latest HTML specification after:
- whatwg/html#2705
- whatwg/html#2734

In particular, col / colspan's span attribute is now clamped to the [1, 1000] range.

Test: imported/w3c/web-platform-tests/html/semantics/tabular-data/processing-model-1/col-span-limits.html

[1] https://html.spec.whatwg.org/#dom-colgroup-span
[2] https://html.spec.whatwg.org/#clamped-to-the-range
[3] https://github.com/whatwg/html/pull/2734/files

* html/HTMLTableColElement.cpp:
(WebCore::HTMLTableColElement::parseAttribute):
As per [1][2], the span attribute should be clamped to the range [1, 1000] with a default value of 1 (on getting).

(WebCore::HTMLTableColElement::setSpan):
As per [2], on setting, we should behave the same as setting a regular reflected unsigned integer when an attribute
is clamped to a range. Therefore, we now call limitToOnlyHTMLNonNegative() instead of
limitToOnlyHTMLNonNegativeNumbersGreaterThanZero(). We used to call limitToOnlyHTMLNonNegativeNumbersGreaterThanZero()
because the value used to be limited to only non-negative numbers greater than zero with fallback before [3]. Without
this change, "col.span = 0" would set the content attribute to 1 instead of 0, which would be wrong.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@217907 268f45cc-cd09-0410-ab3c-d52691b4dbfc

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 7, 2017

Bug 1370835 - Match new standard for span IDL property; r=mystor
whatwg/html#2705
whatwg/html#2734

MozReview-Commit-ID: 7Sso9sO8f7y

--HG--
extra : rebase_source : be2560dce0619c377d570852f92efb5fad0d86ad

luke-chang pushed a commit to luke-chang/gecko that referenced this issue Aug 7, 2017

JerryShih pushed a commit to JerryShih/gecko-dev that referenced this issue Aug 7, 2017

aethanyc pushed a commit to aethanyc/gecko-dev that referenced this issue Aug 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment