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.
Allow
in the Basic HTML configuration
None
None
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 !)
| 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 |
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 818616-12.patch | 2.98 KB | idebr |
| #12 | interdiff-12-9.txt | 1.06 KB | idebr |
| #12 | 818616-12.fail_.patch | 2.24 KB | idebr |
| #9 | interdiff-9-7.txt | 1.19 KB | idebr |
| #9 | 818616-9.patch | 1.92 KB | idebr |
Comments
Comment #1
sunUnfortunately, feature freeze was more than 12 months ago. I'm sorry. Let's revisit this for D8.
Comment #2
nesta_ commentedAny information about this?
Comment #3
wim leersMost 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.Comment #4
Bojhan commentedlol, thats funny. Ok lets do this :)
Comment #5
sunThe 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.
Comment #6
Bojhan commentedAhh good point, tests.
Comment #7
mgiffordStill needs tests, but the patch didn't apply.
Comment #8
jhedstromStill needs tests.
Comment #9
idebr commentedI added a test to check if the
<br>tag as well as the<br />tag pass the HTML filter correctly.Re: sun #9
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?
Comment #10
idebr commentedUnassigned Wim Leers
Comment #11
jhedstromThanks 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.
Comment #12
idebr commented@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.
Comment #13
idebr commentedFixed a typo in the beta evaluation.
Comment #15
jhedstromThis 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.
Comment #16
wim leersNice! :)
Comment #17
alexpottThis is a usability bug. Committed f9821ed and pushed to 8.0.x. Thanks!
Thank you for adding the beta evaluation to the issue summary.
Comment #20
gábor hojtsyThanks, removing from UX sprint now.