Problem/Motivation

The <br> tag should be allowed for the "Basic HTML" text format. Right now, if you use shift+return in CKEditor, a <br> will be inserted, but will be filtered away upon output due to this configuration mismatch.

Proposed resolution

Allow
in the Basic HTML configuration

Remaining tasks

  • Write a patch
  • Review

User interface changes

None

API changes

None

Original report by @DjebbZ

Hi

The defaults Filtered HTML format allow for this tags :
<a> <em> <strong> <cite> <blockquote> <code> <ul> <ol> <li> <dl> <dt> <dd>
and it converts line breaks smartly into p and br tags.

Which means written p and br tags are not interpreted !

So better defaults for Allowed HTML should be :
<a> <em> <strong> <cite> <blockquote> <code> <ul> <ol> <li> <dl> <dt> <dd> <p> <br />

I tested this and it works for both <br /> and <br>.

Here's a pretty simple patch for the filter.module (My first ever !)

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority Normal because it does not affect any APIs
Unfrozen changes Unfrozen because it improves the usability of WYSIWYG formatted text.
Prioritized changes The main goal of this issue is usability.
Disruption Not disruptive
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Version: 7.0-alpha5 » 8.x-dev

Unfortunately, feature freeze was more than 12 months ago. I'm sorry. Let's revisit this for D8.

nesta_’s picture

Any information about this?

Wim Leers’s picture

Title: Better defaults of allowed HTML for the format "Filtered HTML" » Allow <br> in "Basic HTML" text format
Assigned: Unassigned » Wim Leers
Priority: Normal » Major
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Spark, +sprint, +Usability, +CKEditor in core
FileSize
875 bytes

Most of this has long been fixed: see https://drupal.org/node/1941642.

But, the <br> tag should indeed also be allowed for the "Basic HTML" text format. Right now, if you use shift+return in CKEditor, a <br> will be inserted, but will be filtered away upon output due to this configuration mismatch.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

lol, thats funny. Ok lets do this :)

sun’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

The HTML filter had problems with unclosed <br> as well as self-closing <br/> tags in the past, so this should come with tests.

We should also double-check the test coverage for the paragraph filter, as well as the combination of HTML filter + paragraph filter.

Bojhan’s picture

Ahh good point, tests.

mgifford’s picture

Status: Needs work » Needs review
FileSize
750 bytes

Still needs tests, but the patch didn't apply.

jhedstrom’s picture

Status: Needs review » Needs work

Still needs tests.

idebr’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
1.19 KB

I added a test to check if the <br> tag as well as the <br /> tag pass the HTML filter correctly.

Re: sun #9

We should also double-check the test coverage for the paragraph filter, as well as the combination of HTML filter + paragraph filter.

Both the paragraph filter as well as the HTML filter have individual unit tests, but I could not find any tests for a combination of filters. Does your remark still apply? If so, could you elaborate on what should be tested?

idebr’s picture

Assigned: Wim Leers » Unassigned

Unassigned Wim Leers

jhedstrom’s picture

Issue tags: -Needs tests

Thanks for writing tests ~idebr! Could you perhaps upload a patch that contains *just* the test to illustrate the current failing behavior?

Do folks know if this would still be accepted for 8.0? Seems like a nice usability enhancement, which is allowed I think.

idebr’s picture

Issue summary: View changes
FileSize
2.24 KB
1.06 KB
2.98 KB

@jhedstrom The new tests only test the filtering on <br> tags actually applies correctly per suggestion of sun in #5. I have added an additional test for the additional configuration in the Standard profile.

I have also updated the issue summary to reflect the contents of the patch and included a beta evaluation to explain this is a usability improvement.

idebr’s picture

Issue summary: View changes

Fixed a typo in the beta evaluation.

The last submitted patch, 12: 818616-12.fail_.patch, failed testing.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

This now has tests, and as per the beta phase evaluation, it should be allowed in 8.0 as it resolves usability issue with the WYSIWYG.

Wim Leers’s picture

Nice! :)

alexpott’s picture

Category: Feature request » Bug report
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

This is a usability bug. Committed f9821ed and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the issue summary.

  • alexpott committed f9821ed on 8.0.x
    Issue #818616 by idebr, Wim Leers, DjebbZ, mgifford: Allow <br> in "...

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Version: 8.0.x-dev » 8.0.0
Issue tags: -sprint

Thanks, removing from UX sprint now.