mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
127 stars 41 forks source link

nl2br utility function does not convert newlines to br tags if the newline is the only thing between two tags #12289

Closed bobsilverberg closed 5 years ago

bobsilverberg commented 6 years ago

This replaces https://github.com/mozilla/addons/issues/5844, which can be referenced for more details, but the bottom line is that given text like this:

<strong>A title:</strong>
<a href="something">A link</a>

nl2br will not add a <br /> tag added between the <strong> tag and the <a> tag, whereas given:

<strong>A title:</strong>
Some text

nl2br will add a <br /> tag added between the <strong> tag and the "Some text" text.

I'm not sure if there are valid reasons for this exclusion, or if it's just an error in the regular expression operation used for nl2br, which is currently replace(/(\r\n|\r|\n)(?!<.+?>)/g, '<br />').

What do you think @kumar303?

willdurand commented 6 years ago

Please see: https://github.com/mozilla/addons-frontend/pull/5526 (there should not be <br> between html tags)

bobsilverberg commented 6 years ago

I'm not sure that's valid here. The problem isn't that we want a <br /> inside a tag (i.e., between an <a> and an </a>), but rather we want it between the end of one tag and the beginning of another (i.e., between a </strong> and an <a>). Is the latter not valid?

willdurand commented 6 years ago

Not sure it is valid indeed. Why do you need a line break here?

bobsilverberg commented 6 years ago

Why do you need a line break here?

From the original issue, an example of some text in the description is:

<strong>Detailed installation/use instructions:</strong>
<a href="http://proofingtoolgui.org/en_installing.html">http://proofingtoolgui.org/en_installing.html</a>

The intent is to have a title wrapped with <strong> and then the link on the next line. nl2br should be adding a <br> tag between these two lines, but it is not. If it did the html would be:

<strong>Detailed installation/use instructions:</strong><br>
<a href="http://proofingtoolgui.org/en_installing.html">http://proofingtoolgui.org/en_installing.html</a>

which I believe is perfectly valid.

willdurand commented 6 years ago

We should not use <br> for that purpose IIRC. If we want two paragraphs, we need two <p> blocks.

bobsilverberg commented 6 years ago

We should not use <br> for that purpose IIRC.

Why? The html I included above is valid.

If we want two paragraphs, we need two <p> blocks.

Is the suggestion that the user needs to format their text by wrapping it in <p> blocks themselves? Also, if you're suggesting that the solution to the above case is for the html to be written as:

<p><strong>Detailed installation/use instructions:</strong></p>
<p><a href="http://proofingtoolgui.org/en_installing.html">http://proofingtoolgui.org/en_installing.html</a></p>

That won't deliver the desired results as the user wants just a line break between the title and the link, but the approach above introduces an entire blank line between the title and the link.

willdurand commented 6 years ago

ah, I see.

It is tough because depending on which html tags we have, there are valid reasons to drop the <br> tags and we often don't need them.

Is the suggestion that the user needs to format their text by wrapping it in

blocks themselves?

No, that's what most WYSIWYG editors do, though.

marcoagpinto commented 5 years ago

Hello!

Two months have gone by.

Any news regarding this issue?

ioanarusiczki commented 5 years ago

@willdurand I tested the description field, custom license, privacy policy and version notes from dev hub, using the examples mentioned by @bobsilverberg and I also added a <b>This text is bold</b> followed by a link of this format <a href="http://proofingtoolgui.org/en_installing.html">http://proofingtoolgui.org/en_installing.html</a>

The front end displays them correctly , no need to add spaces between them now. - FF64(Win10)

Before (on AMO prod)

before on amo prod

Now: fixed on AMO dev

aligned correctly