Manipulation: Don't provide the parser with sloppy table markup #2499
Conversation
|
It would be great if we had a test case for this. Maybe all we need is to finish #1731 |
Most likely. This will be my priority after dealing with these Android 2.3-related things. |
|
Hm, weird, i remember checking this in xhtml document, there should be more bugs like that, like auto-insertion of |
|
Finishing #1731 would be sufficient to have caught this I believe, but as I outlined in that issue a while back, it's a rather tricky problem (in the same way unit testing under CSP proved very difficult to do in practice). The jQuery test suite was designed almost exclusively for HTML mode so far, and only a subset of the tests would be valid. If we converted the entire test suite a a polyglot that can be run under XHTML and HTML, we introduce the possibility of regressions for people in the HTML mode, which is a very bad idea given that's likely how 99% of jQuery code is executed. I do believe the unit testing of this belongs more in the scope of #1731 than here, since I'm fairly certain this functionality is already has tests, and would be happy to continue the discussion of possible solutions there. |
|
@anthonyryan1 Is this the only XHTML-related issue you found in current code? @jquery/core Do we want to land it as is or wait for a proper XHTML testing? I still have it on my plate but it may require some time so I can't commit to it before October. |
|
I've been running it in production with sizzle, deprecated, jsonp and a few other parts removed so I can't fully comment on those parts at the moment. I'll schedule some testing that will include those. |
|
I think we should land this ASAP and let the XML test arrive when it does. |
|
This will influence #2581 a little as there I'm currently removing one more such thing. It would be good to land this one and then I'll remove the removal from my PR. |
src/manipulation/wrapMap.js
Outdated
There was a problem hiding this comment.
This comment should be removed as it suggests we're only putting <colgroup> here because of Android 2.3 and it's XML as well. The XHTML-related comment from below should be moved above this line.
While we can reply on parsers that were designed to cope with malformed syntax to understand what we mean, we shouldn't intentionally provide bad markup, not all parsers will accept it. "Be conservative in what you do, be liberal in what you accept from others." Reverts 0ea342a See also jquerygh-2031, jquerygh-2002 Closes jquerygh-2493
|
Fixed the comment including removing the support comment for Android 2.3 anticipating #2581. |
While we can reply on parsers that were designed to cope with malformed syntax to understand what we mean, we shouldn't intentionally provide bad markup, not all parsers will accept it. "Be conservative in what you do, be liberal in what you accept from others." (cherry-picked from 99e8ff1) Reverts 0ea342a Refs gh-2031 Refs gh-2002 Fixes gh-2493 Closes gh-2499
|
Landed, thanks! |
6 participants
While we can reply on parsers that were designed to cope with malformed syntax to understand what we mean, we shouldn't intentionally provide bad markup, not all parsers will accept it.
Reverts 0ea342a
See also gh-2031, gh-2002
Fixes gh-2493