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

Support extended color names in HTML 5 #908

Closed
cqcallaw opened this issue Nov 20, 2020 · 10 comments · Fixed by #914
Closed

Support extended color names in HTML 5 #908

cqcallaw opened this issue Nov 20, 2020 · 10 comments · Fixed by #914

Comments

@cqcallaw
Copy link
Contributor

cqcallaw commented Nov 20, 2020

HTML 4.01 only supports 16 color names, but HTML 5 supports a much larger set of named colors. Per the discussion in #903, it would be useful for tidy to support this extended set of colors.

@cqcallaw
Copy link
Contributor Author

@geoffmcl has prepared an array definition for the larger set; attached as colors4.c.zip

@geoffmcl
Copy link
Contributor

@cqcallaw thanks for opening this issue... it has been long outstanding...

When I prepared the colors4.c table I had the mad idea that we could just replaces the 16 color names, with the 148 color names, and all done... and still not sure how crazy that really is...

I am sure browsers have done just that, and in several tests, they seem to support the full 148 names regardless of the DOCTYPE... and why not?

And to my surprise, passing <p><font color="yellowgreen">yellowgreen font color</font></p> in a HTML 4.01 Transitional document also does not raised flags! Of course, if you make that HTML 4.01 Strict then both font tag and the color attribute raise red flags...

Of course, we could adjust my new table to put the 16 legacy color names at the top, then adjust GetColorCode and GetColorName to stop scanning at 16, if in legacy mode, or something along those lines...

What do you, or others, think about this?

And the access.c module also has colorValues, and colorNames with just 16 entries each. Do these also need to be adjusted?

Looking forward to further feedback, comments, patches, PR, etc, on this issue... thanks...

@cqcallaw
Copy link
Contributor Author

@geoffmcl There seems to be a high-level question of philosophy here: should tools like tidy conform to the specification, or should they conform to common browser behavior?

I prefer validation tools that follow the spec closely, because of the regularity with which improving my knowledge of the spec improves my code. I'm just one user though; others might have a different perspective.

@geoffmcl
Copy link
Contributor

@cqcallaw thanks for the quick feedback... much appreciated...

Don't know what I was thinking... momentary lapse... sorry!

Yes, absolutely, the tidy philosophy is certainly to conform to the specification, 100%... well, where ever possible...

And as you can read, probably many times, in these issues, the casting aside of common browser behavior as an invalid tidy argument... always, what do the W3C specs say...

The only concession to browsers is, in tidying a document, removing, changing, fixing, re-lining, etc, etc, to try very hard to keep the same browser rendering... not always easy, and can vary between browsers... but important...

So, as stated, looking forward to further feedback, comments, patches, PR, etc, on this HTML5 only issue... thanks...

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

@geoffmcl sounds good, I've pushed a commit for review.

@geoffmcl
Copy link
Contributor

@cqcallaw pulled your extended-colors branch, and reviewed the diff...

Hmmm, two separate tables... yeah, that's another way to go... switched on IsHTML5Mode... at the slight cost of 16 duplicated colors... but no problem...

Built, and minimally tested, v.5.7.42.xcol1, and it works... so let's go for it...

I did manage to run the regression, and two cases, 476a and 476b, popped on the diff, but it turns out one of the merges since Oct 3, 5.7.35, which is clean, and Nov 22, 5.7.42, caused the 2 duplicated warning message, and nothing to do with this extend color scheme...

Note to self: Really MUST run the regression tests after EACH merge... just set TY_TIDY_PATH in the environment, to the tidy.exe you want to try, and run the alltest.bat, setting a relative output folder, -o path, in the tools-cmd folder... must now backtrack to find the cause... or accept it, and move on...

But as stated, nothing to do with your commit... present the PR, and it will be merged, in due course... thanks...

On a related topic, must also look at the 2 color tables in access.c... do they need to also be expanded?

@geoffmcl
Copy link
Contributor

@cqcallaw found the duplicate warning, and fixed an out-of-order attribute_defs[] table...

Now back to a full pass on the regression tests... and no assert on the DEBUG build ;=))

Bumped to 5.7.43 for these changes... hope you get the chance to rebase... thanks...

@cqcallaw
Copy link
Contributor Author

Fair, the redundancy of color duplication is a bit lazy. I've removed that redundancy and tweaked the logic to accommodate that change. I've updated access.c as well. See https://github.com/cqcallaw/tidy-html5/tree/extended-colors

Regarding automated regressions, perhaps GitHub Actions could help? See https://docs.github.com/en/free-pro-team@latest/actions/guides/about-continuous-integration

@geoffmcl
Copy link
Contributor

@cqcallaw, wow, I did not mean to imply the small duplication needed to be addressed... sorry... but thanks for the change anyway... looks better...

Have now built v.5.7.43.xcol2 and ran the regression tests, and all is fine...

And thanks for extending the tables in access.c, and agree we do not need to separate legacy vs html5, in here...

These access tests are to try to point out things which may cause difficulty to some people with disabilities... see WAI/WCAG ... But this area of tidy has received very little support in recent years. We do have some regression tests for this, see cases/access, alltest.bat -c access, and I just tried it, using v.5.7.43, and it seems ok...

In the past we have rejected CI... and I think that is still true for the main repo...

Now, in this cases it is across two separate repos... yes, it may be possible to have a webhook, that fired on each push, to the main, to cause the regression tests repo, to pull and build the main repo, to get a current tidy binary, to be able to run the tests...

But this is above and beyond my capability, or interest, and even if someone were to build it, implement it, and offered to maintain it, it may still be rejected... so for now it has to remain manual... I set up little bat files, like t-908.bat for this issue, and running it just takes 14 seconds...

But this is probably not the place to discuss CI, or similar... for background see #175, #269, #330, #545, #546, and maybe others...

Look forward to the final PR, closing this color issue... thanks...

@cqcallaw
Copy link
Contributor Author

No worries, I think clean code is appropriate for a tool that's intended to encourage folks to write cleaner code. PR is submitted, and I've rebased the whitespace patch; I think that one is ready to submit as well.

Thanks!

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

Successfully merging a pull request may close this issue.

2 participants