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

Can we remove the HTMLTableDataCellElement/HTMLTableHeaderCellElement fiction? #1115

Closed
bzbarsky opened this Issue Apr 23, 2016 · 31 comments

Comments

6 participants
@bzbarsky
Collaborator

bzbarsky commented Apr 23, 2016

UAs just have HTMLTableCellElement.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 23, 2016

Member

I thought we did this as part of removing table sorting. Yes, we should fix this.

Member

domenic commented Apr 23, 2016

I thought we did this as part of removing table sorting. Yes, we should fix this.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Apr 23, 2016

Member

We discussed that as part of that, but decided not to since it would be more complicated than just removing table sorting. E.g., if we want to keep the current conformance requirements, we cannot really make this change, since td and th both have distinct obsolete members at the moment.

Member

annevk commented Apr 23, 2016

We discussed that as part of that, but decided not to since it would be more complicated than just removing table sorting. E.g., if we want to keep the current conformance requirements, we cannot really make this change, since td and th both have distinct obsolete members at the moment.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Apr 25, 2016

Collaborator

Do browsers have those distinct obsolete members?

Collaborator

bzbarsky commented Apr 25, 2016

Do browsers have those distinct obsolete members?

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 25, 2016

Member

I think @annevk is saying that it will just take a bit of work to spec since we'll want to say something like "This attribute only has meaning for th elements" with regard to the abbr and scope IDL attributes. It shouldn't be very hard. I'll work on it now.

Member

domenic commented Apr 25, 2016

I think @annevk is saying that it will just take a bit of work to spec since we'll want to say something like "This attribute only has meaning for th elements" with regard to the abbr and scope IDL attributes. It shouldn't be very hard. I'll work on it now.

@domenic domenic self-assigned this Apr 25, 2016

domenic added a commit that referenced this issue Apr 25, 2016

Merge the two table cell interfaces back together
Ever since table sorting was introduced, HTMLTableCellElement was split
into two descendant interfaces, HTMLTableDataCellElement for <td>, and
HTMLTableHeaderCellElement for <th>. No browser ever implemented this
split. Although table sorting was removed in
59b7e24,
the split remained; we now remove it, fixing #1115.
@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Apr 26, 2016

Collaborator

Ah, I see. I guess Gecko reflects abbr and scope as strings (on both th and td) but does absolutely nothing else with them. Haven't checked other impls.

Collaborator

bzbarsky commented Apr 26, 2016

Ah, I see. I guess Gecko reflects abbr and scope as strings (on both th and td) but does absolutely nothing else with them. Haven't checked other impls.

@domenic domenic closed this in e383ae2 Apr 26, 2016

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Apr 26, 2016

Collaborator

Need to update WPT too.

Collaborator

bzbarsky commented Apr 26, 2016

Need to update WPT too.

@bzbarsky bzbarsky reopened this Apr 26, 2016

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 26, 2016

Member

Filed web-platform-tests/wpt#2908 to track that.

Member

domenic commented Apr 26, 2016

Filed web-platform-tests/wpt#2908 to track that.

@domenic domenic closed this Apr 26, 2016

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Jul 7, 2016

Contributor

FYI, we had implemented this in WebKit:
https://bugs.webkit.org/show_bug.cgi?id=148859

I think IE had this as well but I am not 100% sure.

Anyway, I am now rolling out the change via:
https://bugs.webkit.org/show_bug.cgi?id=159518

Next time, I'll wait for Firefox and Chrome to follow the spec before implementing :/

Contributor

cdumez commented Jul 7, 2016

FYI, we had implemented this in WebKit:
https://bugs.webkit.org/show_bug.cgi?id=148859

I think IE had this as well but I am not 100% sure.

Anyway, I am now rolling out the change via:
https://bugs.webkit.org/show_bug.cgi?id=159518

Next time, I'll wait for Firefox and Chrome to follow the spec before implementing :/

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 7, 2016

Member

