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.

Sign up for GitHub

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

Jump to bottom

Duplicate IDs are not detected if the ID has an uppercase letter #726

Closed
petdance opened this issue Apr 25, 2018 · 10 comments
Closed

Duplicate IDs are not detected if the ID has an uppercase letter #726

petdance opened this issue Apr 25, 2018 · 10 comments
Labels
Bug
Milestone
5.7

Comments

@petdance
Copy link
Contributor 

$ cat -n foo.html
     1  <!DOCTYPE html>
     2  <html>
     3      <head>
     4          <title>Tidy does not detect dupe IDs if they have uppercase letters</title>
     5      </head>
     6      <body>
     7          <p    id="foo">1</p>
     8          <div  id="foo">2</div>
     9          <span id="foo">3</span>
    10
    11          <p    id="foo99">1</p>
    12          <div  id="foo99">2</div>
    13          <span id="foo99">3</span>
    14
    15          <p    id="Foo">1</p>
    16          <div  id="Foo">2</div>
    17          <span id="Foo">3</span>
    18
    19          <p    id="FOO">1</p>
    20          <div  id="FOO">2</div>
    21          <span id="FOO">3</span>
    22
    23          <a name="foo">1</a>
    24          <a name="foo">2</a>
    25          <a name="foo">3</a>
    26
    27          <a name="FOO">1</a>
    28          <a name="FOO">2</a>
    29          <a name="FOO">3</a>
    30      </body>
    31  </html>

$ tidy -q -e foo.html
line 8 column 9 - Warning: <div> anchor "foo" already defined
line 9 column 9 - Warning: <span> anchor "foo" already defined
line 12 column 9 - Warning: <div> anchor "foo99" already defined
line 13 column 9 - Warning: <span> anchor "foo99" already defined
line 23 column 9 - Warning: <a> anchor "foo" already defined
line 24 column 9 - Warning: <a> anchor "foo" already defined
line 25 column 9 - Warning: <a> anchor "foo" already defined

If the id or name values have an uppercase letter, then Tidy will not dupe-check them. It will do the correct thing with digits.
@petdance petdance added the Bug label Apr 25, 2018
geoffmcl added a commit that referenced this issue Apr 26, 2018
@geoffmcl
Is #726 #185 - HTML5 is case sensitive for anchors.
68e6b3c 
modified:   src/attrs.c
@geoffmcl
Copy link
Contributor

@petdance thank for the issue... yes, this is a bug...

It turns out an incomplete fix was applied in #185...

When creating the NewAnchor structure, had left converting the name to lowercase, so the if ( TY_(tmbstrcmp)(found->name, lname) == 0 ) failed, so no show of the uppercased duplicates...

Have pushed a fix to the issue-726 branch...

My new v.5.7.14.I726 now shows 14 warnings, as opposed to the 7 you show...

line 8 column 9 - Warning: <div> anchor "foo" already defined
line 9 column 9 - Warning: <span> anchor "foo" already defined
line 12 column 9 - Warning: <div> anchor "foo99" already defined
line 13 column 9 - Warning: <span> anchor "foo99" already defined
line 16 column 9 - Warning: <div> anchor "Foo" already defined - NEW
line 17 column 9 - Warning: <span> anchor "Foo" already defined - NEW
line 20 column 9 - Warning: <div> anchor "FOO" already defined - NEW
line 21 column 9 - Warning: <span> anchor "FOO" already defined - NEW
line 23 column 9 - Warning: <a> anchor "foo" already defined
line 24 column 9 - Warning: <a> anchor "foo" already defined
line 25 column 9 - Warning: <a> anchor "foo" already defined
line 27 column 9 - Warning: <a> anchor "FOO" already defined - NEW
line 28 column 9 - Warning: <a> anchor "FOO" already defined - NEW
line 29 column 9 - Warning: <a> anchor "FOO" already defined - NEW
Tidy found 14 warnings and 0 errors!

