SVG attributes flagged as proprietary #903
The following HTML input to tidy (build from 1889880 on the next branch) validates with zero errors and zero warnings on https://validator.w3.org/nu/#file, but generates the following warnings in tidy. The "proprietary" attributes seem to be well-defined in section A.3.7 of the SVG 1.1 DTD, so this looks like a simple implementation gap that can be filled by adding the attributes to
The text was updated successfully, but these errors were encountered:
@cqcallaw thank you for this issue, and sample code, which I will label as a
From the W3C reference, and the fact that your sample passes the W3C
But this is in a HTML5 document, so care should be taken to continue to flag these attributes in legacy documents... if you replace the
And in brief testing, it seems the
For sure, we would need to add the missing
Look forward to further feedback, patches, or PR to implement this... thanks...
@geoffmcl Thanks for the follow up! I've pushed a commit, but it has a few issues:
This issue isn't unique to this patch, however; the
@cqcallaw thanks for kicking this off... this is great... a few thoughts, and suggestions...
You appear to have added your commits to the
It is better you create a local branch, like
Not sure how to back out of this now, but there might be a git way... In the past, when faced with this, I, and others, have just deleted the whole fork, and forked
But yes, your stripping of the line space endings does really mess up a review process, and adds a much larger than needed commit to the git history. But if you take the above
I can see many are just a
But others are words, like
Or each is given there own checker, like
Also we must check those attribute already defined, in
Finally, we must check the document
With a new branch setup in your fork, I could clone, build and test your fork/branch, and add further comments, as this moves forward, until we have a complete PR ready, and tested...
If I have missed something, please ask...
Hope this is not all-too-much for you... but you have made a reasonable start... look forward to more... thanks...
@geoffmcl Thanks for taking a look! I'm pretty sure the next branch on my fork is even with the upstream branch; I'm seeing my commits on the svg-paint-attrs branch I created. Do you see something different?
I've dropped the whitespace commit, so a PR should be cleaner now. Digging into the attribute validator logic is going to take some time; I'll update the branch and the issue when I have more to share.
@geoffmcl I've sketched out what seems like a good direction for the validation of SVG attributes. Please take a look and let me know if the structure seems sound and I'll fill in the rest of the details.
I don't think I can commit to writing the code to process each and every valid SVG attribute, but I should be able to cover the core paint attributes that are defined in my patch.
I'm quite mystified about why the HTML 4.01 parsing doesn't flag the added attributes as proprietary. Seems like the logic at https://github.com/htacg/tidy-html5/blob/188988022da4a64d80bc4a2eba21c33d57eb5152/src/attrs.c#L490..L505 is designed to handle this.
@cqcallaw Wow, this is starting to look good... many thanks...
Yes, I can now see the commits are in the
I cloned your fork, switched branches, and did a build, and started a MSVC Debug session... The first little problem is the
You have duplicated
Of course you have to adjust the
But in the
And maybe it is time we added
And unless you add a
Now to the html5 vs legacy question... yes, the
I have had some success by adding the option
Of course, in
Will revert if I find out more on this question...
This is a great step forward towards a good PR, but needs a little more work... look forward to further feedback... thanks...
@geoffmcl thanks again for your help! I've pushed a couple of additional patches to my branch with fixes for issues highlighted above.
As far as I can tell, the SVG
@cqcallaw thanks for the fixes - using
There are two(2) separate things about
The first is it is NOT present in the
The second may NOT be particularly related to this issue, but Tidy pesently only supports some 16
Fixing the first, and changing my
Now the question is, do these ONLY apply to the
In other words can we just extend the current
I presently think it can be applied to ALL, but could be persuaded otherwise... What do you, others, think?
I did not quite understand what you are trying to say in the last part of the above paragraph. I note there are some 15 attributes listed in the
Now I can see some 6 attributes do NOT use
And a very small thing, can we add this issue number, to the code comments like
Still try to decide on whether
Getting close... look forward to further feedback... thanks...
@geoffmcl thanks again for your continued attention to this issue; I didn't expect it to require so much of your time. I'm glad you're still willing to help :)
I fixed the overzealous bits and added the color property back into the appropriate array. I think the color issue is larger than SVG; I didn't do an exact comparison, but the SVG color names seem to be equivalent to the CSS colors referenced in the HTML5 spec.
Since the coverage of attributes was confusing, I've revised the code so all SVG attribute validation is handled in
I think I agree that
@cqcallaw it is not my time that is the question. I am an old, retired programmer, and have lots of that, now for only FOSS projects...
No, it is the motivation, enthusiasm, ... that is important. I get sick of just me coding tidy, so much so that I recently took nearly a year off from tidy, when nothing was coded... so it's your efforts that keeps me going ;=))
You can see quite a lot of issues which end with
Thank you for putting all svg attrs through
And thanks for finding another W3C
We do have to decided about
And I have been meaning to ask, "Is it true that ALL these paint attrs support
If there is no
Thanks, but please do not include sample test files in this
We do have a repo of
Marching forward... look forward to further feedback... thanks...
PS: Will try to embed
@geoffmcl Glad to hear my contributions aren't a burden, and thank you for maintaining this project!
If that's okay with you, I'd prefer to handle the additional color values in a separate commit. I think this commit is getting pretty large already, and I'd like to constrain its scope if possible.
According to Wikipedia, SVG 1.0 become an official recommendation in 2001, whereas HTML 4.01 was officially recommended in 1999. I'm not sure why the W3C validator allows the tag at all--maybe they knew SVG was being drafted so they thought they'd allow some draft version of SVG to validate?
I've reverted the sample files from the branch, so all should be well there. Thanks!
@cqcallaw Actual code contributions are very, VERY seldom a burden ;=))
Of course, they can be, if you disagree with the developer's direction, which has happened in the past... but not when you are working towards a common, agreed goal... like
Yeah, the additional color support, can be a separate commit... no problem... but it means my
Ah, thanks for tracking down those dates... As you probably already understand, the W3C validator, is actually two(2) separate beasts - the
Given that the
And I am starting to think,
But I am happy with where we are at the moment on this, only giving error if
Yes, I agree, most of the paint attributes support
But now, as the
How you deal with this is up to you. You could have another service like
Re: unix/linux issue
Now being a mainly Windows person, I have been overlooking another important issue, and that is the visibility of these new services. If a service is only ever used in that specific module, ie a single C source file, then is MUST have a
And if it is used across modules, then is MUST be in a
The window compiler/linker already only exposes, in its export library, the API services marked
I can see that there were already some mistakes in this module, before you started, so was misleading, and I am not asking you to fix those, but we must make an effort to conform to the
So that should be
I am sorry to bring this up at such an advanced stage, but it is important, for linux shared library distribution...
And thanks for removing the
Had quite a short day today, and did not get any testing done... but will get to that... thanks for your continued support...
@cqcallaw oops, made 1 or 2 small boo-boos in colors3.c... glad you had not yet included it...
I re-extracted the table from the https://drafts.csswg.org/css-color/#named-color link, ran Perl HTML::Strip;, extracting the text, and ran a Notepad++ macro to remove the RGB values, then some simple find & replace macros, and got a slightly modified color4.c module... all in a few mins... just for fun...
Am now sure if conforms exactly to the web page...
Hence the imbedded zip - colors4.c.zip
And, as before, also available here - http://geoffair.org/tmp/colors4.c.zip
This looks good, when you are ready...
@geoffmcl My basic concern with including the colors patch comes down to bisections and reverts. If we bundle color updates with the SVG paint attribute handling and our bundled commit introduces a bug, it's harder to revert the change cleanly. I don't know how this has been handled by this project in the past, though. How are regressions typically handled?
Treating SVG tags as proprietary in HTML 4.01 sounds good to me, but I'd suggest creating a separate commit for the reasons mentioned above. I think we want to keep any changes that might break existing validation workflows as small as possible, so we can react quickly and precisely to regressions.
Regarding module exposure, no worries; I am a Linux user and want to make sure the tool works well for myself and other Linux users. I pushed an update that makes
@cqcallaw thank you for the further feedback, and sorry for the brief delay...
Although I do not expect any
My minimal experience with
proprietary in HTML 4.01
And again agree this can be dealt with after things have settled down a bit...
@geoffmcl Sounds good, thanks for your continued attention. As of right now, I think we have these additional issues to address once this issue is closed:
I've fixed the double use of
Thanks for the note that a rebase may be necessary. I plan to squash the commits from my branch together before submitting a PR anyway, so I'll probably rebase as part of that operation.
@cqcallaw thanks for the fixes, and feedback...
I agree those 4 items can be separate additional issues...
Although, still very optional on 4... I did a quick check of the some 54,000 lines of current tidy source, and found some 1,186 with trailing white spaces, just a few percent... and checked back to the old
Have also had a chance to do the tidy regression tests using your new tidy, and nothing popped. But that is more because there is only 1 XML
Unfortunately, did not find the energy, enthusiasm, to begin clearing out the long outstanding PRs today... maybe this weekend...
Getting there... thanks...
@cqcallaw as you can see got started on some PR catching up today, on some easy ones - just 4 - a short day... hope I can continue tomorrow, really deciding on some older ones... in or out or whatever...
Thanks for the (squashed) PR #907, for this - seems no conflict yet... good...
And the new issues... will get to them in due course, even the trimming ;=))
The trim PR #911 does already seem to have a small conflict in just 2 files... but the fix probably should wait until I finish my
Moving forward... enthusiasm begets enthusiasm... thanks...