Skip to content
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

Do not throw for zero colgroup/col.span and textarea.rows/cols #1693

Merged
merged 1 commit into from Aug 19, 2016

Conversation

domenic
Copy link
Member

@domenic domenic commented Aug 18, 2016

Instead, fall back to the default value.

For col/colgroup, 3/4 engines do not throw; this aligns with the
majority. For textarea, 2/4 engines do not throw, and Firefox would like
to align with not throwing. More detail at
#1200 (comment).

Fixes #1200.


I think we got general agreement on this direction in #1200, so we only need editor review to land this. But /cc @cdumez @rniwa @ayg @smaug---- who were involved in the thread.

I will also work on some web platform tests for these.

Instead, fall back to the default value.

For col/colgroup, 3/4 engines do not throw; this aligns with the
majority. For textarea, 2/4 engines do not throw, and Firefox would like
to align with not throwing. More detail at
#1200 (comment).

Fixes #1200.
@domenic domenic added normative change compat Standard is not web compatible or proprietary feature needs standardizing topic: forms labels Aug 18, 2016
@domenic
Copy link
Member Author

domenic commented Aug 18, 2016

Hold off, web platform tests reveal this isn't actually how it works. It looks like the content attribute gets set to 0 but on getting the IDL attribute converts that to 1.

@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Aug 18, 2016
@domenic
Copy link
Member Author

domenic commented Aug 18, 2016

OK, so WebKit does something reasonable that matches this spec, whereas everyone else deviates from the spec in strange ways. I suspect that at least Chrome's deviations are due to not having a good implementation of the 2147483647 wrapping or of default value fallback on getting.

We should merge this and then everyone should work to pass the web platform tests that I'm about to send.

@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Aug 18, 2016
domenic added a commit to web-platform-tests/wpt that referenced this pull request Aug 18, 2016
successful, and the value is in the range 1 to 2147483647 inclusive, the resulting value must be
returned. If, on the other hand, it fails or returns an out of range value, or if the attribute is
absent, the default value must be returned instead. On setting, first, if the new value is in the
range 1 to 2147483647, then let <var>n</var> be the new value, otherwise let <var>n</var> be the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should mention inclusive here too (style I used elsewhere was "the range x to y, inclusive," btw)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I guess the current style is to not do that, although if you search for "the range" it seems HTML has all kinds of styles

@annevk
Copy link
Member

annevk commented Aug 19, 2016

LGTM

@domenic domenic merged commit 00b9b91 into master Aug 19, 2016
@domenic domenic deleted the fallback-below-zero branch August 19, 2016 15:52
ayg added a commit to ayg/web-platform-tests that referenced this pull request Aug 21, 2016
@ayg
Copy link
Contributor

ayg commented Aug 21, 2016

ayg added a commit to ayg/web-platform-tests that referenced this pull request Aug 22, 2016
ayg added a commit to web-platform-tests/wpt that referenced this pull request Aug 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat Standard is not web compatible or proprietary feature needs standardizing normative change topic: forms
Development

Successfully merging this pull request may close these issues.

None yet

3 participants