And using the sample below, this new version will still show no warning, as expected, as would previous versions -

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Is #726-2</title>
    </head>
    <body>
        <p    id="foo">1</p>
        <div  id="Foo">2</div>
        <span id="FOO">3</span>
    </body>
</html>

But add a HTML4 doctype will still show 2 warnings, as expected, as did earlier versions...

In regression testing, with this new version, case 588061 now shows multiple additional warnings, for a duplicated anchor of "MAP details", which previous versions missed... looks like the expects output needs to be adjusted...

So this looks like there should be simultanious PR's, here and in tests, not done yet... still testing, checking, etc...

Appreciated if you, or others, could checkout the issue-726 branch, test and report... thanks...

@geoffmcl geoffmcl added this to the 5.7 milestone Apr 26, 2018
@petdance
Copy link
Contributor Author

How do I run the test suite? I can't figure it out.

@geoffmcl
Copy link
Contributor

@petdance the first step is to clone the test-suite repo, and it is best if it is cloned in a parallel directory to your tidy-html5 clone, but this is not essential...

Then cd tidy-html5-tests, then cd tools-sh or cd tools-cmd, depending if unix or windows... run testall.sh or alltest.bat respectively...

There is a RUNTESTS.md hopefully explaining it all...

And it has been discussed lots of time in these issues, the last being #718 (comment), near bottom...

Advise if you have any trouble... thanks...

@petdance
Copy link
Contributor Author

Perhaps we need something in the tidy-html5 repo that explains to go look in the test-suite repo. I didn't realize it was a second repo.

geoffmcl added a commit that referenced this issue Apr 27, 2018
@geoffmcl
Is #718 #726 - Some updates of CONTRIBUTING.md
b952e65
geoffmcl added a commit that referenced this issue Apr 27, 2018
@geoffmcl
Is #718 #726 - Bump to 5.7.16 for this docs update
5f7e367
@geoffmcl
Copy link
Contributor

@petdance have just pushed some updates to CONTRIBUTING.md... commit b952e65... and certainly mentioned the regression tests, in a separate repo...

Appreciated if you would check it over, and advise any errors, or other suggestions... thanks...

@petdance
Copy link
Contributor Author

Looks good. Thanks.

geoffmcl added a commit that referenced this issue May 1, 2018
@geoffmcl
Is #726 #185 - HTML5 is case sensitive for anchors.
cb23ab0 
modified:   src/attrs.c
@geoffmcl geoffmcl mentioned this issue May 14, 2018
Merged
@geoffmcl
Copy link
Contributor

Have now created PR #731, but as mentioned, regression test case 588061 expects needs to be adjusted at the same time... still to do a PR for that...

@geoffmcl geoffmcl mentioned this issue Sep 1, 2018
Closed
@geoffmcl geoffmcl mentioned this issue Nov 16, 2018
Merged
geoffmcl added a commit that referenced this issue Nov 17, 2018
@geoffmcl
Is #726 PR #731 - Bump to 5.7.17 for this merge
c6f1946
@geoffmcl
Copy link
Contributor

@petdance - Now both PRs merged... bump to version 5.7.17, both here, and in tests... closing this... thanks...

@petdance
Copy link
Contributor Author

Excellent, thank you. I'll try to work with it tomorrow.

@petdance
Copy link
Contributor Author

petdance commented Nov 20, 2018 •
edited

I don't see a tag or release for 5.7.17. Am I missing something?

$ git tag
5.0.0
5.1.14
5.1.24
5.1.25
5.1.8
5.2.0
5.4.0
5.6.0

petdance added a commit to petdance/html-tidy5 that referenced this issue Nov 21, 2018
@petdance
Duplicate IDs would not be detected if the ID had an uppercase letter.
efcbb50 
This has been fixed in the underlying library 5.7.17.
htacg/tidy-html5#726
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
5.7
Development

No branches or pull requests
2 participants
@petdance @geoffmcl