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

Fix handling of percent symbols in CheckLength validation routine #910

Closed
cqcallaw opened this issue Nov 20, 2020 · 7 comments · Fixed by #912
Closed

Fix handling of percent symbols in CheckLength validation routine #910

cqcallaw opened this issue Nov 20, 2020 · 7 comments · Fixed by #912
Labels

Comments

@cqcallaw
Copy link
Contributor

Per discussion on another issue, the string 123%45%6 isn't a valid length value, but wouldn't be flagged as invalid by tidy.

@cqcallaw
Copy link
Contributor Author

I pushed a fix to https://github.com/cqcallaw/tidy-html5/tree/checklength-percent-fix, but I'm having trouble verifying it; when I run the fresh build of tidy through a debugger, I see the width="123%45%6" flagged as invalid, but when I run from the command line, I only see the other warnings which are addressed in #903

@geoffmcl
Copy link
Contributor

@cqcallaw thanks for adding this issue...

I just pulled, and checked out your checklength-percent-fix branch, and built it.

Running the debug version in the MSVC debugger, I see the very verbose -

After : ----|----|----|----|----|----|----|----|----|----|----|----|----|----|HT50|XH50
After : ----|----|----|----|----|----|----|----|----|----|----|----|----|----|----|----
line 8 column 9 - Warning: <svg> attribute "width" has invalid value "123%45%6"
All nodes BEFORE clean and repair
Root
 DocType
 StartTag html   lang="en"
  StartTag head
   StartTag meta   charset="UTF-8"
   StartTag title
    Text   (17) '#903 - SVG sample'
  StartTag body
   StartTag svg   xmlns="http://www.w3.org/2000/svg" color="green" width="123%45%6" height="24%" stroke-width="2" stroke-linecap="butt" stroke-linejoin="round" stroke-miterlimit="4" stroke-opacity="1" stroke-dasharray="none" fill="currentColor" stroke="currentColor"
    StartEnd path   d="m 0.75,12.0 11.25,-9.0 4.5,3.375 0.0,-1.5 2.25,0.0 0.0,3.375 4.5,3.75 -2.25,0.0 0.0,9.0 -5.625,0.0 0.0,-7.5 -6.75,0.0 0.0,7.5 -5.625,0.0 0.0,-9.0 z"
line 8 column 9 - Warning: <svg> proprietary attribute "color"
line 8 column 9 - Warning: <svg> proprietary attribute "stroke-width"
etc...

And the same warning, running the release config from the command line...

D:\UTILS\tidy\tidy-cqc\build\temp-fix>release\tidy -q temp.html
line 8 column 9 - Warning: <svg> attribute "width" has invalid value "123%45%6"
line 8 column 9 - Warning: <svg> proprietary attribute "color"
line 8 column 9 - Warning: <svg> proprietary attribute "stroke-width"
etc...

Maybe you ran the wrong version from the command line? I often add -DTIDY_RC_NUMBER=fix123 to the cmake options, so I can do a release\tidy -v, and clearly see HTML Tidy for Windows version 5.7.35.fix123 to be very sure... With umpteen versions of tidy in my system, I make this mistake all the time...

So I find no problem with the patch... it is probably not the way I would code the fix... but to me it does the job, no problems... thanks...

@geoffmcl geoffmcl added the Bug label Nov 21, 2020
@cqcallaw
Copy link
Contributor Author

Thanks - turned out I was accidentally invoking a system-wide install of tidy instead of my local build. D'oh!

I hadn't spent much refining the fix, but I did notice one simplification, which is now pushed. For my own education, I'm interested to know how you'd implement the change, though; how would you tackle this issue?

@geoffmcl
Copy link
Contributor

@cqcallaw can't tell you how many times I have done that...

I had not actually coded a fix, but the thought in my mind, was to check the next byte, if (p[1] != 0) warn;break;, on finding the first %... but...

But now like your current code better... since it avoids have two of the same error messages... which was my only negative about your first code, and mine... but both work, and that is the important thing... thanks...

cqcallaw added a commit to cqcallaw/tidy-html5 that referenced this issue Nov 21, 2020
@geoffmcl
Copy link
Contributor

@cqcallaw I think I spoke too soon...

The present code would pass width=24%%, or even 24%%%%%%%. Do we feel this is important? It has been that way since the module was first introduced, circa 2011, and no issues posted. But I guess so...

It seems we do need to repeat the error message... unless you can find another way... without making the code way more complicated than it should be... thanks...

cqcallaw added a commit to cqcallaw/tidy-html5 that referenced this issue Nov 22, 2020
@cqcallaw
Copy link
Contributor Author

I completely missed that edge case, good catch! I've pushed a fix that works for me locally; please let me know if that works for you.

@geoffmcl
Copy link
Contributor

@cqcallaw works for me... and have now merged it... thanks...

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

Successfully merging a pull request may close this issue.

2 participants