using context:'template' for Nokogiri::HTML5.fragment allows parsing of otherwise illegal fragments like <td> #2426
Conversation
jrochkind
force-pushed
the
render_inline_nokogiri_contextless
branch
from
01d666f to
6bc84f0
Compare
|
I don't understand the test failures, I don't think they have to do with me? |
|
Looks like the memory allocations went up slightly with this change. Could update those specs to increase the range to fix the issues. |
jrochkind
force-pushed
the
render_inline_nokogiri_contextless
branch
from
6bc84f0 to
97c8209
Compare
|
I realized this may not be a good fix, because context can't hold Not sure if there's any way to use Nokogiri::HTML5 fragment API that isn't awful? Not really sure what to do. Ah, but including |
jrochkind
force-pushed
the
render_inline_nokogiri_contextless
branch
2 times, most recently
from
b6b2760 to
55c4ce2
Compare
…of otherwise illegal fragments like <td>
jrochkind
force-pushed
the
render_inline_nokogiri_contextless
branch
from
55c4ce2 to
5f82c72
Compare
|
OK, fixed memory allocation count -- one of the counts for 3.5 went down, which is weird, but ok! Failing "primer compatibility" for reasons that I suspect are unrelated to this PR? |
What are you trying to accomplish?
Fixes #2422, and allows
render_inlinein test to make Nokogiri HTML5 fragments that would otherwise be illegal based on lack of parent context, such as a straight<td>or<tr>What approach did you choose and why?
Created ticket at nokogiri, sparklemotion/nokogiri#3536, and while the response was that nokogiri was working as expected, as HTML5 rules don't allow context-free parsing generally, and can't parse a (eg)
<td>or<tr>without a parent<table>.However, responder @stevecheckoway, graciously helped us think through it and realize that the (hard to find documented!)
context:argument, when passed"template", would trigger HTML5 parsing rules that would allow closer to the arbitrary fragments we are used to with HTML4.Steve suggested that in retrospect perhaps
context: 'template'should have been the default for fragments, instead of"body"which is in fact the default. It's unclear why thecontextarg hasn't made it into generated nokogiri docs at all!It's clear to me that this is the right move, and I can't think of any problems or even backwards incompatibilites it would cause.
Anything you want to highlight for special attention from reviewers?
Confirmed new test failed without change.