Problem/Motivation
Consumers such as search engines are expecting to find the schema.org annotated entity title somewhere inside the entity HTML markup, near the other field output. It doesn't really matter whether it's at the beginning or at the end. #1077602: Convert node.tpl.php to HTML5 was supposed to solve this issue by placing the title inside the entity HTML, but it didn't go through.
Proposed resolution
Drupal 7 outputs the title as metadata inside the <head>
element:
<?php
<meta about="/node/1" property="dc:title" content="Lorem ipsum" />
?>
so we just need to move that markup inside the entity DOM element. This is only needed for the full view mode.
<?php
<meta property="dc:title" content="Lorem ipsum" />
?>
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Create a patch | Instructions | ||
Reroll the patch if it no longer applies. | Novice | Instructions | |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
User interface changes
There is no UI associated with this.
API changes
None.
Original report by scor
The upgrade to HTML5 of node.tpl.php brings in a nice feature: the title of the node is not rendered outside the main node DOM element any more on full view mode. This is the case in D7. This was the main reason for adding a special case in rdf_preprocess_node() for outputing the title in RDFa in the head element. This will no longer be needed once #1077602: Convert node.tpl.php to HTML5 is committed.
Comment | File | Size | Author |
---|---|---|---|
#49 | Screen Shot 2014-07-01 at 6.58.34 PM.png | 1.3 MB | leslieg |
#47 | 1323830-rdfa_title-47.patch | 4.81 KB | kay_v |
#42 | test_case_passed.png | 7.14 KB | er.pushpinderrana |
#40 | interdiff-1323830-40.txt | 484 bytes | er.pushpinderrana |
#40 | drupal7-rdf-title-1323830-40.patch | 4.84 KB | er.pushpinderrana |
Comments
Comment #0.0
scor CreditAttribution: scor commentedupdate summary given that #1077602 didn't solve this issue.
Comment #0.1
scor CreditAttribution: scor commentedcode fix
Comment #1
scor CreditAttribution: scor commented#1077602: Convert node.tpl.php to HTML5 didn't address this use case in the end, the placement of title inside the main entity output was removed from the patch, and moved to #1328048: Page title location needs to be flexible. Unfortunately it looks like this issue (#1328048) isn't going to make it in for various reasons, so we need to keep using the D7 approach and place the title metadata in the entity HTML. I've updated the issue summary.
Comment #1.0
scor CreditAttribution: scor commentedremaining tasks
Comment #2
scor CreditAttribution: scor commentedThis patch takes the same approach as contextual.module and inserts the hidden metadata markup in the element of the node output, right after the title. Inserting such metadata markup in
<header>
was considered a decent solution by @effulgentsia and @Zarabadoo.<header>
at the node template level can be considered the equivalent of the<head>
element at the document level.This patch should pass the tests since there is no change in the semantics of the output. Follow up patches should try to generalize the approach to work for all entities.
Comment #4
scor CreditAttribution: scor commentedwe had a dirty little hack in the test to account for some spaces in the title of the node on full view mode. we will be able to clean that up and remove at least one @todo in our tests.
Comment #6
scor CreditAttribution: scor commented#4: 1323830_4_rdfa_title.patch queued for re-testing.
Comment #7
scor CreditAttribution: scor commentednew patch with cleaned up tests (removed 3 @todo).
Comment #8
scor CreditAttribution: scor commentedHere is the DOM in Bartik with the metadata title element:
Comment #9
Zarabadoo CreditAttribution: Zarabadoo commentedI like the placement of the markup in the header better than the content. There is less variation up there and easier to target with css. It also makes more sense to me semantically. The tag is giving more description to the entity and is not content.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedLooks good to me.
Comment #11
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #12
scor CreditAttribution: scor commentedI'd like to see whether this could be backported to Drupal 7 easily.
Comment #13
scor CreditAttribution: scor commentedComment #13.0
scor CreditAttribution: scor commentednote about full view mode
Comment #14
chrisfromredfinHere is a first attempt at the backport patch.
Comment #16
chrisfromredfinThis patch also corrects the tests to look for span tags instead of meta tags.
Comment #17
chrisfromredfinHere is a screenshot of the markup in D7 with patch applied.
Comment #18
scor CreditAttribution: scor commentedThis is looking great, Chris. We need to remove the logic for the node teaser as well since now the markup is consistent no matter what view mode we're in:
And I believe some of the tests will need to be adjusted as a result.
Comment #19
chrisfromredfinI think this is correct now, for removing the logic around teaser mode. This is a simplified, unified approach now.
Comment #20
chrisfromredfinflagging needs review for testing.
Comment #21
scor CreditAttribution: scor commentedYou're making good progress on this issue, Chris!
The about attribute is not needed since we know we're in the context of the node in the DOM (title_suffix is always display inside the wrapping
Also, the entire logic of adding the markup in title_suffix should go in that if (!empty(..., because there is no point adding that tag unless we know for sure there is an actual predicate defined for the title.
While you are there, please also add a "." at the end of the comment at the beginning: // Adds RDFa markup...
No need to use an intermediary $element variable here, just throw the array() in $variables[...] :)
Now that the markup will always be inside the node DOM element, let's add //div[@about='$url'] at the beginning of the XPath expression to make it look more consistent with the XPath that follows. (This applies to all three changes in rdf.test)
Comment #22
chrisfromredfinUpdated the tests, and overall cleaned up interstitial arrays, and wrapped all logic in the !empty/predicates conditional.
Comment #24
chrisfromredfinHmm, messed that up somehow. This one should apply clean.
Comment #25
chrisfromredfinNew patch to replace missing comma per coding standards. Doh!
Comment #26
mgifford25: 1323830-rdfa_title-25.patch queued for re-testing.
Comment #28
scor CreditAttribution: scor commentedComment #29
dcam CreditAttribution: dcam commented#25 needs to be rerolled.
Comment #30
mgiffordHopefully this addresses that.
Comment #32
scor CreditAttribution: scor commentedThis patch undoes the fix #2011918: Titles are often double-escaped (including in the content attribute of the dc:title meta element for nodes), watch for those lines:
Make sure to use
$variables['node']->title
in the new version. Secondly, the tests are failing because in testAttributesInMarkup2(), the node title includes a single quote (line 320), and this quote will be escaped in the output HTML. The node title also has to be escaped/check_plain'ed in the test XPath, otherwise the XPath expression is invalid as reported by the test bot.Comment #33
mgifford#1 Easy to do the conversion to
'content' => $variables['node']->title
#2 I'm a bit lost here as what to do with the single quote. Is there an example to look to for check_plain() a similar title?
I grepped but couldn't find it.
Comment #34
scor CreditAttribution: scor commentedre #33.2: all you need to do is to check_plain $node->title in the test, and insert that values in the XPath expression instead of the raw $node->title. That way the single quote will automatically be escaped in the expected value in the test.
Comment #35
mgiffordOk, so like this I assume.
Comment #36
scor CreditAttribution: scor commentedYes, #33.2 looks good now, but we're missing #33.1.
oops :)
Comment #37
mgiffordThanks!
Comment #40
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedFixed the test case issue in this patch and also tested locally to ensure it get pass through Testbot.
Thanks!
Comment #42
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedNot sure why this failure occur because this test gets passed locally attached screenshot of same.
Comment #45
scor CreditAttribution: scor commented#40 is wrong, there should be no check_plain in rdf_preprocess_node().
Forget about the check_plain idea in the test. Since there is a single quote in the expected value, we just have to make sure we wrap it in double quotes in the final XPath expression. Both of those worked for me locally: http://pastebin.com/PhSg1UTk (using pastebin because the text format is munging some backslashes)
Comment #46
er.pushpinderrana CreditAttribution: er.pushpinderrana commented@scor thanks for your valuable suggestion. I would remove check_plain from rdf_preprocess_node(). As I am unable to test it locally so unable to identify what would be the right code, can we have check_plain in following way, please confirm.
Drupal submission also hiding (\) from above code that are placed in front of
"".check_plain(
and from end after($node->title)."
Thanks again!
Comment #47
kay_v CreditAttribution: kay_v commentedimplemented Scor's pastebin code :)
Comment #48
leslieg CreditAttribution: leslieg commentedComment #49
leslieg CreditAttribution: leslieg commentedChecked code diff and viewed source. Here is a screenshot of the markup in D7 with patch applied.
Comment #50
leslieg CreditAttribution: leslieg commentedComment #51
scor CreditAttribution: scor commentedLooks good. Thank you all for your patience and great work!
Comment #54
scor CreditAttribution: scor commentedback to RTBC.
Comment #57
David_Rothstein CreditAttribution: David_Rothstein commentedThis patch looks great to me. My only question is whether this extra
<span>
will run the risk of breaking someone's theming? (It's not best practice to write CSS that targets a generic span, without targeting a particular class, but I see it all the time.)To mitigate that possibility, should we add the "element-hidden" class (or similar) to this tag, to try to make sure it will never display?
Comment #58
scor CreditAttribution: scor commentedoops, the screenshot from #49 isn't highlighting the right markup. The relevant markup is:
(it's right at the top of the screenshot, though you can only see half the line)
@David: There is a class in that span, so themer could always target span.rdf-meta to hide it if they wanted to. I'm also fine adding the element-invisible class to it... we should probably add it to theme_rdf_metadata() directly. What do you think?
Comment #59
David_Rothstein CreditAttribution: David_Rothstein commentedI agree, might as well add it to theme_rdf_metadata() to catch all future cases as well. I do think it's worth adding (even with the rdf-meta class already present) because we are adding new markup and we want it to work without the chance of themers having to come in and fix things on existing sites.
I believe it would be "element-hidden" (which does a simple display: none), rather than "element-invisible" (which does more complicated things to allow screen readers to access what's inside the tag - that shouldn't be relevant here)?
Comment #60
scor CreditAttribution: scor commentedWe have 2 alternatives to hide this span. Let's discuss this in #2301955: Ensure RDFa metadata tags are hidden.
I guess it means we will have to wait until #2301955: Ensure RDFa metadata tags are hidden gets committed to D8 and D7 to make progress here :)
Comment #61
scor CreditAttribution: scor commented#47 is good to go and get committed right after #2301955: Ensure RDFa metadata tags are hidden. Since both those patches are touching different parts of rdf.module, no reroll is necessary.
Comment #64
dcam CreditAttribution: dcam commentedComment #65
lokapujyaComment #70
dcam CreditAttribution: dcam commentedComment #73
dcam CreditAttribution: dcam commentedComment #74
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!