fix(Readability): treat S and BDI as phrasing content - #1011
Conversation
e3eb4f3 to
888a768
Compare
gijsk
left a comment
There was a problem hiding this comment.
Thanks for the patch.
The testcase is a bit strange. We would typically add a minimal testcase to the testcases folder instead, with source and expected output.
I also think that we should include u and update the testcases accordingly (generate-testcase.js can do this for you if you delete the expected.html file first and then run it with the testcase slug as an argument). Or at least - I would like to see what changes that causes, before deciding whether any of these changes make sense. In isolation it is hard to see what this is really accomplishing.
Signed-off-by: Sai Asish Y <[email protected]>
|
Good points, thanks. Dropped the ad-hoc unit test and added a proper testcase under test-pages/phrasing-elements-s-bdi-u (source + expected regenerated with generate-testcase.js). I also added U to PHRASING_ELEMS as you suggested. The fixture makes the effect visible: without these tags in the list, the run gets shattered into separate blocks with the
|
|
Gentle nudge on this one when you get a chance. The fixture is in place (source + regenerated expected under test-pages/phrasing-elements-s-bdi-u) and Worth noting the red Community-TC run isn't from this diff: lint/prettier pass, but the mocha step dies before any test with |
Yes, but I don't understand why your PR is running into it when the last main run was fine, and I haven't had time to dig into this. |
|
Plus, both your PR and other earlier PRs for the same issue claim that |
|
The harness failure turned out to be just this branch predating ab4027a on main, which bumped the yargs/mocha dependency chain; after a local rebase the suite runs fine again. With the rebase in place hukumusume fails exactly as predicted, and regenerating it with generate-testcase.js makes the JSDOMParser variant green but leaves the jsdom variant red, because the two parsers disagree on the malformed nested p inside the u/span run in that page (JSDOMParser keeps the p-inside-p nesting while jsdom auto-closes it), so a single expected.html cannot satisfy both. I can push the rebase plus the regenerated fixture so the effect is visible in CI, or drop u from the list and keep this to s and bdi which stay green on both parsers, whichever you prefer. |
PHRASING_ELEMSis missing the HTML5 phrasing elementsSandBDI, so Readability treats them as block-level and splits paragraphs around them (e.g.<div>foo <s>bar</s> baz</div>becomes<p>foo </p><s>bar</s><p> baz</p>). This adds both toPHRASING_ELEMSwith a regression test.Towards #997. I left
Uout for now because adding it changes the extracted output of the hukumusume test page, so that one is better handled separately.