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

Make global attribute dir accept auto as well. #712

Merged
merged 1 commit into from Apr 23, 2018

Conversation

doronbehar
Copy link
Contributor

@geoffmcl
Copy link
Contributor

geoffmcl commented Apr 4, 2018

@doronbehar thank you for this PR... we like those...

But maybe this fix does not go far enough? Or maybe too far, depending on your perspective...

As far as I can quickly read in W3C documentation - note we do not normally accept w3schools, which as far as I know has no affiliation with the W3C - this auto value for the dir attribute was only introduced in HTML5...

See attribute-auto, dir-attribute, dirauto, and probably others references...

Since tidy works in legacy html4 or html5 mode depending on the doctype found, and none defaults to html5, maybe the CheckTextDir service should reflect this?

Maybe we could have something like (code not tested) -

/* checks dir attribute */
void CheckTextDir( TidyDocImpl* doc, Node *node, AttVal *attval)
{
    ctmbstr const values4[] = {"rtl", "ltr", NULL};
    /* Is #712 - add 'auto' for HTML5 */
    ctmbstr const values5[] = {"rtl", "ltr", "auto", NULL};
    CheckAttrValidity( doc, node, attval, 
       (TY_(IsHTML5Mode)(doc) ? values5 : values4) );
}

Or maybe there is a better way to do this switching...

What do you, or others, think? Thanks...

@geoffmcl geoffmcl added this to the 5.7 milestone Apr 4, 2018
@geoffmcl
Copy link
Contributor

@doronbehar I have cycled around to shortly merging this PR, but still think the above patch is required...

It is only in a HTML5 document that auto is valid...

Yes I could add this after the merge, but it seems better if this was part of your fix... thanks...

@geoffmcl geoffmcl merged commit 3475d3e into htacg:next Apr 23, 2018
geoffmcl added a commit that referenced this pull request Apr 23, 2018
geoffmcl added a commit that referenced this pull request Apr 23, 2018
@geoffmcl
Copy link
Contributor

@doronbehar now merged, and then patched as indicated... thanks...

@doronbehar
Copy link
Contributor Author

Thanks you so much for suggesting making this part of my fix, I just couldn't find the time to test it myself.

@geoffmcl
Copy link
Contributor

@doronbehar no problem...

Have added 2 tests to my tests repo, with dir="auto" -

  1. in_712.html - a HTML5 doc, that now passes tidy 5.7.11 plus, with no warnings. And also passes the W3C nu validator.
  2. in_712-1.html - a HTML4 doc, which tidy 5.7.11, and most other versions, earlier or later, will still flag as a warning, as does the W3C legacy validator.

As a note to myself, I guess I should also add these to the official regression tests, as a continual reminder of the addition of dir="auto" in HTML5, unless someone beats me to it...

Another little step forward... 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 this pull request may close these issues.

None yet

2 participants