Oh my goodness, we're so sorry! We'll try to be better about testing in nightlies and giving everyone a heads-up in the future :(. This should not happen again.

Member

domenic commented Jul 7, 2016

Oh my goodness, we're so sorry! We'll try to be better about testing in nightlies and giving everyone a heads-up in the future :(. This should not happen again.

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Jul 7, 2016

Contributor

It is a good thing you guys updated the web-platform-tests so we noticed the change before actually shipping HTMLTableDataCellElement / HTMLTableHeaderCellElement.

Contributor

cdumez commented Jul 7, 2016

It is a good thing you guys updated the web-platform-tests so we noticed the change before actually shipping HTMLTableDataCellElement / HTMLTableHeaderCellElement.

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Jul 7, 2016

Contributor

In the future, checking Safari Technology Preview [1], rather than shipping Safari would be nice. We have been making an effort aligning our implementation with the spec.

[1] https://developer.apple.com/safari/technology-preview/

Contributor

cdumez commented Jul 7, 2016

In the future, checking Safari Technology Preview [1], rather than shipping Safari would be nice. We have been making an effort aligning our implementation with the spec.

[1] https://developer.apple.com/safari/technology-preview/

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 7, 2016

Member

Yep. I specifically ordered a Mac for testing Safari Tech Preview after opening a couple conformance bugs and learning they were fixed. It is sitting on my desk. That's part of why I'm more confident we won't let this kind of thing happen again.

It's also a lesson to us to make a greater effort to get sign-off from all implementers, even on changes that seem like they're just aligning with reality. It's already risky enough for implementers to go first in implementing something; we should not be making it more risky in this way. Being willing to go first should be celebrated, not punished.

Member

domenic commented Jul 7, 2016

Yep. I specifically ordered a Mac for testing Safari Tech Preview after opening a couple conformance bugs and learning they were fixed. It is sitting on my desk. That's part of why I'm more confident we won't let this kind of thing happen again.

It's also a lesson to us to make a greater effort to get sign-off from all implementers, even on changes that seem like they're just aligning with reality. It's already risky enough for implementers to go first in implementing something; we should not be making it more risky in this way. Being willing to go first should be celebrated, not punished.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Jul 7, 2016

Collaborator

Unfortunately, Safari Technology Preview requires the most recent Mac OS version, which not everyone may have available. Amusingly, WebKit nightlies are less strict about that.

For the rest, I agree that people making changes first should not be punished, having been in that position myself. But I also think implementors should be very wary of making changes that move from "everyone is interoperable" to "now we follow the spec and are no longer doing the same thing as everyone else". In particular, there's a very good chance that a spec that is not matching something that every implementation does is just wrong.

Collaborator

bzbarsky commented Jul 7, 2016

Unfortunately, Safari Technology Preview requires the most recent Mac OS version, which not everyone may have available. Amusingly, WebKit nightlies are less strict about that.

For the rest, I agree that people making changes first should not be punished, having been in that position myself. But I also think implementors should be very wary of making changes that move from "everyone is interoperable" to "now we follow the spec and are no longer doing the same thing as everyone else". In particular, there's a very good chance that a spec that is not matching something that every implementation does is just wrong.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Jul 7, 2016

Collaborator

Oh, and I guess my point was that implementors should actually talk to each other about making such breaking changes. Chrome and Firefox both have public intent-to-implement/intent-to-ship processes that allow other implementors and spec editors to have an idea of what's going on and have a built-in "what do other UAs do?" sanity-check step. I wish other implementors would adopt something similar...

Collaborator

bzbarsky commented Jul 7, 2016

Oh, and I guess my point was that implementors should actually talk to each other about making such breaking changes. Chrome and Firefox both have public intent-to-implement/intent-to-ship processes that allow other implementors and spec editors to have an idea of what's going on and have a built-in "what do other UAs do?" sanity-check step. I wish other implementors would adopt something similar...

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Jul 7, 2016

Contributor

FYI, WebKit nightlies still exist as well if your system does not support Safari Technology preview.

Also, the reason we made the change in WebKit was because I believe IE/Edge already had implemented it (At least this is what my changelog says). So not everyone was interoperable unless I incorrectly tested IE / Edge at the time.

Contributor

cdumez commented Jul 7, 2016

FYI, WebKit nightlies still exist as well if your system does not support Safari Technology preview.

Also, the reason we made the change in WebKit was because I believe IE/Edge already had implemented it (At least this is what my changelog says). So not everyone was interoperable unless I incorrectly tested IE / Edge at the time.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 7, 2016

Member

I've just tested Edge, and it's true: they implemented this. We should have caught that as well.

Given that, with Edge + Safari TP on one side vs. Chrome and Firefox on the other, should we revert the removal? I think the separate classes is a better model...

Member

domenic commented Jul 7, 2016

I've just tested Edge, and it's true: they implemented this. We should have caught that as well.

Given that, with Edge + Safari TP on one side vs. Chrome and Firefox on the other, should we revert the removal? I think the separate classes is a better model...

@rniwa

This comment has been minimized.

Show comment
Hide comment
@rniwa

rniwa Jul 7, 2016

Collaborator

FYI, W3C version has these two classes separate: https://w3c.github.io/html/tabular-data.html#the-th-element

Collaborator

rniwa commented Jul 7, 2016

FYI, W3C version has these two classes separate: https://w3c.github.io/html/tabular-data.html#the-th-element

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 7, 2016

Member

Yes, they haven't managed to copy over this change yet, although it's in their list

Member

domenic commented Jul 7, 2016

Yes, they haven't managed to copy over this change yet, although it's in their list

@rniwa

This comment has been minimized.

Show comment
Hide comment
@rniwa

rniwa Jul 7, 2016

Collaborator

I think we should revert the change and keep two classes separate given abbr and scope behave differently on td and th.

Collaborator

rniwa commented Jul 7, 2016

I think we should revert the change and keep two classes separate given abbr and scope behave differently on td and th.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 7, 2016

Member

@rniwa how do they behave differently in WebKit? In Blink they behave the same (reflecting the content attribute).

I'm happy to revert, although I'd prefer to hear from @bzbarsky before doing so.

Member

domenic commented Jul 7, 2016

@rniwa how do they behave differently in WebKit? In Blink they behave the same (reflecting the content attribute).

I'm happy to revert, although I'd prefer to hear from @bzbarsky before doing so.

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Jul 7, 2016

Contributor

In WebKit, th.scope only returns known values while td.scope does simple reflection.

Contributor

cdumez commented Jul 7, 2016

In WebKit, th.scope only returns known values while td.scope does simple reflection.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Jul 7, 2016

Collaborator

I don't have a strong opinion on this. I had been told that in IE these used the same interface, but if Edge is not matching IE here, I guess I don't feel too strongly about changing the behavior.

I should note that the behavior of scope/abbr in the spec still does not match the WebKit implementation described above, so the spec needed some sort of change here no matter what...

Collaborator

bzbarsky commented Jul 7, 2016

I don't have a strong opinion on this. I had been told that in IE these used the same interface, but if Edge is not matching IE here, I guess I don't feel too strongly about changing the behavior.

I should note that the behavior of scope/abbr in the spec still does not match the WebKit implementation described above, so the spec needed some sort of change here no matter what...

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Jul 7, 2016

Contributor

@bzbarsky: Well, iirc the spec has scope / abbr on HTMLTableHeaderCellElement but not on HTMLTableDataCellElement. We merely had them on both in WebKit for backward compatibility.

Contributor

cdumez commented Jul 7, 2016

@bzbarsky: Well, iirc the spec has scope / abbr on HTMLTableHeaderCellElement but not on HTMLTableDataCellElement. We merely had them on both in WebKit for backward compatibility.

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Jul 7, 2016

Contributor

@bzbarsky: As far as I can tell, our implementation of th.scope / th.abbr matched the spec:

  • The scope IDL attribute must reflect the content attribute of the same name, limited to only known values.
  • The abbr IDL attribute must reflect the content attribute of the same name.

The only difference was that we had them on td as well for backward compatibility, unless I am missing something?

Contributor

cdumez commented Jul 7, 2016

@bzbarsky: As far as I can tell, our implementation of th.scope / th.abbr matched the spec:

  • The scope IDL attribute must reflect the content attribute of the same name, limited to only known values.
  • The abbr IDL attribute must reflect the content attribute of the same name.

The only difference was that we had them on td as well for backward compatibility, unless I am missing something?

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Jul 7, 2016

Collaborator

@cdumez The point is, the spec needs to have them on whatever it is <td> ends up as, since all UAs do. Unless you have data that this is not needed for web compat?

Collaborator

bzbarsky commented Jul 7, 2016

@cdumez The point is, the spec needs to have them on whatever it is <td> ends up as, since all UAs do. Unless you have data that this is not needed for web compat?

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Jul 7, 2016

Contributor

@bzbarsky: I see. I agree with you then. And no, I have have data saying they not needed on td for web-compatibility, which is why I kept them on td in WebKit.

Contributor

cdumez commented Jul 7, 2016

@bzbarsky: I see. I agree with you then. And no, I have have data saying they not needed on td for web-compatibility, which is why I kept them on td in WebKit.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 7, 2016

Member

This gets worse and worse. Edge has scope on TableCell (the base class) and TableHeaderCell (th) but not on TableDataCell (td). It has abbr on the base class but not on the two derived classes.

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

So we have four mutually-incompatible models here:

  • The current spec/Chrome/Firefox
  • The old spec (scope and abbr only on th)
  • Safari (scope and abbr only on th and td, not on TableCell)
  • Edge (scope on TableCell and th, abbr on TableCell)

With this in mind two proposals make sense to me:

  • Go with the only two browsers who agree: Chrome and Firefox
  • Go with Safari's model, with scope and abbr on td being relegated to the appendix full of obsolete APIs

Given that every browser has scope and abbr on tds either directly or indirectly, I don't think we should try to go down the route of the old spec and remove them from there.

Member

domenic commented Jul 7, 2016

This gets worse and worse. Edge has scope on TableCell (the base class) and TableHeaderCell (th) but not on TableDataCell (td). It has abbr on the base class but not on the two derived classes.

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

So we have four mutually-incompatible models here:

  • The current spec/Chrome/Firefox
  • The old spec (scope and abbr only on th)
  • Safari (scope and abbr only on th and td, not on TableCell)
  • Edge (scope on TableCell and th, abbr on TableCell)

With this in mind two proposals make sense to me:

  • Go with the only two browsers who agree: Chrome and Firefox
  • Go with Safari's model, with scope and abbr on td being relegated to the appendix full of obsolete APIs

Given that every browser has scope and abbr on tds either directly or indirectly, I don't think we should try to go down the route of the old spec and remove them from there.

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Jul 7, 2016

Contributor

I personally don't mind going with the Firefox / Chrome behavior at this point given that:

  1. This is also Safari 9 behavior
  2. The new WebKit behavior has not shipped in Safari yet.
  3. I have a patch ready to land to restore the old behavior in WebKit which would mean Safari / Chrome and Firefox would all stay aligned.
Contributor

cdumez commented Jul 7, 2016

I personally don't mind going with the Firefox / Chrome behavior at this point given that:

  1. This is also Safari 9 behavior
  2. The new WebKit behavior has not shipped in Safari yet.
  3. I have a patch ready to land to restore the old behavior in WebKit which would mean Safari / Chrome and Firefox would all stay aligned.
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 7, 2016

Member

OK. I think we should probably do that then, although this still stands out as a particularly bad oversight by us spec editors.

You aren't planning to implement XMLDocument.prototype.load, are you? :) #1478

Member

domenic commented Jul 7, 2016

OK. I think we should probably do that then, although this still stands out as a particularly bad oversight by us spec editors.

You aren't planning to implement XMLDocument.prototype.load, are you? :) #1478

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Jul 7, 2016

Contributor

Ok, WebKit agrees with the spec again: http://trac.webkit.org/changeset/202937.

Contributor

cdumez commented Jul 7, 2016

Ok, WebKit agrees with the spec again: http://trac.webkit.org/changeset/202937.

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Jul 8, 2016

Member

The old spec (scope and abbr only on th)

The old spec had abbr on td as well. (In the obsolete section.)

https://github.com/whatwg/html/pull/1125/files#diff-36cd38f49b9afa08222c0dc9ebfe35ebL113151

Member

zcorpan commented Jul 8, 2016

The old spec (scope and abbr only on th)

The old spec had abbr on td as well. (In the obsolete section.)

https://github.com/whatwg/html/pull/1125/files#diff-36cd38f49b9afa08222c0dc9ebfe35ebL113151

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