Wicket
  1. Wicket
  2. WICKET-3433

Wicket parses HTML inside IE conditional comments

    Details

      Description

      I cannot use the HTML5 Boilerplate HTML with wicket. Wicket appears to parse HTML inside IE conditional comments when they appear between the doctype declaration and the <html> tag.

      The first lines of the boilerplate html look like this:

      <!doctype html>
      <Unable to render embedded object: File (--[if lt IE 7 ]> <html lang="en" class="no-js ie6"> <) not found.[endif]-->
      <Unable to render embedded object: File (--[if IE 7 ]> <html lang="en" class="no-js ie7"> <) not found.[endif]-->
      <Unable to render embedded object: File (--[if IE 8 ]> <html lang="en" class="no-js ie8"> <) not found.[endif]-->
      <Unable to render embedded object: File (--[if IE 9 ]> <html lang="en" class="no-js ie9"> <) not found.[endif]-->
      <Unable to render embedded object: File (--[if (gt IE 9)) not found.(IE)]><Unable to render embedded object: File (--> <html lang="en" class="no-js"> <) not found.-<![endif]->

      ...which causes a problem for wicket, as it thinks there are five opening <html> tags on the page.

      1. programmatic-way.tgz
        18 kB
        Martin Grigorov
      2. quickstart.zip
        4.31 MB
        Christian Lerch
      3. WICKET-3433.patch
        75 kB
        Pedro Santos
      4. SimplePageExpectedResult_12a.html
        1 kB
        Pedro Santos
      5. wicket-conditional-comments.patch
        20 kB
        Juergen Donnerstag
      6. html5boilerplate.ui.wicket.parsefail-working.zip
        33 kB
        Pedro Santos
      7. html5boilerplate.ui.wicket.parsefail.zip
        36 kB
        Alan Shaw

        Issue Links

          Activity

          Hide
          Martin Grigorov added a comment -

          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+.

          Show
          Martin Grigorov added a comment - 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+.
          Hide
          Robert Niestroj added a comment -

          Made a workaround today. This is attached to the body tag:

          WebMarkupContainer body = new WebMarkupContainer("bodyContainer"){

          @Override
          protected void onConfigure() {
          super.onConfigure();
          if (WebSession.get().getClientInfo().getProperties().isBrowserInternetExplorer())

          { add(AttributeModifier.replace("class", "ie ie" + WebSession.get().getClientInfo().getProperties().getBrowserVersionMajor() )); }

          }

          };

          Show
          Robert Niestroj added a comment - Made a workaround today. This is attached to the body tag: WebMarkupContainer body = new WebMarkupContainer("bodyContainer"){ @Override protected void onConfigure() { super.onConfigure(); if (WebSession.get().getClientInfo().getProperties().isBrowserInternetExplorer()) { add(AttributeModifier.replace("class", "ie ie" + WebSession.get().getClientInfo().getProperties().getBrowserVersionMajor() )); } } };
          Hide
          Florian Stoerkle added a comment -

          Any news here?

          Show
          Florian Stoerkle added a comment - Any news here?
          Hide
          Juergen Donnerstag added a comment -

          Microsoft removed support for Conditional Comments in IE10. See http://blogs.msdn.com/b/ie/archive/2010/04/14/same-markup-writing-cross-browser-code.aspx on MS proposed ways to solve cross browser issues.

          Show
          Juergen Donnerstag added a comment - Microsoft removed support for Conditional Comments in IE10. See http://blogs.msdn.com/b/ie/archive/2010/04/14/same-markup-writing-cross-browser-code.aspx on MS proposed ways to solve cross browser issues.
          Hide
          Martin Grigorov added a comment - - edited

          Attaching the fixed quickstart provided by Christian.

          @Christian: its your turn again. Please create a wiki page, describe the use case and paste the relevant snippets

          Show
          Martin Grigorov added a comment - - edited Attaching the fixed quickstart provided by Christian. @Christian: its your turn again. Please create a wiki page, describe the use case and paste the relevant snippets
          Hide
          Christian Lerch added a comment -

          Thanks Martin, that's gracious.
          Please find attached a very simple quickstart, that should demonstrate that setting the class attribute on the html tag via TransparentWebMarkupContainer and AttributeAppender does not work, well, at least as I understand it. Please let me know what's wrong with it.
          Thank you,
          Chris

          Show
          Christian Lerch added a comment - Thanks Martin, that's gracious. Please find attached a very simple quickstart, that should demonstrate that setting the class attribute on the html tag via TransparentWebMarkupContainer and AttributeAppender does not work, well, at least as I understand it. Please let me know what's wrong with it. Thank you, Chris
          Hide
          Igor Vaynberg added a comment -

          where are we on this?

          Show
          Igor Vaynberg added a comment - where are we on this?
          Hide
          Martin Grigorov added a comment -

          Please test the fix here with getMarkupSettings().setStripComments(true);
          This setting has some issues with IE conditional comments - see WICKET-3648.

          Show
          Martin Grigorov added a comment - Please test the fix here with getMarkupSettings().setStripComments(true); This setting has some issues with IE conditional comments - see WICKET-3648 .
          Hide
          Juergen Donnerstag added a comment -

          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

          Show
          Juergen Donnerstag added a comment - 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
          Hide
          Pedro Santos added a comment -

          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.

          Show
          Pedro Santos added a comment - 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.
          Hide
          Pedro Santos added a comment -

          Nice, I think I did that in the patch, the new filter fully handling conditional comment logic is the ConditionalCommentFilter

          Show
          Pedro Santos added a comment - Nice, I think I did that in the patch, the new filter fully handling conditional comment logic is the ConditionalCommentFilter
          Hide
          Juergen Donnerstag added a comment -

          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.

          Show
          Juergen Donnerstag added a comment - 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.
          Hide
          Pedro Santos added a comment -

          same patch for the current trunk

          Show
          Pedro Santos added a comment - same patch for the current trunk
          Hide
          Pedro Santos added a comment -

          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
          Show
          Pedro Santos added a comment - 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
          Hide
          Pedro Santos added a comment -

          Hi Juergen, the SimplePageTest#testRenderHomePage_12 is failing always, I'm attaching what I think that we should expect.

          Show
          Pedro Santos added a comment - Hi Juergen, the SimplePageTest#testRenderHomePage_12 is failing always, I'm attaching what I think that we should expect.
          Hide
          Pedro Santos added a comment - - 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.

          Show
          Pedro Santos added a comment - - 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.
          Hide
          Juergen Donnerstag added a comment -

          my working file with additional test cases

          Show
          Juergen Donnerstag added a comment - my working file with additional test cases
          Hide
          Juergen Donnerstag added a comment -

          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

          Show
          Juergen Donnerstag added a comment - 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
          Hide
          Igor Vaynberg added a comment -

          thanks for the patch Pedro, lets see if Juergen agrees.

          Show
          Igor Vaynberg added a comment - thanks for the patch Pedro, lets see if Juergen agrees.
          Hide
          Pedro Santos added a comment -

          Patch suporting the related usecase and Downlevel-revealed Conditional Comments as well

          Show
          Pedro Santos added a comment - Patch suporting the related usecase and Downlevel-revealed Conditional Comments as well
          Hide
          Pedro Santos added a comment -

          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.

          Show
          Pedro Santos added a comment - 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.
          Hide
          Alan Shaw added a comment -

          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.

          Show
          Alan Shaw added a comment - 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.
          Hide
          Martin Grigorov added a comment -

          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.

          Show
          Martin Grigorov added a comment - 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.
          Hide
          Juergen Donnerstag added a comment -

          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.

          Show
          Juergen Donnerstag added a comment - 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.
          Hide
          Alan Shaw added a comment -

          You are correct, apologies. The problem is slightly different to what I thought the problem was.

          Show
          Alan Shaw added a comment - You are correct, apologies. The problem is slightly different to what I thought the problem was.
          Hide
          Pedro Santos added a comment -

          I'm sending back your quickstart working

          Show
          Pedro Santos added a comment - I'm sending back your quickstart working
          Hide
          Pedro Santos added a comment -

          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

          Show
          Pedro Santos added a comment - 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
          Hide
          Pedro Santos added a comment -

          Close the html tag as below for now (as you suggested in the quickstart), IMO MarkupParser needs to allow your usecase.

          <Unable to render embedded object: File (--[if lt IE 7 ]> </html> <) not found.[endif]-->
          <Unable to render embedded object: File (--[if IE 7 ]> </html> <) not found.[endif]-->
          <Unable to render embedded object: File (--[if IE 8 ]> </html> <) not found.[endif]-->
          <Unable to render embedded object: File (--[if IE 9 ]> </html> <) not found.[endif]-->
          <Unable to render embedded object: File (--[if (gt IE 9)) not found.(IE)]><Unable to render embedded object: File (--> </html> <) not found.-<![endif]->

          Show
          Pedro Santos added a comment - Close the html tag as below for now (as you suggested in the quickstart), IMO MarkupParser needs to allow your usecase. < Unable to render embedded object: File (--[if lt IE 7 ]> </html> <) not found. [endif] --> < Unable to render embedded object: File (--[if IE 7 ]> </html> <) not found. [endif] --> < Unable to render embedded object: File (--[if IE 8 ]> </html> <) not found. [endif] --> < Unable to render embedded object: File (--[if IE 9 ]> </html> <) not found. [endif] --> < Unable to render embedded object: File (--[if (gt IE 9)) not found. (IE)]>< Unable to render embedded object: File (--> </html> <) not found. - <! [endif] ->
          Hide
          Alan Shaw added a comment -

          This quickstart demonstrates the issue.

          Show
          Alan Shaw added a comment - This quickstart demonstrates the issue.

            People

            • Assignee:
              Martin Grigorov
              Reporter:
              Alan Shaw
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development