michelf / php-markdown

Parser for Markdown and Markdown Extra derived from the original Markdown.pl by John Gruber.
http://michelf.ca/projects/php-markdown/
Other
3.42k stars 530 forks source link

Better Footnote title attributes? #387

Open edent opened 1 year ago

edent commented 1 year ago

Footnote links' titles can only be set once by the library. This change would make the attribute display the content of the footnote.

How

At the moment, if a user hovers over a footnote, the title attribute displays a tool tip.

For example, if https://github.com/michelf/php-markdown/blob/eb176f173fbac58a045aff78e55f833264b34e71/Michelf/MarkdownExtra.php#L28 is set to "Read the footnote." then all footnotes have this pop-up:

Screenshot of a pop up which says 'read the footnote'

I think it would be nice if it showed users the full text of the footnote. For example:

The pop up now displays the full note

This is controlled by :

https://github.com/michelf/php-markdown/blob/eb176f173fbac58a045aff78e55f833264b34e71/Michelf/MarkdownExtra.php#L1764

The change is relatively straightforward:

if ($this->fn_link_title != "") {
    $title = trim( strip_tags( $this->footnotes[$node_id] ) );
    $title = $this->encodeAttribute( $title );
    $attr .= " title=\"$title\"";
}

That takes the text of the footnote, sanitises it, and adds it as the title element.

Would you be interested in a PR for this?

