Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warning about missing </summary> #895

Closed
arrmo opened this issue Sep 27, 2020 · 9 comments
Closed

Warning about missing </summary> #895

arrmo opened this issue Sep 27, 2020 · 9 comments

Comments

@arrmo
Copy link
Contributor

arrmo commented Sep 27, 2020

Hi,

I am using the details element, and have it formatted like below (example) - need to do this to be able to format (heading style) the summary element, as

has to be the first child after
.

<details><summary><h4>1. Package / Updates Status</h4></summary></details>

But with this, tidy-html gives me the warning that,

line 17 column 10 - Warning: missing </summary> before <h4>
line 17 column 55 - Warning: discarding unexpected </summary>

And proceeds to remove /summary, as noted. But this breaks the details element 😞

Thoughts?

Thanks!

@geoffmcl
Copy link
Contributor

@arrmo thank you for your issue... you appear right...

I constructed a small sample, in_895.html, constructed from your snippet, which, by the way, passes the W3C validator... always a good sign...

Reading W3C Refs like the-summary-element, shows the content can indeed be Phrasing content, optionally intermixed with heading content...

I am surprised this has not come up before now...

In tags.c the summary is marked as a CM_BLOCK, but uses the inline parser, TY_(ParseInline), which will warn, and close, if the next node fetched is a header element... h1,h2,... then there is a cascade effect... with tidy failing the parse...

The following patch fixes that - just replace ParseInline with ParseBlock for the summary tag -

diff --git a/src/tags.c b/src/tags.c
index af2b7d9..3f9a890 100644
--- a/src/tags.c
+++ b/src/tags.c
@@ -323,7 +323,7 @@ static Dict tag_defs[] =
   { TidyTag_PROGRESS,    "progress",     VERS_ELEM_PROGRESS,    &TY_(W3CAttrsFor_PROGRESS)[0],    (CM_INLINE),                   TY_(ParseInline),    NULL           },
   { TidyTag_SECTION,     "section",      VERS_ELEM_SECTION,     &TY_(W3CAttrsFor_SECTION)[0],     (CM_BLOCK),                    TY_(ParseBlock),     NULL           },
   { TidyTag_SOURCE,      "source",       VERS_ELEM_SOURCE,      &TY_(W3CAttrsFor_SOURCE)[0],      (CM_BLOCK|CM_INLINE|CM_EMPTY), TY_(ParseBlock),     NULL           },
-  { TidyTag_SUMMARY,     "summary",      VERS_ELEM_SUMMARY,     &TY_(W3CAttrsFor_SUMMARY)[0],     (CM_BLOCK),                    TY_(ParseInline),    NULL           },
+  { TidyTag_SUMMARY,     "summary",      VERS_ELEM_SUMMARY,     &TY_(W3CAttrsFor_SUMMARY)[0],     (CM_BLOCK),                    TY_(ParseBlock),     NULL           },
   { TidyTag_TEMPLATE,    "template",     VERS_ELEM_TEMPLATE,    &TY_(W3CAttrsFor_TEMPLATE)[0],    (CM_BLOCK),                    TY_(ParseBlock),     NULL           },
   { TidyTag_TIME,        "time",         VERS_ELEM_TIME,        &TY_(W3CAttrsFor_TIME)[0],        (CM_INLINE),                   TY_(ParseInline),    NULL           },
   { TidyTag_TRACK,       "track",        VERS_ELEM_TRACK,       &TY_(W3CAttrsFor_TRACK)[0],       (CM_BLOCK|CM_EMPTY),           TY_(ParseBlock),     NULL           },

Now it has been that way ever since the introduction of tags.c into the repo, commit b92d7aa, circa 2011... so this should be looked at closely...

Still to fully test that, to see if there are any other adverse, regressive, effects... it can not be said often enough, changing one small thing in tidy code can have wide reaching effects... or not... but must be fully tested

Meantime, look forward to futher feedback, comments, patches, or PR... and testing... thanks...

@arrmo
Copy link
Contributor Author

arrmo commented Sep 27, 2020

Thanks for digging in to this @geoffmcl! I was expecting (hoping?) you to tell me that I was just being stupid ... 🤣.

I am using the "stock" Ubuntu version of tidy-html, but I have no issue pulling a local version and using that. I think that's what you're thinking ... make the changed noted above, test it out?

Thanks again.

@arrmo
Copy link
Contributor Author

arrmo commented Sep 28, 2020

FYI, took a guess I had your meaning right => made the change, rebuilt and ran. Life is good 😄.

Thanks!

@geoffmcl
Copy link
Contributor

@arrmo your welcome...

I have already run the modified tidy through the regression checks, and nothing pops... but that is just because we have no <details><summary><h4>1... sample(s)... maybe one should be added...

And if you have made the change, in a cloned tidy fork, probably in a new branch, like issue-895, you could present a PR, to help fix the upstream granddaddy tidy... thanks...

Cheers.

@arrmo
Copy link
Contributor Author

arrmo commented Sep 28, 2020

you could present a PR, to help fix the upstream granddaddy tidy

Sure, will do - will submit it tonight, NP!

maybe one should be added

I assume you're taking that one? 😄. Thanks!

@arrmo
Copy link
Contributor Author

arrmo commented Sep 29, 2020

Done! PR #897

Thanks!

@geoffmcl
Copy link
Contributor

@arrmo thank you for the PR #897... I will try to get to it soonest...

Meantime, could I ask you to make 2 small additional commits, to be added to the PR...

  1. In doing the research for this bug I noted a duplicated define - please remove one VERS_ELEM_KEYGEN -
diff --git a/src/tags.c b/src/tags.c
index af2b7d9..0908273 100644
--- a/src/tags.c
+++ b/src/tags.c
@@ -146,7 +146,6 @@ static CheckAttribs CheckHTML;
 #define VERS_ELEM_MAIN       (xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|HT50|XH50)
 #define VERS_ELEM_MARK       (xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|HT50|XH50)
 #define VERS_ELEM_MENUITEM   (xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|HT50|XH50)
-#define VERS_ELEM_KEYGEN     (xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|HT50|XH50)
 #define VERS_ELEM_METER      (xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|HT50|XH50)
 #define VERS_ELEM_NAV        (xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|HT50|XH50)
 #define VERS_ELEM_OUTPUT     (xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|xxxx|HT50|XH50)
  1. Where possible I like to note the issue number on the change made... helps with the history, remembering, why... like add /* Is. #895 */ to the line... like you see on a few others, in this BIG tag_defs table -
+  { TidyTag_SUMMARY,     "summary",      VERS_ELEM_SUMMARY,     &TY_(W3CAttrsFor_SUMMARY)[0],     (CM_BLOCK),                    TY_(ParseBlock),     NULL           }, /* Is. #895 */

Or we can try to remember to do these, when the actual merge is done...

And granddaddy tidy thanks you... ;=))

@geoffmcl geoffmcl linked a pull request Sep 29, 2020 that will close this issue
@geoffmcl geoffmcl removed a link to a pull request Sep 29, 2020
@arrmo
Copy link
Contributor Author

arrmo commented Sep 29, 2020

NP, can do - will add these in! Just make them another commit under this PR, right?

@arrmo
Copy link
Contributor Author

arrmo commented Oct 1, 2020

Done! Updates added to the commit 😄.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants