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

Contributor tasks needed
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.

CommentFileSizeAuthor
#49 Screen Shot 2014-07-01 at 6.58.34 PM.png1.3 MBleslieg
#47 1323830-rdfa_title-47.patch4.81 KBkay_v
#42 test_case_passed.png7.14 KBer.pushpinderrana
#40 interdiff-1323830-40.txt484 byteser.pushpinderrana
#40 drupal7-rdf-title-1323830-40.patch4.84 KBer.pushpinderrana
#37 1323830-rdfa_title-37.patch4.83 KBmgifford
#35 1323830-rdfa_title-34.patch5.59 KBmgifford
#30 1323830-rdfa_title-30.patch4.8 KBmgifford
#25 1323830-rdfa_title-25.patch4.79 KBchrisfromredfin
#24 1323830-rdfa_title-24.patch4.79 KBchrisfromredfin
#22 1323830-rdfa_title-22.patch4.25 KBchrisfromredfin
#22 interdiff.txt4.25 KBchrisfromredfin
#19 interdiff.txt2.08 KBchrisfromredfin
#19 1323830-rdfa_title-19.patch4.88 KBchrisfromredfin
#17 Screen Shot 2013-12-03 at 4.58.08 PM.png106.93 KBchrisfromredfin
#16 interdiff.txt2.72 KBchrisfromredfin
#16 1323830-rdfa_title-16.patch3.97 KBchrisfromredfin
#14 1323830-rdfa_title-14.patch1.19 KBchrisfromredfin
#8 1323830_rdfa_title-1.png87.15 KBscor
#7 1323830_7_rdfa_title.patch5.42 KBscor
#7 interdiff.txt3.31 KBscor
#4 1323830_4_rdfa_title.patch2.84 KBscor
#4 interdiff.txt825 bytesscor
#2 1323830_2_rdfa_title.patch2.03 KBscor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

scor’s picture

scor CreditAttribution: scor commented
Issue summary: View changes

update summary given that #1077602 didn't solve this issue.

scor’s picture

scor CreditAttribution: scor commented
Issue summary: View changes

code fix

scor’s picture

scor CreditAttribution: scor commented
Title: Node title RDFa markup in <head> no longer needed » Place title RDFa metadata inside entity HTML element
Assigned: Unassigned » scor
Category: feature » task
Status: Postponed » Active
Issue tags: +RDFa, +html data

#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.

scor’s picture

scor CreditAttribution: scor commented
Issue summary: View changes

remaining tasks

scor’s picture

scor CreditAttribution: scor commented
Status: Active » Needs review
FileSize
1323830_2_rdfa_title.patch2.03 KB

This 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.

Status: Needs review » Needs work

The last submitted patch, 1323830_2_rdfa_title.patch, failed testing.

scor’s picture

scor CreditAttribution: scor commented
Status: Needs work » Needs review
FileSize
interdiff.txt825 bytes
1323830_4_rdfa_title.patch2.84 KB

we 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.

Status: Needs review » Needs work
Issue tags: -RDFa, -html data

The last submitted patch, 1323830_4_rdfa_title.patch, failed testing.

scor’s picture

scor CreditAttribution: scor commented
Status: Needs work » Needs review
Issue tags: +RDFa, +html data

#4: 1323830_4_rdfa_title.patch queued for re-testing.

scor’s picture

scor CreditAttribution: scor commented
FileSize
interdiff.txt3.31 KB
1323830_7_rdfa_title.patch5.42 KB

new patch with cleaned up tests (removed 3 @todo).

scor’s picture

scor CreditAttribution: scor commented
FileSize
1323830_rdfa_title-1.png87.15 KB

Here is the DOM in Bartik with the metadata title element:
1323830_rdfa_title-1.png

Zarabadoo’s picture

Zarabadoo CreditAttribution: Zarabadoo commented

I 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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

scor’s picture

scor CreditAttribution: scor commented
Status: Fixed » Active
Issue tags: +Needs backport to D7

I'd like to see whether this could be backported to Drupal 7 easily.

scor’s picture

scor CreditAttribution: scor commented
Version: 8.x-dev » 7.x-dev
scor’s picture

scor CreditAttribution: scor commented
Issue summary: View changes

note about full view mode

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin commented
Issue summary: View changes
Status: Active » Needs review
FileSize
1323830-rdfa_title-14.patch1.19 KB

Here is a first attempt at the backport patch.

Status: Needs review » Needs work

The last submitted patch, 14: 1323830-rdfa_title-14.patch, failed testing.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin commented
Status: Needs work » Needs review
FileSize
1323830-rdfa_title-16.patch3.97 KB
interdiff.txt2.72 KB

This patch also corrects the tests to look for span tags instead of meta tags.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin commented
FileSize
Screen Shot 2013-12-03 at 4.58.08 PM.png106.93 KB

Here is a screenshot of the markup in D7 with patch applied.
screenshot of code markup

scor’s picture

scor CreditAttribution: scor commented
Assigned: scor » Unassigned
Status: Needs review » Needs work

