Skip to content

Require itemprop, property, or rel on link element #814

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

Closed
wants to merge 1 commit into from

Conversation

da2x
Copy link

@da2x da2x commented Apr 22, 2019

<link itemprop="subject" href="../example"> in Microdata is equivalent to <link property="subject" href="../example"> in RDFa. This HTML+RDFa syntax doesn’t trigger any errors or warnings from the W3C Nu Html Checker in HTML5 mode.

This change changes the requirement from link[itemprop|rel] to link[itemprop|property|rel] to support the HTML5+RDFa syntax.

See also: https://www.w3.org/TR/html-rdfa/#extensions-to-the-html5-syntax

@geoffmcl
Copy link
Contributor

@da2x thank you for the issue, and a potential fix, via this PR...

Still reading, understanding, and checking, but this looks good to merge... maybe with an added comment, linking the change back to this issue...

Meantime, look forward to any further, other feedback... thanks...

@geoffmcl
Copy link
Contributor

geoffmcl commented Jun 9, 2019

@da2x this continues to look good... but need a simple test case...

Have not yet had a chance to run the regression tests, for this parsing change... have you?

Maybe just needs a synchronous test change, if a problem... Or maybe consider adding a test case... for the future...

Will work on this, unless someone beats me to it... adds info... comments...

Look forward to any further feedback... thanks...

@geoffmcl
Copy link
Contributor

@da2x ran into probs... this PR is against master... ie version 5.6.0 currently...

As indicated in CONTRIBUTING.md, a PR can only be against next... maybe this needs to be made clearer... so the PR is rejected... sorry... but your fix is required...

I am presently getting near to testing with the following patch applied... you will note I have added some comments, as suggested...

diff --git a/src/tags.c b/src/tags.c
index af2b7d9..e639e5b 100644
--- a/src/tags.c
+++ b/src/tags.c
@@ -1008,19 +1008,25 @@ void CheckTABLE( TidyDocImpl* doc, Node *node )
     }
 }
 
-/* report missing href attribute; report missing rel attribute */
+/* report missing href attribute; report missing rel attribute
+   Is. #814 - change from link[itemprop|rel] to link[itemprop|property|rel]
+   to support the HTML5+RDFa syntax.
+   Ref: https://www.w3.org/TR/html-rdfa/#extensions-to-the-html5-syntax
+ */
 void CheckLINK( TidyDocImpl* doc, Node *node )
 {
     Bool HasHref = TY_(AttrGetById)(node, TidyAttr_HREF) != NULL;
     Bool HasRel = TY_(AttrGetById)(node, TidyAttr_REL) != NULL;
     Bool HasItemprop = TY_(AttrGetById)(node, TidyAttr_ITEMPROP) != NULL;
+    Bool HasProperty = TY_(AttrGetById)(node, TidyAttr_PROPERTY) != NULL; /* Is #814 */
 
     if (!HasHref)
     {
       TY_(ReportMissingAttr)( doc, node, "href" );
     }
 
-    if (!HasItemprop && !HasRel)
+    /* Is. #814 - @da2x patch - add 'property' attr */
+    if (!HasItemprop && !HasRel && !HasProperty)
     {
       TY_(ReportMissingAttr)( doc, node, "rel" );
     }

Will shortly, hopefully get to the regression testing... and maybe adding a test sample...

Look forward to any further feedback... thanks...

@geoffmcl
Copy link
Contributor

@da2x after doing the regression tests, no probs, expanded patch to deal with legacy docs -

diff --git a/src/tags.c b/src/tags.c
index af2b7d9..ecc0ee7 100644
--- a/src/tags.c
+++ b/src/tags.c
@@ -1008,21 +1008,44 @@ void CheckTABLE( TidyDocImpl* doc, Node *node )
     }
 }
 
-/* report missing href attribute; report missing rel attribute */
+/* report missing href attribute; report missing rel attribute
+   Is. #814 - change from link[itemprop|rel] to link[itemprop|property|rel]
+   to support the HTML5+RDFa syntax.
+   Ref: https://www.w3.org/TR/html-rdfa/#extensions-to-the-html5-syntax
+ */
 void CheckLINK( TidyDocImpl* doc, Node *node )
 {
     Bool HasHref = TY_(AttrGetById)(node, TidyAttr_HREF) != NULL;
     Bool HasRel = TY_(AttrGetById)(node, TidyAttr_REL) != NULL;
     Bool HasItemprop = TY_(AttrGetById)(node, TidyAttr_ITEMPROP) != NULL;
+    AttVal *prop = TY_(AttrGetById)(node, TidyAttr_PROPERTY);
+    Bool HasProperty = (prop == NULL) ? no : yes; /* Is #814 */
 
     if (!HasHref)
     {
       TY_(ReportMissingAttr)( doc, node, "href" );
     }
 
-    if (!HasItemprop && !HasRel)
+    if (TY_(IsHTML5Mode)(doc))
+    {
+        /* Is. #814 - @da2x patch - add 'property' attr */
+        if (!HasItemprop && !HasRel && !HasProperty)
+        {
+            TY_(ReportMissingAttr)(doc, node, "rel");
+        }
+    }
+    else 
     {
-      TY_(ReportMissingAttr)( doc, node, "rel" );
+        /* legacy doc parsing */
+        if (HasProperty)
+        {
+            TY_(ReportAttrError)(doc, node, prop, MISMATCHED_ATTRIBUTE_WARN);
+        }
+        else if (!HasItemprop && !HasRel)
+        {
+            TY_(ReportMissingAttr)(doc, node, "rel");
+        }
+
     }
 }

Added 2 tests - in_814.html for html5, and in_814-1.html for legacy html4... which can be put to the W3C validator... should get pass and fail respectively...

Somehow feel the addition of the property attr should have maybe been flagged earlier than here... still testing, experimenting, reviewing, etc...

But now feel this PR has to be closed, and will try to get around to opening a new issue, to fully explore this change... unless someone beats me to it...

Further feedback should be directed to that new issue, when created... thanks...

@geoffmcl
Copy link
Contributor

All discussion on this link, property issue moved to #833 - look forward to your participation... 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.

2 participants