Closed
Bug 932844
Opened 11 years ago
Closed 11 years ago
Allow unclosed <li>, <tr>, <td> and <p>
Categories
(Webmaker Graveyard :: Thimble, defect)
Webmaker Graveyard
Thimble
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bruant.d, Assigned: michiel)
References
Details
Attachments
(2 files)
<!doctype html>
<html>
<head>
<meta chaset="utf-8">
<title>votre superbe page web créée le mer. 30 oct. 2013 13:01</title>
</head>
<body>
<h1>Yo, Guillaume</h1>
<ul>
<li> yo
</ul>
</body>
</html>
Currently, with this snippet, Thimble emits a warning saying the </ul> doesn't match the <li>. This is true, but doesn't matter in HTML. <li>s don't need to be closed. Neither do <tr> and <td>.
That's a style of HTML authoring I and others (like Paul Irish [1]) use.
[1] http://dl.dropboxusercontent.com/u/39519/talks/html5-foundation/index.html#17
Assignee: nobody → dchen.xd
Status: NEW → ASSIGNED
Flags: needinfo?(pomax)
added <p> as additional element that has "doesn't necessarily require closing tags in certain cases" rules. Also note that any work on this will need to be accompanied by a w3c or whatwg spec that explains the rules for these elements being left unclosed.
Summary: Allow unclosed <li> <tr> and <td> → Allow unclosed <li>, <tr>, <td> and <p>
|
||
Comment 3•11 years ago
|
||
Please review if the solution is ok, then continue to add new rule comments.
Attachment #8335741 -
Flags: review?(pomax)
one thing missing that will be very important to add is the official specification that says which elements may remain "open" when followed by which other elements. Without that specification, any implementation will be based on guesses rather than correct guidelines.
|
|
||
Comment 5•11 years ago
|
||
(In reply to Mike "Pomax" Kamermans [:pomax] from comment #4)
> one thing missing that will be very important to add is the official
> specification that says which elements may remain "open" when followed by
> which other elements. Without that specification, any implementation will be
> based on guesses rather than correct guidelines.
modify the existing spec to cover the both cases, or generate new spec for optional-omit-endTag case and leave the existing one for the normal case.
and if it's the latter, where to add it?
Flags: needinfo?(pomax)
no, I mean there is official w3c specifications on which elements are allowed to stay unclosed under which conditions. We need to point to that official spec because we need slowparse to do the right thing, rather than implement something that might seem reasonable, but isn't according to the HTML5 rules. We need to find those specs, and point to them in the code where we define the behaviour.
Flags: needinfo?(pomax)
|
|
||
Comment 7•11 years ago
|
||
let's take the p tag for example. In the spec of "http://www.w3.org/TR/html-markup/p.html", there is Tag omission definition:
"Tag omission
A p element must have a start tag.
A p element’s end tag may be omitted if the p element is immediately followed by an address, article, aside, blockquote, dir, div, dl, fieldset, footer, form, h1, h2, h3, h4, h5, h6, header, hr, menu, nav, ol, p, pre, section, table, or ul element, or if there is no more content in the parent element and the parent element is not an a element."
1) Then in the code when meet <p> ... <???> case, we need to check if the following tag are one of those listed above?
2) As slowparse.js only trigger errors, and these cases are not errors, then "point to them" means did something in js/fc/help.js?
Flags: needinfo?(pomax)
1) Exactly. Our code should look at what the currently open tag is, and whether a block-level tag is then seen that is not in the set {address, article, aside, blockquote, dir, div, dl, fieldset, footer, form, h1, h2, h3, h4, h5, h6, header, hr, menu, nav, ol, p, pre, section, table, ul}.
Note that things like <span>, <img>, <a>, <em> etc. (basically: inline elements) should be disregarded under these rules, since they're legal <p> content already.
2) If the <p> is followed by a block level element that is not in the "allowed" set, then that is definitely an error and slowparse should throw up the error dialog.
Flags: needinfo?(pomax)
|
|
||
Comment 9•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
Updated solution follow Tag omission rules in HTML5 spec.
|
Assignee | |
Comment 10•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
comments in the PR
Attachment #8335741 -
Flags: review?(pomax) → review-
|
|
||
Updated•11 years ago
|
Attachment #8335741 -
Flags: review- → review?(pomax)
|
|
Assignee | |
Comment 11•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
comments in PR
Attachment #8335741 -
Flags: review?(pomax) → review-
|
|
||
Updated•11 years ago
|
Attachment #8335741 -
Flags: review- → review?(pomax)
|
|
Assignee | |
Comment 12•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
comments in PR -- we can either add the remaining elements with optional closing tags in a follow-up bug (in which case you'll have to file that bug and mark it as "see <this bug>" using the 'see' field) or we can make it part of this bug and just do all of them in one go.
Attachment #8335741 -
Flags: review?(pomax) → review-
|
|
||
Updated•11 years ago
|
Attachment #8335741 -
Flags: review- → review?(pomax)
|
|
||
Comment 13•11 years ago
|
||
(In reply to Mike "Pomax" Kamermans [:pomax] from comment #12)
> Comment on attachment 8335741 [details] [review]
> https://github.com/mozilla/thimble.webmaker.org/pull/296
>
> comments in PR -- we can either add the remaining elements with optional
> closing tags in a follow-up bug (in which case you'll have to file that bug
> and mark it as "see <this bug>" using the 'see' field) or we can make it
> part of this bug and just do all of them in one go.
I would like to focus on p, li, th, td for this patch, while leave the rest for a new ticket.
|
|
Assignee | |
Comment 14•11 years ago
|
||
works for me, please file a followup ticket and we can link this one and the followup by marking the follow up as "see 932844" (using the "see" functionality for bugs)
|
|
Assignee | |
Comment 15•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
I tried this with the following HTML code:
<html>
<body>
<p>test</p>
<p>test 2</p>
</body>
</html>
but the new code causes an error to be flagged, saying that "/body" does not close "html". Somehow it doesn't close off the right tags now for optional-but-closed-anyway tags.
Attachment #8335741 -
Flags: review?(pomax) → review-
|
|
||
Comment 16•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
fixed
Attachment #8335741 -
Flags: review- → review?(pomax)
|
|
Assignee | |
Comment 17•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
looks like it fixed that error, so that's progress. However, for <li> I get errors when doing:
<html>
<body>
<ol>
<li>x
</ol>
</body>
</html>
slowparse now signals that </ol> does not close <li>, which is true but also permitted, so that'll also need to be fixed before we can land this. I tested
<table>
<tr><td>b</tr>
</table>
and that has the same problem, where it signals that the <td> element is getting closed off by a </tr> in error, rather than this being correct use of HTML.
Attachment #8335741 -
Flags: review?(pomax) → review-
|
|
||
Comment 18•11 years ago
|
||
(In reply to Mike "Pomax" Kamermans [:pomax] from comment #17)
> Comment on attachment 8335741 [details] [review]
> https://github.com/mozilla/thimble.webmaker.org/pull/296
>
> looks like it fixed that error, so that's progress. However, for <li> I get
> errors when doing:
>
> <html>
> <body>
> <ol>
> <li>x
> </ol>
> </body>
> </html>
>
> slowparse now signals that </ol> does not close <li>, which is true but also
> permitted, so that'll also need to be fixed before we can land this. I tested
>
> <table>
> <tr><td>b</tr>
> </table>
>
> and that has the same problem, where it signals that the <td> element is
> getting closed off by a </tr> in error, rather than this being correct use
> of HTML.
According to the Standards: A xxx element's end tag may be omitted if the xxx element is immediately followed by an yyy,zzz,... or if there is no more content in the parent element (and the parent element is not an a element in the case of p).
Then both cases above should raise warnings, while the following cases should not:
<html>
<body>
<ol>
<li>x
<li>
</ol>
</body>
</html>
<table>
<tr>
<td>
</tr>
</table>
|
|
||
Updated•11 years ago
|
Attachment #8335741 -
Flags: review- → review?(pomax)
|
|
Assignee | |
Comment 19•11 years ago
|
||
text node do not count as element for the purpose of "following". running the following code through http://validator.w3.org/check will pass, for instance:
<!doctype html>
<html>
<head>
<meta charset="UTF-8">
<title>a</title>
</head>
<body>
<ol>
<li>x
</ol>
</body>
</html>
|
|
Assignee | |
Comment 20•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
based on http://validator.w3.org/check results, the original points on <td>...</tr> and <li>...</ol> are valid (#text nodes are not considered elements and do not factor into the rules for optional closing tags). We will need to make sure the code is well behaved for these validation-passing cases.
Attachment #8335741 -
Flags: review?(pomax) → review-
|
|
||
Comment 21•11 years ago
|
||
that simplify the 'foresee' function ...
|
|
||
Updated•11 years ago
|
Attachment #8335741 -
Flags: review- → review?(pomax)
Attachment #8335741 -
Attachment filename: file_932844.txt → 296
|
|
Assignee | |
Comment 22•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
We should move most of that functionality into the stream prototype instead.
Attachment #8335741 -
Flags: review?(pomax) → review-
|
||
Comment 23•11 years ago
|
||
Mark this issue as closed when this bug lands
|
|
||
Comment 24•11 years ago
|
||
(In reply to Mike "Pomax" Kamermans [:pomax] from comment #22)
> Comment on attachment 8335741 [details] [review]
> https://github.com/mozilla/thimble.webmaker.org/pull/296
>
> We should move most of that functionality into the stream prototype instead.
The regular expression solution won't work, in the following case
<body>
<p>...<p>...
</body>
it will trigger the popElement() on the first <p>, as stream found the closest close tag from the current position is </body> which meet with the current parentTag <body>, but as it also meets the criteria of activeTag(which performed popElement() already), then after two times of popElement(), the parentTag will be documentFragment.
When the stream comes to the second <p>, it won't popElement() as the parentTag is documentFragment and never paired with the closest close tag </body>, then there is always a warning raised for unmatched <p> and </body>.
Flags: needinfo?(pomax)
|
|
||
Comment 25•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
make it works
Attachment #8335741 -
Flags: review- → review?(pomax)
|
|
Assignee | |
Comment 26•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
the code is not clear enough in what it does, and what it stores in which variables.
Attachment #8335741 -
Flags: review?(pomax) → review-
|
|
||
Comment 28•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
Please see my reply to comments on the last commit for the function findNext() and isException().
Attachment #8335741 -
Flags: review- → review?(pomax)
|
|
Assignee | |
Comment 29•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
If I use the following HTML code, I get errors:
<body>
<p>Make something amazing with the web<a href="lol">ac</a>
<p>and more.
</body>
but this works fine:
<body>
<p>Make something amazing with the web
<p>and more.
</body>
Attachment #8335741 -
Flags: review?(pomax) → review-
|
|
||
Comment 30•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
In case of following HTML code, there will still be errors for unmatched <p> (the first <p>) and </body>, as the first <p> followed by <a> broken the Tag omission rules, but the second <p> is fine for the rules.
<body>
<p>Make something amazing with the web<a href="lol">ac</a>
<p>and more.
</body>
If we change <a> to one of the enable ommitable tags like <div>, no errors at all.
<body>
<p>Make something amazing with the web<div>ac</div>
<p>and more.
</body>
Attachment #8335741 -
Flags: review- → review?(pomax)
|
|
Assignee | |
Comment 31•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
wow, missed the fact that <a> doesn't count. I think we're good, then, although it might be worth rebasing, and then adding unit tests. I have reworked unit testing in https://github.com/pomax/thimble.webmaker.org/tree/bug950197 (for https://bugzilla.mozilla.org/show_bug.cgi?id=950197), we can probably extend the slowparse tests with several cases that have been used to validate this bug.
If you can rebase (with squashed commits) then I can try to land the unit testing and we can add the necessary tests to make sure slowparse doesn't suffer any regression failures with this patch (I doubt it will, good work on the patch)
Attachment #8335741 -
Flags: review?(pomax) → review+
Flags: needinfo?(dchen.xd)
|
|
||
Comment 32•11 years ago
|
||
Comment on attachment 8335741 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/296
done!
Attachment #8335741 -
Flags: review+ → review?(pomax)
Flags: needinfo?(dchen.xd)
|
|
Assignee | |
Comment 33•11 years ago
|
||
I landed the unit tests into master, and tried a rebase which went off without a hitch, so we can add unit tests to the ./public/friendlycode/vendor/slowparse/test/test-slowparse.js file to test these functions. The good news: no tests fail that weren't already failing! (svg and implicitly self-closing elements like img and br have some issues due to jsdom parsing slightly different from a true browser).
I added these two tests, for instance, in my local rebased branch, and they both pass (hurray!)
test("parsing elements with optional close tags: <p>", function() {
var html = '<div><p>text\n<p>more text</div>';
var result = Slowparse.HTML(document, html);
ok(!result.error, "no error on omitted </p>");
});
test("fail parsing elements with optional close tags: <p>", function() {
var html = '<div><p>text\n<a>more text</a></div>';
var result = Slowparse.HTML(document, html);
var expected = {
type: 'MISMATCHED_CLOSE_TAG',
openTag: {
name: 'p',
start: 5,
end: 8
},
closeTag: {
name: 'div',
start: 29,
end: 34
},
cursor: 29
};
equal(result.error, expected, "bad omission error for <p>");
});
If you want I can add those tests in, but if you feel like adding them in that's fine too. Let me know?
Flags: needinfo?(dchen.xd)
Attachment #8335741 -
Flags: review?(pomax) → review+
|
|
||
Comment 34•11 years ago
|
||
Please go ahead to add them in, and hope it be landed soon.
Flags: needinfo?(dchen.xd)
|
||
Comment 35•11 years ago
|
||
Commits pushed to master at https://github.com/mozilla/thimble.webmaker.org
https://github.com/mozilla/thimble.webmaker.org/commit/86acaa7732a3391be2c0e49e8316fdefd10dc0d0
Implement tag omission rules for bug932844
https://github.com/mozilla/thimble.webmaker.org/commit/e58e39d9ff00dae76c1695eaa61401a80531708a
Merge pull request #340 from Pomax/bug932844
Implement tag omission rules for bug932844
|
|
Assignee | |
Comment 36•11 years ago
|
||
great win
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•