The purpose of the placeholder is to wrap replaced text in markup so that a themer can identify and manipulate it. The <em> tag has semantic value.

The em element represents stress emphasis of its contents

WHATWG

But we cannot know the semantics of something as generic as a placeholder. Therefore, the markup of the placeholder should be a simple, non-semantic <span>.

The span element doesn't mean anything on its own, but can be useful when used together with the global attributes, e.g. class, lang, or dir.

WHATWG

CommentFileSizeAuthor
#19 percent_grep.txt161.42 KBjhr
#15 drupal_placeholder_grep.txt22.2 KBjhr
#1 1447678_drupal-placeholder_1.patch883 bytesjessebeach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
Status: Active » Needs review
FileSize
1447678_drupal-placeholder_1.patch883 bytes

Simple one-liner that replaces <em> with <span>.

Status: Needs review » Needs work

The last submitted patch, 1447678_drupal-placeholder_1.patch, failed testing.

jessebeach’s picture

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

#1: 1447678_drupal-placeholder_1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1447678_drupal-placeholder_1.patch, failed testing.

Heine’s picture

Heine CreditAttribution: Heine commented

Isn't this desired / by design?

From the function's PHPDOC:

Formats text for emphasized display in a placeholder inside a sentence.

Emphasis added.

For non-emphasized display, @ can be used.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

Heine, the @ case doesn't provide a placeholder wrapper

function format_string($string, array $args = array()) {
  // Transform arguments before inserting them.
  foreach ($args as $key => $value) {
    switch ($key[0]) {
      case '@':
        // Escaped only.
        $args[$key] = check_plain($value);
        break;

      case '%':
      default:
        // Escaped and placeholder.
        $args[$key] = drupal_placeholder($value);
        break;

      case '!':
        // Pass-through.
    }
  }
  return strtr($string, $args);
}

So the only choice without this change is a semantically-incorrect wrapper, or no wrapper. Changing <em> to <span> makes this function more generic and doesn't force a themer to unitalicize the .placeholder class in their theme.

I'll change the function comment in a future patch with some work on those failing tests as well.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Issue tags: +html5, +sprint

I agree with @jessebeach on this. em is definitely not right for this. Also, tagging this for the current HTML5 sprint.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Component: drupal.module » markup

Also, technically this is a markup issue.

Snugug’s picture

Snugug CreditAttribution: Snugug commented

If we're deciding to use non-semantic markup for this, I know the favorite of some of the Big Names in Front End™ like Divya and Lea Verou is the <b> tag as it's short, sweet, and to the point. Thoughts on that instead of <span>?

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Doesn't this cause a regression in the actual message display though since the text would no longer be highlighted?

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

Dave, which 'this' do you mean? Using <b> or using <span>?

This came up with this string that I encountered in the aggregator module: <em>X days Y min</em> old

Wrapping with <em> creates a weird visual italicizing in the middle of the string, leaving "old" unitalicized. Also, in this context, <em> is unnecessary and semantically wrong. We're not trying to redefine the meaning of X days Y min in displaying the published date of an item. We're trying to create a hook for themers to hang styles on if they choose.

If the italicized styling is desired in some cases, then we can achieve the styling with CSS, not markup, to address styling regressions.

Heine’s picture

Heine CreditAttribution: Heine commented

So, wouldn't adding a placeholder span for @, ! and an extra emphasized equivalent of ! be the way forward? Once again, it seems to me that emphasis, NOT styling is the purpose of the % placeholder.

An alternative approach would be to ditch % altogether and simply add desired HTML to the string.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

If this one function is meant to wrap text in <em>, then why not call it theme_em? This would suggest that we also have functions or cases for <strong>, <i>, and <b>.

Taking such a route complicates what should be a simple, generic, inline text wrapping function that utilizes a <span> because it has the widest applicability and no semantic assumptions associated with it.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

Taking such a route complicates what should be a simple, generic, inline text wrapping function that utilizes a because it has the widest applicability and no semantic assumptions associated with it.

I agree with this as well.

We should look at the examples in core where this is being used and evaluate from that. If only I could figure out regex I would paste a list.

