Set table cell colspan=0 to 1 instead as the HTML spec requires and other UAs do

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: mats, Assigned: mats)

Tracking

({html5})

Trunk
mozilla47
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox46 affected, firefox47 fixed)

Details

Attachments

(1 attachment)

(In reply to Boris Zbarsky [:bz] from comment #22)
> For colspan, could we just remove support for colspan="0"?  Other UAs don't
> seem to support it and the HTML5 spec says colspan "must be a valid
> non-negative integer greater than zero".  And the UA requirements are "If
> parsing that value failed, or returned zero, or if the attribute is absent,
> then let colspan be 1, instead."

Yeah, I think we should map colspan=0 to 1 like the spec says and other UAs do.
https://html.spec.whatwg.org/multipage/tables.html#attr-tdth-colspan

I got some WPT failures though.  Apparently we test setting all (long) IDL
members to 0 and -0 and expect to get 0, or something:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fff0dbf437c3&selectedJob=15767547&filter-searchStr=Linux%20x64%20debug%20W3C%20Web%20Platform%20Tests%20W3C%20Web%20Platform%20Tests%20W%283%29

Do you know how to work around that?
Flags: needinfo?(bzbarsky)
You could just add the relevant failure annotations and file a bug on wpt, since it's not matching spec here.
Flags: needinfo?(bzbarsky)
Sorry to bother you again, but how does one add failure annotations in wpt?
I think the tests that fails lives here:
http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/html/dom/reflection.js
but I can't find anything that looks like a manifest file around here.
Flags: needinfo?(bzbarsky)
The relevant failing test is html/dom/reflection-tabular.html

So you want to edit testing/web-platform/meta/html/dom/reflection-tabular.html.ini and add the relevant bits in there.

I thought there was a way to just run wpt in such a way that it updates the ini file for you, but I can't figure it out from reading mach help.  James, am I remembering correctly?
Flags: needinfo?(bzbarsky) → needinfo?(james)
You can; it's a two step process:

|./mach web-platform-tests html/dom/reflection-tabular.html --log-raw output.log|
|./mach web-platform-tests-update output.log|
Flags: needinfo?(james)
Oh, and I should say that will try to do a commit or alter mq. You might want --no-patch.
Thanks Boris, that makes the tests pass locally.  New Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f7b198d8e19
(In reply to James Graham [:jgraham] from comment #4)
> You can; it's a two step process:

Good to know, I'll try that next time :-)
Posted patch fixSplinter Review
Attachment #8711212 - Flags: review?(bzbarsky)
Comment on attachment 8711212 [details] [diff] [review]
fix

This is good as far as it goes, assuming the reflection matches other UAs, but I think you can also get rid of a ton of complexity in the cellmap around 0 colspans...

Please get dbaron to review getting rid of this functionality once he gets back.
Attachment #8711212 - Flags: review?(bzbarsky) → review+
Blocks: 1242164
(In reply to Boris Zbarsky [:bz] from comment #9)
> This is good as far as it goes, assuming the reflection matches other UAs,

Yes, the reflection and layout now matches that of Chrome, Safari and IE11.

> but I think you can also get rid of a ton of complexity in the cellmap
> around 0 colspans...

Indeed, that is my goal.  I figured we should start by making it dead code
and then we can remove it in a separate bug.  I filed bug 1242164.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/786b9802eb48
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
FWIW, comments marked as "advocacy" are completely unavailable (impossible to unminimize) with JavaScript disabled. According to unobtrusive-JS principle, such elements should be visible by default and hidden only when JS is enabled.
Unfortunately, supporting colspan="0" was not free even given the "efforts and man-hours put into its implementation".  It added significant complexity to the table code, which prevented people from fixing other table issues because the code could not be understood by anyone I am aware of.  It also caused the table code to be slower than it could be in many cases (we're talking different algorithmic complexity here, not just constant-factor slowdown).

I agree that the existence of already functioning code is an argument to consider when deciding whether to keep/implement a feature.  But it can't be the only argument if the feature imposes other costs; at some point you cross over into the realm of sunk cost fallacy.

Given the lack of use of this on the web (due to other browsers not implementing it), the fact that the other browsers have explicitly said they do not plan to implement it, and the fact that the existence of this code imposed a quite significant ongoing maintenance burden, I believe that removing it was in fact the right decision.  Of course I reviewed the patch that removes it, and if I thought it was the wrong decision I wouldn't have done that....
(In reply to Boris Zbarsky [:bz] from comment #16)
> It added significant complexity to the table code, which prevented people 
> from fixing other table issues because the code could not be understood 
> by anyone I am aware of.

Couldn't this be implemented without mixing the colspan=0 code with any other code -- by just (sort of) _preprocessing_ tables by internally replacing `0` value with the corresponding calculated nonzero value _before_ any other table code runs? So first we calculate the actual nonzero value, then run the rest code as (almost) if the calculated value has been originally used instead of `0` by the author.

> It also caused the table code to be slower than it could be in many cases
> (we're talking different algorithmic complexity here, not just constant-factor
> slowdown).

I'm not sure why this should slow down tables that don't have cells with colspan=0.

> the other browsers have explicitly said they do not plan to implement it

Could you provide corresponding links? Thanks.
> Couldn't this be implemented without mixing the colspan=0 code with any other code

Maybe.  There are complications with your proposal because cells can be added and removed dynamically, including both colspan="0" cells and cells before/after them.  So the preprocessing would have to be either done or optimized out on every mutation, at least for tables that have colspan=0.

But the existing code was most definitely NOT implemented that way; it was very much done to make the colspan=0 case reasonably fast, which meant it had its fingers in the central data structure keeping track of the table structure, and complicated code to keep everything up to date on mutations.

> I'm not sure why this should slow down tables that don't have cells with colspan=0.

You _could_ have a performance cliff like that: mutating tables has tolerable performance unless there's a single colspan=0 cell somewhere in there, and then it gets way slower.  But generally performance cliffs are bad, especially for a feature we want people to actually use. 

> Could you provide corresponding links?

Not without searching the relevant mailing lists and bugzilla stuff (whatwg, HTML working group, the HTML bug database in the W3C bugzilla, the HTML issue tracker).  You can do that yourself just as well as I can...
> You can do that yourself just as well as I can

I tried (e.g. found corresponding bug-tracker tickets for Chromium [1] and WebKit [2]), but with no success as for proving your statement. And isn't it logical to expect that it's who has stated something unproved should provide a proof, not someone else? ;-)

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=180722
[2] https://bugs.webkit.org/show_bug.cgi?id=10300
(In reply to comment #18)
> But the existing code was most definitely NOT implemented that way

Maybe some refactoring would be more reasonable than dropping the entire distinctive feature.
> And isn't it logical to expect that it's who has stated something unproved should provide a proof

If I wanted to "win the argument", perhaps.  But I honestly just don't care that much to convince you of the truth of that statement. 

> Maybe some refactoring would be more reasonable than dropping the entire distinctive feature.

It needed a complete rewrite at best, not refactoring.
(In reply to Boris Zbarsky [:bz] from comment #21)
> > And isn't it logical to expect that it's who has stated something unproved should provide a proof
> 
> If I wanted to "win the argument", perhaps.  But I honestly just don't care
> that much to convince you of the truth of that statement.

As long as an unproved fact is used as one of major reasons to drop a distinctive feature, this is probably a matter of something more than just a _wish_ of someone. (To be clear: I don't insist on anything, just trying to figure out what's happening and why, substantively.)
> As long as an unproved fact is used as one of major reasons to drop a distinctive feature

The fact is proved to my satisfaction, based on observation of the other UAs bug databases and interactions with their developers.  I can't recall whether the relevant interactions were in the public mailing lists, in private mail, in-person discussions, or whatnot, but they certainly occurred.

The fact proved to _your_ satisfaction, but that doesn't affect my decisionmaking process.
> The fact proved to _your_ satisfaction

I meant "not proved".
(In reply to Boris Zbarsky [:bz] from comment #23)
Boris, I respect and trust you, just would like to be able to learn more details with some reasoning background, firsthand.
You need to log in before you can comment on or make changes to this bug.