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

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Mats Palmgren (inactive)]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

You could just add the relevant failure annotations and file a bug on wpt, since it's not matching spec here.

Comment 2

[Mats Palmgren (inactive)]

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.

Comment 3

[Boris Zbarsky [: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?

Comment 4

[James Graham [:jgraham]]

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|

Comment 5

[James Graham [:jgraham]]

Oh, and I should say that will try to do a commit or alter mq. You might want --no-patch.

Comment 6

[Mats Palmgren (inactive)]

Thanks Boris, that makes the tests pass locally.  New Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f7b198d8e19

Comment 7

[Mats Palmgren (inactive)]

(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 :-)

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix

Comment 9

[Boris Zbarsky [: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.

Comment 10

[Mats Palmgren (inactive)]

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/786b9802eb48

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/786b9802eb48

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/786b9802eb48

Comment 14

[Marat Tanalin]

Guys, I've accidentally discovered that zero `colspan` has stopped to work in Firefox 47+, and I'm almost shocked by this change.

Supporting zero values of `colspan` and `rowspan` was one of a few really SMART features that are valuable ADVANTAGES of Firefox over other browsers while not actually having ANY harmful effects in real-world scenarios. I believe such useful features are among those DEEP PLATFORM INNOVATIONS that Brendan Eich meant [1] when Opera has announced their swith to WebKit/Blink.

In fact, the only reason why zero `colspan` has been removed from HTML5 compared with HTML4 was the fact that it is not widely supported, but at the same time, Ian Hickson said that it can be returned to the spec once more browsers support it.

(BTW, I believe removing the feature from the spec was a mistake, because now, all the more, nobody will bother to implement this since it's not a technical debt anymore and therefore lack of its support is kinda not an indicator of programmers' (in)competence anymore.)

It's somewhat okay that other browsers don't support it, but it looks absolutely NONSENSICAL to DROP support for the useful feature from the browser that ALREADY supports it and has efforts and man-hours put into its implementation.

REGARDLESS of support by other browsers, we could continue using the `colspan` attribute in EXTENSIONS, especially now with the advent of the WebExtensions API where e.g. options UI is implemented in pure HTML/CSS, but now we can't (I've tested and can confirm: `colspan` does NOT work in extensions too, not just in web content) just because other-browser programmers are too lazy or just not capable of implementing such features.

I believe this is a big long-term mistake. Please consider reverting the change as soon as possible. Such smart and useful features are valuable advantages of Firefox, let's NOT LOSE THEM. Thanks.

[1] https://brendaneich.com/2013/02/why-mozilla-matters/

Comment 15

[Marat Tanalin]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Marat Tanalin]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[Marat Tanalin]

> 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

Comment 20

[Marat Tanalin]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Marat Tanalin]

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

> The fact proved to _your_ satisfaction

I meant "not proved".

Comment 25

[Marat Tanalin]

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