Use <mark> element for 'mark' theme hook

Here's a first stab at this.

Here's an updated patch for the /core change and removing one last MARK_NEW from common.inc.

needs review

Missed a stray MARK_READ in that last patch.

Re-queuing due to testbot failures seemingly unrelated to this patch.

#6: 1311372-theme_mark.006.patch queued for re-testing.

on my list for tonight

Rerolled and made the one-liner that jacine requested in #10

Thank you for the patch.

+++ b/core/includes/theme.incundefined
@@ -2053,19 +2053,13 @@ function theme_tablesort_indicator($variables) {
+    return ' <mark class="' . t($type) . '">' . t($type) . '</mark>';

We should not use t() for class names. We also can't call t() on a variable -- the first parameter to t() must be a string literal. That's why there were MARK_NEW, MARK_UPDATED and MARK_READ and why they probably should stay.

I agree that MARK_NEW, etc should stay, but for completely different reasons. Drupal uses node_mark() to mark nodes and comments as read, new or updated. It's simplest if node_mark returns a constant rather then a translated string and a class name.

So let's make theme_mark more useful by having an optional $content variable which takes a translated string containing the text that should be placed inside the mark element. If $content is not given but a $type is given, then we put some default text into $content and wrap that in the mark element.

This new $content variable will allow theme_mark() to be used for any legitimate usage of the HTML5 mark element, e.g. marking a highlighted passage of an existing document.

I've removed the leading space from theme_mark because that was dumb and only needed in one place in Drupal core. I've also removed the global $user bit because it was redundant with the check done in node_mark() and because it was incredibly brain-dead to put business logic in a theme function.

I've also removed the implicit check for MARK_READ in theme_mark() before returning a themed mark. theme_mark() now assumes that if you called theme('mark') that you really did want a mark element. The check for MARK_READ (i.e. I do not want a mark element) is now done before theme('mark') is called.

This theme function isn't used everywhere it could be right now.

And now it is, I think. :-) I've made the comment.tpl's $new variable and the outdated translation marker use theme_mark.

So you can now use theme_mark in 2 ways:

1. theme('mark', array('type' => MARK_NEW));
   

2. theme('mark', array('content' => t('outdated'), 'attributes' => array('class' => array('outdated'))))
   

Updated patch

CNR

Looks good

This makes a lot of sense.