(I have also raised this with WordPress's Jetpack which is running an older version of your library - https://github.com/Automattic/jetpack/issues/27387)

michelf commented 1 year ago

How do you propose to handle footnotes like this one[^1].

[^1]: Footnotes are often one line of text, but this line of text can contain formatting such as code, emphasis, links, and images such as this one. Actually, it wouldn't surprise me to find that links are pretty common since footnotes are often used for references.

They can also go quite long with paragraphs and other block-level elements such as:

| tables | tables |
| ------ | ------ |
| rows   | rows   |

> blockquotes

    code blocks

- lists
- and more lists

So we need a strategy to deal with this. I don't think `strip_tags` is good enough.
edent commented 1 year ago

A very reasonable question!

To me, it comes down to user needs. I want a tool-tip so I can see whether the footnote says "(Smith, 1996)" or "A funny story about that, back in 1996 I saw ...".

That way, I can decide if I want to visit the footnote.

Links are handled well, I think. echo strip_tags('<a href="https://example.com/">(Smith, 1996)</a>'); spits out (Smith, 1996)

The HTML spec for title doesn't define a maximum length but, anecdotally, some browsers will truncate them.

So I would use strip_tags and then mb_substr() to knock it down to, say, a max of 100±10 characters.

What do you think to that idea?

michelf commented 1 year ago

I think truncating it if too long makes this idea reasonable. It should probably truncate at the first end of paragraph </p> and some other tags too (<table>, <blockquote>…). It'd be nice to replace images with the alt text. And spacing is interpreted differently inside title="" than in HTML, so it should probably be somehow normalized (collapsing double-spaces and newlines, then replacing <br/> with actual newlines).

Sorry if this seems a lot of work. Maybe it's not all needed.

I'm not sure if this tooltip should be the default, but at least it should be possible to enable or disable it.

edent commented 1 year ago

Things like alt text will be tricky without either parsing the DOM or using a library like https://github.com/mtibben/html2text

Similarly, stopping at specific tag or replacing <br> with \n might involve a fair bit of parsing.

So, I guess the options are:

  1. Cheat. Strip the tags, Grab the first N characters, and give users a somewhat meaningful preview.
  2. Fully parse the DOM of the generated HTML to grab the appropriate text to a suitable boundary.
  3. Generate a bespoke title from the Markdown directly.

I'm lazy, so happy to contribute a patch for (1). But if you're looking for something more comprehensive, I'll have to bow out.

michelf commented 1 year ago

Personally, I think we could cheat a bit more. Something like this:

list($title, $tail) = preg_split('{</p\s*>\s*}i', $title, 2); // split at </p>
$truncated = $tail != '';
$title = preg_replace('{<img\b[^<]\balt\s*=\s*(?:"([^"]*")|(\'([^\']*\')|(\S*)\b)[^<]*>}i', '\1\2\3', $title); // img -> alt text
$title = preg_replace('{\s+}', ' ', $title); // normalize whitespace
$title = trim($title); // remove leading & trailing whitespace
$title = preg_replace('{<br\s*/?>}i', '\n', $title); // br -> \n
$title = strip_tags($title);
$before_truncation = $title;
$title = preg_replace('{^.{0,100}+?\b}', '', $title); // cut after 100 characters, but continue for last word.
if ($truncated || $before_truncation != $title) {
   $title .= '…'; // add ellipsis after truncation
}

There's some situations where the img will be parsed incorrectly (if you include unescaped > in quoted attributes (other than the alt itself). But otherwise it should work... in theory. I didn't test it at all, feel free if you want to.

edent commented 1 year ago

Here's a slightly less regex-y way of doing things using PHP's DOMDocument

$doc = new DOMDocument();
$doc->loadHTML( mb_convert_encoding("<p>fugiat <em>consequuntur</em>. 蘵 <h2>Dolores deleniti</h2> <a href='/home'>quia voluptas</a> unde dolor <img alt='esse perspiciatis voluptas' src='img.jpg' />. <p>Autqui 🇬🇧<strong> corporisrerumfuga</strong> quo a. Fuga voluptatem rerum autem. Beatae dolorem et porro ut culpa. Est ut deserunt in quod non omnis suscipit veritatis.</p>", 'HTML', 'UTF-8' ));

$xp = new DOMXPath( $doc );

//  Replace each element with the text inside it
foreach ( $xp->query('//*/text()') as $node ) {
    $p_text = $doc->createTextNode( $node->wholeText . "\n");
    $node->parentNode->replaceChild( $p_text, $node );
}

//  Replace each <img> with its alt text
foreach ( $xp->query('//img') as $node ) {
    $alt_text = $doc->createTextNode($node->getAttribute('alt') . "\n");
    $node->parentNode->replaceChild($alt_text, $node);
}

//  Replace each <br> with a newline
foreach ( $xp->query('//br') as $node ) {
    $newline = $doc->createTextNode( "\n" );
    $node->parentNode->replaceChild( $newline, $node );
}

//  Get a plaintext representation
$title_text = html_entity_decode( strip_tags( $doc->saveHTML() ) );

//  Trim any whitespace
$title_text = trim( $title_text );

The only problem is splitting the text. Not all multibyte languages (like Chinese) have word boundaries like \b. I'm still having a think about how to solve that.

michelf commented 1 year ago

I suppose a solution could be to have two limits: 100 characters + characters until word break, but if no word break is found between the 100th and 120th character you simply cut at 100 (or any other arbitrary length).

// cut after 100 characters, but continue for last word (if less than 20 characters).
$title = preg_replace('{^.{0,100}({1,20)?\b|)}u', '', $title);

I think the u pcre flag will make it consider UTF-8 correctly (not cut in the middle of a code point). It would still be susceptible of breaking things in the middle of grapheme clusters (like for multi-code-point emojis, Korean syllables, combining characters, etc.). Fixing this is going to require something more much more complex, like inspecting character classes I suppose. Cutting only at word breaks keeps things much simpler in comparison.

edent commented 1 year ago

I couldn't get that regex to work - and I'm always a little paranoid about how readable and maintainable they are.

This should accomplish the same thing:

//  Split by space
$parts = explode( " ", $title_text );

//  Add each part to a new string until it is 100 characters long
$title = "";

foreach ( $parts as $part) {
    //  Always add the first part
    if ( mb_strlen( $title ) == 0 ) {
        $title .= $part . " ";
        //  If the first part is a very long string, reduce it to 100 characters
        if ( mb_strlen( $title ) > 100 ) {
            $title = mb_substr( $title, 0, 100 );
            break;
        }
    } else if ( ( mb_strlen( $title ) + mb_strlen( $part ) ) < 100 ) {
        //  Add the next whole word which doesn't take the total length over 100 characters
        $title .= $part . " ";
    } else {
        break;
    }
}

//  Trim any whitespace
$title = trim( $title );

//  If it has been truncated, add an ellipsis
if ( mb_strlen( $title ) < ( mb_strlen( $title_text ) ) ) {
    $title .= "…";
}

I've tested this with several long strings and it seems to work. I've also made some encoding changes to the above function so it copes better with multibyte characters.

Let me know if you'd like this as a proper PR.

edent commented 1 year ago

One last bug :-)

$this->footnotes[$node_id] returns hashed values for some elements, as defined in:

https://github.com/michelf/php-markdown/blob/eb176f173fbac58a045aff78e55f833264b34e71/Michelf/MarkdownExtra.php#L408

So, for example, a footnote which is a table is returned as B2B

Is there any way to unhash $this->footnotes[$node_id]?

michelf commented 1 year ago

$this->unhash($text) (inherited from the Markdown class)

edent commented 1 year ago

I ended up using formParagraphs() so that markdown inside a title was properly decoded first.