jhr’s picture

jhr CreditAttribution: jhr commented
FileSize
drupal_placeholder_grep.txt22.2 KB

Hope the attachment helps. Colons(:) around the line # show that drupal_placeholder is in it. 

core/modules/filter/filter.module:336:

grep crashcourse:

grep command

/drupal-8.x-dev$  grep drupal_placeholder * -r -n -20 > drupal_placeholder_grep.txt

grep
drupal_placeholder (find this string, fyi: escape spaces like 'function\ drupal_placeholder')
* (in any file)
-r = recursive (sub directories)
-n = show line numbers
-20 = show +/- 20 lines

<Druplicon> grep -rinC3 "string to find" /folder/to/search/recursively (where -Cn is how many lines to display around the text, -r is recursive, -i is case-insensitive, and -n prints the line numbers)

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

Aww, thank you so much @jhr! You rock. :o)

Is there a way to only search for occurrences of % inside t()?

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

@jhr, thank you! This is really helpful.

I realize that if this patch is going o work, I'll need to replace the structure-styling of the <em> tag with CSS for the place (like text formats) where one currently expects italicized text.

If we could find the % variables in t functions that Jacine suggested, that would really get this effort springboarded. Thank you!

Heine’s picture

Heine CreditAttribution: Heine commented

Wouldn't it be better to just replace % with @ ?

jhr’s picture

jhr CreditAttribution: jhr commented
FileSize
percent_grep.txt161.42 KB

This file has 858 lines, but with some false positives.

grep -rn -E "\Wt\([^\)%]*%" * > .percent_grep.txt

The grep is just searching for a '%' after a 't(', and then writes the output to a hidden file (.percent_grep.txt) to keep from falling into an infinite loop by searching for text in the file that it's currently writing to.

Also, it's limited to one line at a time. So if % shows up after a line-break, it won't detect it.
ex: t( "lorem ipsum doloret " .
"%var lorem ipsum etc", etc.. );

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

Heine, perhaps. I'm bent on determining if <em> is indeed semantically correct in all the instances that it's used in core or if this function is more useful with a generic tag like <span>.

If we are using this function to wrap text to represent "stress emphasis of its contents", then we're using it correctly. Otherwise, the inertia of past practice is simply keeping us from improving.

jhr’s picture

jhr CreditAttribution: jhr commented

Manually changing this would be a nightmare, but programmatically it's manageable. A whole bunch of review in a mega patch.
Try giving this <em> tag a special class, <em class=1447678> and changing the text-color & background to make it stand out.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Title: drupal_placeholder should use non-semantic <span> instead of semantic <em> » Fix strings with broken semantics caused by using %variable placeholder incorrectly

Can I say that the thought of Drupal outputting <span class="placeholder">text</span> makes me want to stab my eyes? Replacing a semantic element with an unsemantic one because some strings are using it incorrectly is a bad idea. Its the strings that need to change, not drupal_placeholder().

From the docs from format_string():

Occurrences in $string of any key in $args are replaced with the corresponding value, after sanitization. The sanitization function depends on the first character of the key:

  • !variable: Inserted as is. Use this for text that has already been sanitized.
  • @variable: Escaped to HTML using check_plain(). Use this for anything displayed on a page on the site.
  • %variable: Escaped as a placeholder for user-submitted content using drupal_placeholder(), which shows up as <em>emphasized</em> text.

The purpose of %variable is to wrap it in an em. Otherwise, you'd just use @variable.

Or you'd add the element you actually want and put it around the actual text you want wrapped. For example, taking the <em>X days Y min</em> old snippet from aggregator above, it seems clear that aggregator is simply calling t() incorrectly. It should be t('<em>@time old</em>', array('@time' => $time)) instead of t('%time old', array('%time' => $time)). It's not the $time text snippet that deserves semantic distinction; its the entire phrase.

So, I oppose changing the HTML element used by t() and format_string(). It's <em> by design.

It’s the strings that need to be fixed, not the function.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
Status: Needs work » Closed (won't fix)

Conceded.