Wicket chunks the conditional comment and parse only the markup inside it. Closing the html the as many times it was opened (as you suggested ), and wrapping then inside the correspondent conditional comments (as I'm suggesting) generate valid html to browser, I just tested in http://validator.w3.org/check

Pedro Santos
added a comment - 08/Feb/11 10:41 Wicket chunks the conditional comment and parse only the markup inside it. Closing the html the as many times it was opened (as you suggested ), and wrapping then inside the correspondent conditional comments (as I'm suggesting) generate valid html to browser, I just tested in http://validator.w3.org/check

I find it difficult to imagine how Wicket could handle possibilities. Pedro is right in how Wicket will see the markup once loaded. It'll be like
<html>
<html>
<html>
<html>
<html>

Since Wicket validates that for every open tag, there is a close tag as well, Wicket complains about the missing </html> tags in your original example. And for the browser to be happy you need add what Pedro has suggested.

Now what would Wicket need to do in order to understand that use case: Wicket needs to validate that each body of the conditional leaves the very same open tag behind. In theory that could be done though a bit of work.

But it's not as simple as one my think. E.g. what is the rule that 2+ conditional tag belong to a group? I'd guess only "empty" raw markup between <Unable to render embedded object: File ([endif]> and the next <) not found.--[if...>. But if the next conditional leaves different open tags behind, it should not be part of that group, right? => increase in complexity
<Unable to render embedded object: File (--[if ..IE7...]> body AAAA <) not found.[endif]>
<Unable to render embedded object: File (--[if ..IE7...]> body BBBB by purpose <) not found.[endif]>

If your conditional body for whatever reason gets a Component attached (either by the user or automatically by Wicket), than you'll need all the close tags. Otherwise the render process will fail.

May be we can't handle all possible scenarios, but improve a bit. An additional IMarkupFilter would be needed. I'm happy to review it if someone would provide the code.

Juergen Donnerstag
added a comment - 12/Feb/11 10:35 I find it difficult to imagine how Wicket could handle possibilities. Pedro is right in how Wicket will see the markup once loaded. It'll be like
<html>
<html>
<html>
<html>
<html>
Since Wicket validates that for every open tag, there is a close tag as well, Wicket complains about the missing </html> tags in your original example. And for the browser to be happy you need add what Pedro has suggested.
Now what would Wicket need to do in order to understand that use case: Wicket needs to validate that each body of the conditional leaves the very same open tag behind. In theory that could be done though a bit of work.
But it's not as simple as one my think. E.g. what is the rule that 2+ conditional tag belong to a group? I'd guess only "empty" raw markup between < Unable to render embedded object: File ([endif]> and the next <) not found. --[if...>. But if the next conditional leaves different open tags behind, it should not be part of that group, right? => increase in complexity
< Unable to render embedded object: File (--[if ..IE7...]> body AAAA <) not found. [endif] >
< Unable to render embedded object: File (--[if ..IE7...]> body BBBB by purpose <) not found. [endif] >
If your conditional body for whatever reason gets a Component attached (either by the user or automatically by Wicket), than you'll need all the close tags. Otherwise the render process will fail.
May be we can't handle all possible scenarios, but improve a bit. An additional IMarkupFilter would be needed. I'm happy to review it if someone would provide the code.

I think we should not do anything for this issue.
The solution/workaround is to use the same checks for the closing (</html>) elements, as in Pedro's comment from 08/Feb/11 1:22 AM.

HTML5 Boilerplate uses IE comments because it is more easy for them, but most of the JS libraries use onDomReady to assign the classes - see http://davidwalsh.name/mootools-css for the MooTools and Dojo approaches.

Adding a new markup filter will just make things more complex than needed for no big gain.

Martin Grigorov
added a comment - 14/Feb/11 09:59 I think we should not do anything for this issue.
The solution/workaround is to use the same checks for the closing (</html>) elements, as in Pedro's comment from 08/Feb/11 1:22 AM.
HTML5 Boilerplate uses IE comments because it is more easy for them, but most of the JS libraries use onDomReady to assign the classes - see http://davidwalsh.name/mootools-css for the MooTools and Dojo approaches.
Adding a new markup filter will just make things more complex than needed for no big gain.

Alan Shaw
added a comment - 14/Feb/11 10:06 HTML5 boilerplate uses IE conditional comments so that you can still apply IE specific styles to browsers that don't have JavaScript enabled.
Wicket has no business in IE conditional comments. Wicket is not IE.

The big problem is that open some tag N times inside conditional comments an close it once gives us valid markup, I tested on w3c checker, and every browser understands it, I tested on Chrome, FireFox, Opera and Safari.

Pedro Santos
added a comment - 14/Feb/11 10:42 The big problem is that open some tag N times inside conditional comments an close it once gives us valid markup, I tested on w3c checker, and every browser understands it, I tested on Chrome, FireFox, Opera and Safari.

you "fixed" a test case by properly closing the <link> tag. Though it is proper XHTML now, HTML and most browsers did allow that inaccuracy, and we supported it as well. Introducing your patch means it no long works if markup contains conditional comments => That will be a difficult to find problem for user I guess

since you are skipping all markup elements in HtmlHandler between the comment open and the close tag, no wicket components (manual or automatic ones) are allowed anymore in the markup embraced by the conditional. Something we used to support (see SimplePage_12.html). <link href="some.css"> which are common in between conditional comment e.g. will be affected => if we want to go down that road, than we need to properly communicate that limitation to our users.

Juergen Donnerstag
added a comment - 19/Feb/11 10:39 Hi Pedro,
few comments to your patch
you "fixed" a test case by properly closing the <link> tag. Though it is proper XHTML now, HTML and most browsers did allow that inaccuracy, and we supported it as well. Introducing your patch means it no long works if markup contains conditional comments => That will be a difficult to find problem for user I guess
since you are skipping all markup elements in HtmlHandler between the comment open and the close tag, no wicket components (manual or automatic ones) are allowed anymore in the markup embraced by the conditional. Something we used to support (see SimplePage_12.html). <link href="some.css"> which are common in between conditional comment e.g. will be affected => if we want to go down that road, than we need to properly communicate that limitation to our users.
Juergen

Hi Juergen thank for the review. The idea of skip repeated markup inside the conditional comments has those drawbacks you pointed, we would have to skip only repeated not closed or not opend tags inside different conditional comments to solve it. I think it can be done nicely, let me come back with another path.

I didn't fixed the MarkupParserTest by closing the link tag at MarkupParserTest_9.html because it wasn't failing, I just reverted it here to maintain it testing the tolerance of non close tags, thanks for noticed and test cases.

Pedro Santos
added a comment - 19/Feb/11 20:34 - edited Hi Juergen thank for the review. The idea of skip repeated markup inside the conditional comments has those drawbacks you pointed, we would have to skip only repeated not closed or not opend tags inside different conditional comments to solve it. I think it can be done nicely, let me come back with another path.
I didn't fixed the MarkupParserTest by closing the link tag at MarkupParserTest_9.html because it wasn't failing, I just reverted it here to maintain it testing the tolerance of non close tags, thanks for noticed and test cases.

Pedro Santos
added a comment - 22/Feb/11 00:10 Hi Juergen, I'm attaching the patch with a new filter to skip tags inside conditional comments as you suggested. Below some explains about changed classes in the patch:
AbstractMarkupParser and WicketRemoveTagHandler: changed to support early test of tags on ConditionalCommentFilter
TagStack: html tags open close balance control moved out from HtmlHandler to be used by ConditionalCommentFilter also
ConditionalComment: class created to enable parse filters to control this MarkupElement
XmlPullParser: changed to support the new markup element type - ConditionalComment
ConditionalCommentFilter: new filter in the chain to skip mismatch tags inside conditional comments
RootMarkupFilter: changed to return the new comment type to chained filters
WicketTagIdentifier: simple change to avoid class cast exceptions

Juergen Donnerstag
added a comment - 20/Mar/11 12:58 Hi Pedro,
I'm not yet there, but I'd like to forward every tag (including comments) to markup filters, so that conditional comments can be fully handled by a markup filter.

Hi Juergen, in the last commit you broke the HtmlHandler validation by moving it to postProcess (bug exposed by MarkupParserTest#testErrorOnMismatchTag in the patch). The problem is that postProcess is not invoked for all filters in the chain since its first filter - StyleAndScriptIdentifier - is not forwarding the call.
I understand your idea of moving the conditional comments handle code to a filter in the chain, I think it is OK and it is why I moved it to there in my second patch I guess.
In your last commit you also made a relatively big change in the parser by not considering the XmlTag as a MarkupElement. I also agree with the change, and merged it into my patch. But I ask you to hold on further changes in the parser until we close this ticket, because it's getting hard to follow you in the changes.
The Wicket parser is an hostile land for newcomers, I already saw a single innocent line invoking a method in the wrong sequence break more than 500 test cases.
The new attached patch fix this issue, the WICKET-3500, and the on described before.

Pedro Santos
added a comment - 21/Mar/11 01:03 Hi Juergen, in the last commit you broke the HtmlHandler validation by moving it to postProcess (bug exposed by MarkupParserTest#testErrorOnMismatchTag in the patch). The problem is that postProcess is not invoked for all filters in the chain since its first filter - StyleAndScriptIdentifier - is not forwarding the call.
I understand your idea of moving the conditional comments handle code to a filter in the chain, I think it is OK and it is why I moved it to there in my second patch I guess.
In your last commit you also made a relatively big change in the parser by not considering the XmlTag as a MarkupElement. I also agree with the change, and merged it into my patch. But I ask you to hold on further changes in the parser until we close this ticket, because it's getting hard to follow you in the changes.
The Wicket parser is an hostile land for newcomers, I already saw a single innocent line invoking a method in the wrong sequence break more than 500 test cases.
The new attached patch fix this issue, the WICKET-3500 , and the on described before.

Juergen Donnerstag
added a comment - 21/Mar/11 08:37 Hi Pedro,
I didn't mean to give you a hard time. That's why I wrote my last comment. Though re-reading it, it says nothing about "hold off until I'm finished". Sorry for that.
I took a look at you patch. I think a little bit more tweaking to the framework - compared to the dedicated comment handling code - is necessary to make the markup filters more versatile.
Juergen

Martin Grigorov
added a comment - 14/Aug/13 12:28 Closing as "Won't fix".
The workaround in programmatic-way.tgz (simple version can be seen in Robert's comment) is better solution than what h5boilerplate provides. The Wicket way will work even in IE10+.