This 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:

  // Adds RDFa markup to the title of the node. Because the RDFa markup is
  // added to the <h2> tag which might contain HTML code, we specify an empty
  // datatype to ensure the value of the title read by the RDFa parsers is a
  // literal.
  $variables['title_attributes_array']['property'] = empty($variables['node']->rdf_mapping['title']['predicates']) ? NULL : $variables['node']->rdf_mapping['title']['predicates'];
  $variables['title_attributes_array']['datatype'] = '';

And I believe some of the tests will need to be adjusted as a result.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin commented
FileSize
1323830-rdfa_title-19.patch4.88 KB
interdiff.txt2.08 KB

I think this is correct now, for removing the logic around teaser mode. This is a simplified, unified approach now.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin commented
Status: Needs work » Needs review

flagging needs review for testing.

scor’s picture

scor CreditAttribution: scor commented
Status: Needs review » Needs work

You're making good progress on this issue, Chris!

  1. +++ b/modules/rdf/rdf.module
@@ -471,28 +471,19 @@ function rdf_preprocess_node(&$variables) {
+  // Adds RDFa markup about the title of the node to the title_suffix
+  $title_attributes = array(
+    'content' => $variables['title'],
+    'about' => $variables['node_url'],
+  );
+  if (!empty($variables['node']->rdf_mapping['title']['predicates'])) {
+    $title_attributes['property'] = $variables['node']->rdf_mapping['title']['predicates'];

    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

    which has the about attribute already). It was needed before when we were displaying the title RDFa markup outside the node output.

    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... 

  2. +++ b/modules/rdf/rdf.module
@@ -471,28 +471,19 @@ function rdf_preprocess_node(&$variables) {
+  $element = array(
+    '#theme' => 'rdf_metadata',
+    '#metadata' => array($title_attributes),
+  );
+  $variables['title_suffix']['rdf_meta_title'] = $element;

    No need to use an intermediary $element variable here, just throw the array() in $variables[...] :) 

  3. +++ b/modules/rdf/rdf.test
@@ -301,7 +301,7 @@ class RdfMappingDefinitionTestCase extends TaxonomyWebTestCase {
-    $blog_title = $this->xpath("//meta[@property='dc:title' and @content='$node->title']");
+    $blog_title = $this->xpath("//span[@property='dc:title' and @content='$node->title']");
     $blog_meta = $this->xpath("//div[(@about='$url') and (@typeof='sioct:Weblog')]//span[contains(@property, 'dc:date') and contains(@property, 'dc:created') and @datatype='xsd:dateTime' and @content='$isoDate']");

    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)

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin commented
Status: Needs work » Needs review
FileSize
interdiff.txt4.25 KB
1323830-rdfa_title-22.patch4.25 KB

Updated the tests, and overall cleaned up interstitial arrays, and wrapped all logic in the !empty/predicates conditional.

Status: Needs review » Needs work

The last submitted patch, 22: 1323830-rdfa_title-22.patch, failed testing.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin commented
Status: Needs work » Needs review
FileSize
1323830-rdfa_title-24.patch4.79 KB

Hmm, messed that up somehow. This one should apply clean.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin commented
FileSize
1323830-rdfa_title-25.patch4.79 KB

New patch to replace missing comma per coding standards. Doh!

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented

25: 1323830-rdfa_title-25.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 25: 1323830-rdfa_title-25.patch, failed testing.

scor’s picture

scor CreditAttribution: scor commented
Issue tags: +RDF code sprint
dcam’s picture

dcam CreditAttribution: dcam commented
Issue summary: View changes
Issue tags: +Needs reroll, +Novice

#25 needs to be rerolled.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Status: Needs work » Needs review
FileSize
1323830-rdfa_title-30.patch4.8 KB

Hopefully this addresses that.

Status: Needs review » Needs work

The last submitted patch, 30: 1323830-rdfa_title-30.patch, failed testing.

scor’s picture

scor CreditAttribution: scor commented

This 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:

-        'content' => $variables['node']->title,
...
+          'content' => $variables['title'],

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.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented

#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?

    $node = $this->drupalCreateNode(array(
      'type' => 'test_bundle_hook_install',
      'title' => $this->randomName(8) . "'",
    ));
    $isoDate = date('c', $node->changed);
    $url = url('node/' . $node->nid);
    $this->drupalGet('node/' . $node->nid);

    // Ensure the mapping defined in rdf_module.test is used.
    $test_bundle_title = $this->xpath("//div[@about='$url']/span[@property='dc:title' and @content='$node->title']");

I grepped but couldn't find it.

scor’s picture

scor CreditAttribution: scor commented

re #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.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Status: Needs work » Needs review
FileSize
1323830-rdfa_title-34.patch5.59 KB

Ok, so like this I assume.

    // Ensure the mapping defined in rdf_module.test is used.
    $test_bundle_title = $this->xpath("//div[@about='$url']/span[@property='dc:title' and @content='" . check_plain($node->title) . "']");
scor’s picture

scor CreditAttribution: scor commented
Status: Needs review » Needs work

Yes, #33.2 looks good now, but we're missing #33.1.

+++ b/sites/default/default.settings.php
@@ -433,6 +433,16 @@ ini_set('session.cookie_lifetime', 2000000);
+ * HTTP Request Buffer
+ *
+ * When drupal requests a file from another webserver, it periodically checks
+ * for end-of-file and server timeouts.  The default is to read a maximum of
+ * 1024 bytes between each check.  Increasing this limit may help performance
+ * when reading large files from the network.
+ */
+# $conf['http_request_buffer'] = 1024;
+
+/**

oops :)

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Status: Needs work » Needs review
FileSize
1323830-rdfa_title-37.patch4.83 KB

Thanks!

The last submitted patch, 35: 1323830-rdfa_title-34.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 37: 1323830-rdfa_title-37.patch, failed testing.

er.pushpinderrana’s picture

er.pushpinderrana CreditAttribution: er.pushpinderrana commented
Status: Needs work » Needs review
FileSize
drupal7-rdf-title-1323830-40.patch4.84 KB
interdiff-1323830-40.txt484 bytes

Fixed the test case issue in this patch and also tested locally to ensure it get pass through Testbot.

Thanks!

Status: Needs review » Needs work

The last submitted patch, 40: drupal7-rdf-title-1323830-40.patch, failed testing.

er.pushpinderrana’s picture

er.pushpinderrana CreditAttribution: er.pushpinderrana commented
FileSize
test_case_passed.png7.14 KB

Not sure why this failure occur because this test gets passed locally attached screenshot of same.

Image Effect Test Case

mgifford queued 40: drupal7-rdf-title-1323830-40.patch for re-testing.

The last submitted patch, 40: drupal7-rdf-title-1323830-40.patch, failed testing.

scor’s picture

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)

er.pushpinderrana’s picture

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.

we just have to make sure we wrap it in double quotes in the final XPath expression.

// Ensure the mapping defined in rdf_module.test is used.
$test_bundle_title = $this->xpath("//div[@about='$url']/span[@property='dc:title' and @content='\"".check_plain($node->title)."\"']"); 

// Ensure the mapping defined in rdf_module.test is used.
$test_bundle_title = $this->xpath("//div[@about='$url']/span[@property='dc:title' and @content='\"".check_plain($node->title)."\"']");

Drupal submission also hiding (\) from above code that are placed in front of "".check_plain( and from end after ($node->title)."

Thanks again!

kay_v’s picture

kay_v CreditAttribution: kay_v commented
Status: Needs work » Needs review
FileSize
1323830-rdfa_title-47.patch4.81 KB

implemented Scor's pastebin code :)

leslieg’s picture

leslieg CreditAttribution: leslieg commented
Assigned: Unassigned » leslieg
leslieg’s picture

leslieg CreditAttribution: leslieg commented
FileSize
Screen Shot 2014-07-01 at 6.58.34 PM.png1.3 MB

Checked code diff and viewed source. Here is a screenshot of the markup in D7 with patch applied.

leslieg’s picture

leslieg CreditAttribution: leslieg commented
Assigned: leslieg » Unassigned
Status: Needs review » Reviewed & tested by the community
scor’s picture

scor CreditAttribution: scor commented

Looks good. Thank you all for your patience and great work!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 1323830-rdfa_title-47.patch, failed testing.

scor queued 47: 1323830-rdfa_title-47.patch for re-testing.

scor’s picture

scor CreditAttribution: scor commented
Status: Needs work » Reviewed & tested by the community

back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 1323830-rdfa_title-47.patch, failed testing.

scor queued 47: 1323830-rdfa_title-47.patch for re-testing.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Needs work » Needs review

This 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?

scor’s picture

scor CreditAttribution: scor commented

oops, the screenshot from #49 isn't highlighting the right markup. The relevant markup is:

      <span property="dc:title" content="test" class="rdf-meta"></span>

(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?

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

I 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)?

scor’s picture

scor CreditAttribution: scor commented

We 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 :)

scor’s picture

scor CreditAttribution: scor commented
Status: Needs review » Reviewed & tested by the community

#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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 1323830-rdfa_title-47.patch, failed testing.

dcam queued 47: 1323830-rdfa_title-47.patch for re-testing.

dcam’s picture

dcam CreditAttribution: dcam commented
Status: Needs work » Reviewed & tested by the community
lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Issue tags: -Needs reroll

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 1323830-rdfa_title-47.patch, failed testing.

mgifford queued 47: 1323830-rdfa_title-47.patch for re-testing.

The last submitted patch, 47: 1323830-rdfa_title-47.patch, failed testing.

dcam queued 47: 1323830-rdfa_title-47.patch for re-testing.

dcam’s picture

dcam CreditAttribution: dcam commented
Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 1323830-rdfa_title-47.patch, failed testing.

dcam queued 47: 1323830-rdfa_title-47.patch for re-testing.

dcam’s picture

dcam CreditAttribution: dcam commented
Status: Needs work » Reviewed & tested by the community
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +7.33 release notes

Committed to 7.x - thanks!

  • David_Rothstein committed 4d5a0ad on 7.x 
    Issue #1323830 by cwells, scor, mgifford, er.pushpinderrana, kay_v:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.