+++ b/core/includes/theme.incundefined
@@ -1982,24 +1982,31 @@ function theme_tablesort_indicator($variables) {
+ *   - content (optional): A string containing the text to mark.
...
+  $content = $variables['content'];

These two things seem to contradict each other. If $content is optional, then shouldn't we do an isset() check in that line below?

+++ b/core/includes/theme.incundefined
@@ -1982,24 +1982,31 @@ function theme_tablesort_indicator($variables) {
+ *   - attributes: The attributes applied to the mark element.

Is attributes required? Or is it also optional? I think it's optional, in which case it's weird that's not specified, where it is on type and content.

These two things seem to contradict each other. If $content is optional, then shouldn't we do an isset() check in that line below?

Good question! But all 3 of those variables have default values set in hook_theme(). Note this hunk in the patch:

@@ -6598,7 +6598,7 @@ function drupal_common_theme() {
       'variables' => array('style' => NULL),
     ),
     'mark' => array(
-      'variables' => array('type' => MARK_NEW),
+      'variables' => array('type' => MARK_READ, 'content' => NULL, 'attributes' => array()),

So they will definitely exist when theme_mark is called.

Is attributes required? Or is it also optional? I think it's optional, in which case it's weird that's not specified, where it is on type and content.

Which means I'm not explaining the variables properly. Technically, most variables in the theme system will have default values and, thus, are "optional". I just rewrote that docblock to make it obvious how the "type" variable effects the other variables.

/**
 * Returns HTML for marked content; often used for new or updated content.
 *
 * @param $variables
 *   An associative array containing:
 *   - content: A string containing the text to mark.
 *   - attributes: The attributes applied to the mark element.
 *   - type: An optional integer representing the marker type to display; see
 *     MARK_NEW and MARK_UPDATED. If type is specified, the "content" does not
 *     need to be set since a default value will be provided for it.
 */

Guh. Wrong patch. This one please.

After applying the latest patch I get the following whitescreen-error:
Fatal error: Call to undefined function drupal_attributes() in /core/includes/theme.inc on line 2002

Is this a new function yet to be implemented?

#22: 1311372-22-mark-html5.patch queued for re-testing.

Here's a re-rolled patch.

Review!

#26: 1311372-26-mark-html5.patch queued for re-testing.

After talking with Jen Lapton, we decided we should remove this entire theme hook.

Here's the patch that removes the theme function.

lol. totally wrong patch file.

*sigh* Third time's a charm.

Re-roll.

@JohnAlbin I've trying to refactor theme_mark() in another issue #2062097: Remove calls to deprecated global $user in theme.inc and came to nearly the same idea.

The function needed for node, comment, tracker and translation modules so should be shared because comment module could be used for any entity soon #1907960-381: Helper issue for "Comment field"

The only thing I suggest is remove access check from theme function

Updated issue summary.

Re-rolled again.

Re-rolled the patch to apply cleanly against 8.x.

You'll want to update HistoryTimestampTest, i.e. replace

$result = $this->xpath('//span[@class=:class]', array(':class' => 'marker'));

with

$result = $this->xpath('//span[@class=:class]', array(':class' => 'mark--new'));

Questions:

• why not have a render element type, like the issue title says?
• why not apply a general class in all cases?

+++ b/core/modules/tracker/tracker.pages.inc
@@ -80,19 +80,22 @@ function tracker_page($account = NULL) {
-      $mark_build = array(
-        '#theme' => 'mark',
-        '#status' => node_mark($node->id(), $node->getChangedTime()),
-      );
...
-        'title' => array('data' => l($node->getTitle(), 'node/' . $node->id()) . ' ' . drupal_render($mark_build)),
+        'title' => array('data' => l($node->getTitle(), 'node/' . $node->id())),
...
+        $row['title']['data'] .= ' <mark class="mark--new">' . t('new') . '</mark>';
...
+        $row['title']['data'] .= ' <mark class="mark--updated">' . t('updated') . '</mark>';

We're losing the render array structure by adding in static markup. This makes it difficult to replace (or change classes accordingly).

In retrospect, these table rows really should be using a render array for the data/link instead of l(). That would allow us to append the render array with a new "marker" renderable child. This would allow us to replace just the $row['title']['data']['marker']['#markup'] instead of having to do a preg_replace() on the string.

Also, FWIW, we're kind of duplicating the efforts in #1939092: Convert theme_mark() to Twig... although this issue is completely removing the "mark" template entirely (which isn't a really good idea IMO). Twig caches everything so having a separate "mark" theme hook for the <mark> tag makes sense. It shouldn't be subject to the MARKED_NEW and MARKED_UPDATE constants though, it should be abstracted so the classes are added via preprocess and not in the template:

<mark{{attributes}}>{{value}}</mark>

Happy to mark this as a duplicate and focus efforts on #1939092: Convert theme_mark() to Twig. I was just trying to nudge this issue along.

I think this should be a separate issue (as it's changing the markup), so just postponing it until that issue is resolved.

Forgot to some other issue cleanup

It's not clear what is a current proposal after #1939092: Convert theme_mark() to Twig

@andypost I think the distinction is clear; this issue is about the markup, and that issue was about how to deliver the markup.

I'm going to mark this as a duplicate as it's being addressed over here #867830: "Unpublished" style of rendered entities is not accessible (and looks bad). Can we focus our efforts on this patch?

Ok, we're moving this back here.

Crap we missed this, the only tricky part is maybe getting this logged_in status check moved out of the template and onto the render array.

Here's a rough conversion that will likely fail, and I'm not supposed to change stable/classy so likely have to revert that change later... just want to kick the tires on this a bit...

And there is a bug fix in here now too AND this likely doesn't work well with render cache as it probably needs some cache tags and context when it's being built.

+++ b/core/modules/system/templates/mark.html.twig
@@ -9,14 +9,9 @@
+<mark{{ attributes }}>{{ text }}</mark>

Do we want to show the mark element even when the text is empty?

+++ b/core/includes/theme.inc
@@ -468,6 +468,38 @@ function theme_settings_convert_to_config(array $theme_settings, Config $config)
+      $variables['attributes']['class'][] = 'mark--new';
...
+      $variables['attributes']['class'][] = 'mark--updated';
...
+      $variables['attributes']['class'][] = 'mark--outdated';

I'm curious whether this logic should/could be part of the template instead?

To address comment #58 by @lauriii , based on all my digging into standards around the tag it is valid for it to be empty. don't think we'd want to hide it if text is empty.

@dawehner keeping that logic within theme.inc seems like a cleaner approach then moving it into the template. If moved into the template, we would end up with a template looking something like this:

{% if status is constant('MARKUP_NEW') %}
    <mark{{ attributes.addClass('markup--new') }}>{{ text }}</mark>
{% elseif status is constant('MARKUP_UPDATED') %}
    <mark{{ attributes.addClass('markup--updated') }}>{{ text }}</mark>
{% elseif status is constant('MARKUP_READ') %}
  <mark{{ attributes.addClass('markup--outdated') }}>{{ text }}</mark>
{% else %}
    <mark{{ attributes }}>{{ text }}</mark>
{% endif %}

That seems like a not-ideal scenario.

I have made an update to patch #56. Noticed that the render method in HistoryUserTimestamp.php was using the depreciated drupal_render().

Add this time to attach the correct patch file...

1. +++ b/core/includes/theme.inc
   @@ -468,6 +468,38 @@ function theme_settings_convert_to_config(array $theme_settings, Config $config)
   + */
   +function template_preprocess_mark(&$variables) {
   +  switch ($variables['status']) {
   +    case MARK_NEW:
   +      $variables['text'] = t('New');
   +      $variables['attributes']['class'][] = 'mark--new';
   +      break;
   +    case MARK_UPDATED:
   +      $variables['text'] = t('Updated');
   +      $variables['attributes']['class'][] = 'mark--updated';
   +      break;
   +    case MARK_READ:
   +      $variables['text'] = t('Read');
   +      $variables['attributes']['class'][] = 'mark--outdated';
   

   This looks something which could be totally moved to the template. Well these days we try to basically remove preprocess functions as much as possible.

2. +++ b/core/modules/node/src/NodeListBuilder.php
   @@ -105,7 +105,8 @@ public function buildRow(EntityInterface $entity) {
   -      '#mark_type' => node_mark($entity->id(), $entity->getChangedTime()),
   +      '#status' => node_mark($entity->id(), $entity->getChangedTime()),
   +      '#access' => (\Drupal::currentUser()->isAuthenticated() && ($mark == MARK_NEW || $mark == MARK_UPDATED)),
        );
   

   This would need a cache context so it changes between authenticated and non authenticated users.

• Moved the logic back to template.
• Reverted changes made for Classy and Stable because of BC.
• Added cache context for the user permissions.

+++ b/core/modules/system/templates/mark.html.twig
@@ -13,10 +13,18 @@
 {% if logged_in %}

Let's please move the logged_in check out of this template, that logic doesn't need to be representative of a mark tag, it should be more controller logic deciding when to use that element.

Rerolled new patch without macros and added missing comma for HistoryUserTimestamp

Thank you @lauriii. Here's an iteration on that and fix the missing variable bug and removes the markup changes in stable.

This looks beautiful to me, issues mentioned on #65 are addressed as well.

1. +++ b/core/modules/history/src/Plugin/views/field/HistoryUserTimestamp.php
   @@ -100,8 +100,15 @@ public function render(ResultRow $values) {
   +        '#cache' => array(
   +          'contexts' => array(
   +            'user.permissions',
   +          ),
   +        ),
   
   +++ b/core/modules/node/src/NodeListBuilder.php
   @@ -103,9 +103,11 @@ public function buildHeader() {
   +      '#access' => (\Drupal::currentUser()->isAuthenticated() && ($node_mark == MARK_NEW || $node_mark == MARK_UPDATED)),
   

   This cache context does not match the logic. It describes far more variations than what's actually going on here: a binary "anon or auth" variation.

   The proper cache context to use here is user.roles:authenticated.

2. +++ b/core/modules/system/templates/mark.html.twig
   @@ -13,10 +13,10 @@
   -{% if logged_in %}
   -  {% if status is constant('MARK_NEW') %}
   -    {{ 'New'|t }}
   -  {% elseif status is constant('MARK_UPDATED') %}
   -    {{ 'Updated'|t }}
   -  {% endif %}
   +{% if status is constant('MARK_NEW') %}
   +  <mark{{ attributes }}>{{ 'New'|t }}</mark>
   +{% elseif status is constant('MARK_UPDATED') %}
   +  <mark{{ attributes }}>{{ 'Updated'|t }}</mark>
   +{% elseif status is constant('MARK_READ') %}
   +  <mark{{ attributes }}>{{ 'Read'|t }}</mark>
   

   SO MUCH BETTER!

3. +++ b/core/themes/stable/templates/content/mark.html.twig
   @@ -16,5 +16,7 @@
   +  {% elseif status is constant('MARK_READ') %}
   +    {{ 'Read'|t }}
   

   Except this is new? We never had Read markers before. This is a huge visual change. Let's have screenshots to properly assess this before committing.

Note the cache context was okay: it would have ensured correct render caching. But it is not optimal, because it causes more variations than necessary.

So, this is not about security or correctness, this is about efficiency.

+++ b/core/modules/system/templates/mark.html.twig
@@ -13,10 +13,10 @@
-{% if logged_in %}
-  {% if status is constant('MARK_NEW') %}
-    {{ 'New'|t }}
-  {% elseif status is constant('MARK_UPDATED') %}
-    {{ 'Updated'|t }}
-  {% endif %}
+{% if status is constant('MARK_NEW') %}
+  <mark{{ attributes }}>{{ 'New'|t }}</mark>
+{% elseif status is constant('MARK_UPDATED') %}
+  <mark{{ attributes }}>{{ 'Updated'|t }}</mark>
+{% elseif status is constant('MARK_READ') %}
+  <mark{{ attributes }}>{{ 'Read'|t }}</mark>

Are we getting rid of the 'mark-XXX' classes? Or should this section be updated to be
{{ attributes.addClass('mark-XXX') }}

#73.3: It is not actually a change for core's use cases because we never actually show that when it is read.
'#access' => ($mark == MARK_NEW || $mark == MARK_UPDATED)

#75: We didn't use to have any classes in core so this doesn't affect the situation at all. What we do is simply wrap the text with mark element.

Interesting/fair.

+++ b/core/modules/node/src/NodeListBuilder.php
@@ -103,9 +103,11 @@ public function buildHeader() {
+      '#access' => (\Drupal::currentUser()->isAuthenticated() && ($node_mark == MARK_NEW || $node_mark == MARK_UPDATED)),

Doesn't this also need the user.roles.authenticated cache context?

Also while I'm here, the docs for mark don't make me 100% confident it's the right thing here for 'new'/'updated'. https://developer.mozilla.org/en/docs/Web/HTML/Element/mark

i.e. we should definitely be using it in https://api.drupal.org/api/drupal/core%21modules%21search%21search.modul... instead of strong tags because that's explicitly about highlighting a particular section of text based on the context that it's being shown in a search result for the word that's highlighted.

I don't see the same applicability to new/updated.

Of course if we don't use it, that makes the 'mark' element very unfortunately named.

#78: that is already there, see #76.

@WimLeers I see it in HistoryUserTimestamp but not in NodeListBuilder?

Since it hasn't really been discussed on this issue I'd love to see a response to #79. I did a brief search and turned up this:

Indicate new items in a list—If you have a list of items that is regularly being updated, you can indicate the new items with the MARK element to make it easier for readers to see what’s been added.

– http://webdesign.about.com/od/html5tags/a/understanding-the-mark-element...

Not sure I'd consider webdesign.about.com the best resource but at least it's not totally unheard of…

From the W3C: "The element represents a run of text in one document marked or highlighted for reference purposes, due to its relevance in another context. "

Hmm.. Yes.. This doesn't quite seem what what they meant. New & Updated are useful to highlight, but don't seem to fit well with examples. Even "Indicate new items in a list" would probably be a matter of wrapping the string with the new item, rather than attaching a New behind it.

Updating the summary with some references.

Here's another article about the mark element: HTML5 Doctors: Draw attention with mark.

One of the items in the W3C definition:

In the main text, the highlighted text typically marks text that may be of special relevance for the user's current activity, like search results.

Coming to a page and seeing items that are new or updated seems like items of special relevance to a user's current activity. There isn't always a perfect HTML element to convey the right semantics, and I can see that the mark element may be a bit of a stretch here. That said, I think the mark element conveys better and more useful semantics than a simple span, so I think it is worth making the change.

+++ b/core/modules/history/src/Plugin/views/field/HistoryUserTimestamp.php
@@ -100,8 +100,15 @@ public function render(ResultRow $values) {
+        '#access' => ($mark == MARK_NEW || $mark == MARK_UPDATED),

+++ b/core/modules/system/templates/mark.html.twig
@@ -13,10 +13,10 @@
+{% if status is constant('MARK_NEW') %}
...
+{% elseif status is constant('MARK_UPDATED') %}
...
+{% elseif status is constant('MARK_READ') %}

+++ b/core/themes/classy/templates/content/mark.html.twig
@@ -13,8 +13,10 @@
   {% if status is constant('MARK_NEW') %}
...
   {% elseif status is constant('MARK_UPDATED') %}
...
+  {% elseif status is constant('MARK_READ') %}

+++ b/core/themes/stable/templates/content/mark.html.twig
@@ -16,5 +16,7 @@
   {% elseif status is constant('MARK_UPDATED') %}
...
+  {% elseif status is constant('MARK_READ') %}

Let's replace constants as well here

I'm still not convinced that this is applicable at all, see for example https://developer.mozilla.org/en-US/docs/Web/HTML/Element/mark