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

Details open, Value added to Attribute #925

Closed
arrmo opened this issue Mar 24, 2021 · 11 comments
Closed

Details open, Value added to Attribute #925

arrmo opened this issue Mar 24, 2021 · 11 comments
Labels

Comments

@arrmo
Copy link
Contributor

arrmo commented Mar 24, 2021

Hi,

Not sure this is a big issue, but to let you know. I have a details element, as below (before tidying),

<details open>

Formatted as it should be, more info here. But, after tidying,

<DETAILS OPEN="">

Thinking the value (="") shouldn't be added?

Thanks!

@geoffmcl
Copy link
Contributor

@arrmo, thank you for the issue... but need docs...

Yes, tidy does NOT treat the attribute open as a boolean type... so will add ="" when Pretty Printing, if blank... see code ...

IIF it is true that open is ALWAYS a boolean attribute - need W3C documentation - I do not think w3schools is affiliated to the W3C- then the patch is simple -

diff --git a/src/attrs.c b/src/attrs.c
index 981235f..5d8a614 100644
--- a/src/attrs.c
+++ b/src/attrs.c
@@ -318,7 +318,7 @@ static const Attribute attribute_defs [] =
   { TidyAttr_MEDIAGROUP,              "mediagroup",              CH_PCDATA   },
   { TidyAttr_MIN,                     "min",                     CH_PCDATA   },
   { TidyAttr_NOVALIDATE,              "novalidate",              CH_PCDATA   },
-  { TidyAttr_OPEN,                    "open",                    CH_PCDATA   },
+  { TidyAttr_OPEN,                    "open",                    CH_BOOL     },
   { TidyAttr_OPTIMUM,                 "optimum",                 CH_PCDATA   },
   { TidyAttr_OnABORT,                 "onabort",                 CH_PCDATA   },
   { TidyAttr_OnAFTERPRINT,            "onafterprint",            CH_PCDATA   },

Advise on the documentation, and it can be applied... thanks...

Pending W3C verification, marking this as Technical Discussion meantime...

@arrmo
Copy link
Contributor Author

arrmo commented Mar 24, 2021

That makes complete sense - thanks for the explanation! Does this work for you?

@geoffmcl
Copy link
Contributor

@arrmo, thank you for the Mozilla link... yes, but...

While most HTML documentation helps, given tidy's heritage, what we seek most is the W3C recs, and more recently the WHATWG specs...

Some W3C info on the <details> element can be seen here, and it mentions that it can take an open attribute... and note they use the form open="open", so open is definitely a Boolean Attribute...

Tidy presently marks some 21 attributes as CH_BOOL... I guess I was looking for a simple list, which may not exist... but I am convinced that open should be added to that list... so marking this as a bug... baring any negative feedback...

Regrettably, open="false", nor say open="XXXX", are not flagged with a warning, although the latter will be silently changed to open="xxxx", if def. config of --lower-literals yes, ... but that/these could be separate, new issue(s), once this first issue is closed...

Briefly looking at it, in Debug mode, and it seems the CheckBool service, needs to be expanded, to something like the CheckIs service, to fully conform to the Boolean Attribute spec...

Will try to get to setting up a Pull Request to allow further testing, comments, etc, unless someone beats me to it... and run the new exe through the regression tests, to see if anything pops... I found nothing, using my tidy-5.7.45.I925.exe, but appreciated if others could confirm... thanks...

@arrmo
Copy link
Contributor Author

arrmo commented Mar 26, 2021

Thank you! Let me know if there is anything I can do to help. Really appreciate the quick (and thorough) investigation.

@geoffmcl
Copy link
Contributor

@arrmo, to help, well you, or others, could create a PR... the suggested steps, are as follows...

  1. Fork the repo # I note you have already done this...
  2. Bring that fork fully up-to-date # your repo shows you are behind next, so
  3. Add an 'upstream' remote to your repo - ONCE only
  4. $ git remote add upstream git@github.com:htacg/tidy-html5.git
    $ git remote -v # check, and you should see something like -
origin  git@github.com:geoffmcl/tidy-fork.git (fetch)
origin  git@github.com:geoffmcl/tidy-fork.git (push)
upstream        git@github.com:htacg/tidy-html5.git (fetch)
upstream        git@github.com:htacg/tidy-html5.git (push)
  1. $ git fetch upstream # get the main repo updates...
  2. $ git rebase upstream/next # fix any conflicts, hopefully none...
  3. $ git push # update your fork - should now read 'even with'...
  4. $ git checkout -b issue-925 # create, and switch to new branch
  5. Apply the above patch, gen tidy, test, commit, and push the change(s)
  6. $ git push -u origin issue-925 # add branch to your repo
  7. Create the PR... github should prompt this...

Advise if you need further information... thanks...

@arrmo
Copy link
Contributor Author

arrmo commented Mar 28, 2021

Sure, NP - thanks for the pointers! Changed, built, tested ... and working 😄.

Done - created PR #932

Thanks again.

@geoffmcl
Copy link
Contributor

geoffmcl commented Apr 1, 2021

@arrmo, thank you for the PR #932...

As an after thought - I like to mark such patches with, like /* Is. #925 PR #932 */, or something, to show their history - but I can try to remember to do this when I do the merge... so no problem...

Will try to get to the merge soonest... remind me if it is taking too long...

Again, thanks for the issue and fix...

@arrmo
Copy link
Contributor Author

arrmo commented Apr 1, 2021

I don't mind adding that note (mark), NP at all. Where to add it at? Don't want to mess up your list / table formatting.

Thanks!

@geoffmcl
Copy link
Contributor

geoffmcl commented Apr 5, 2021

@arrmo, I was thinking something like, preserving table spacing -

+  { TidyAttr_OPEN,                    "open",                    CH_BOOL     }, /* Is. #925 PR #932 */ 

But no prob...

@arrmo
Copy link
Contributor Author

arrmo commented Apr 6, 2021

Updated! Amended the commit, no issue adding this.

Thanks!

geoffmcl pushed a commit that referenced this issue Apr 15, 2021
@geoffmcl
Copy link
Contributor

@arrmo done the PR #932 merge... thanks for fix...

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

No branches or pull requests

2 participants