backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 38 forks source link

Filtered / Basic HTML format should disable filter_autop #6429

Open leeksoup opened 4 months ago

leeksoup commented 4 months ago

Description of the need

The "Convert line breaks into HTML" / filter_autop filter breaks text_summary() when combined with CKEditor. See https://github.com/backdrop/backdrop-issues/issues/6423

It is also redundant in an Editor configuration, as is "Convert URLs into links."

Idk whether to consider this a bug report or a feature request, honestly.

Proposed solution

Disable the following filters in the default Filtered HTML and Full HTML configurations:

Alternatives that have been considered

Each site owner who uses trimmed / auto-generated summary with text_summary() can disable these filters.

Additional information

Edit: I picked out the format names from the documentation at https://docs.backdropcms.org/documentation/text-formats-editors-filters but it appears that that is out of date and needs to be updated wrt format names and settings. I will update issue https://github.com/backdrop-ops/docs.backdropcms.org/issues/233 accordingly.

Draft of feature description for Press Release (1 paragraph at most)

Backdrop now disables redundant filters in the default Editor configurations.

indigoxela commented 4 months ago

Idk whether to consider this a bug report or a feature request, honestly.

Neither me, honestly. :wink:

FTR: the first thing I always turn off when configuring filters for a new project, are exactly these two. We inherited that filter combo from Drupal, but with a WYSIWYG editor enabled (which we do by default on Basic format), these filters make no sense at all.

I'm voting for disabling filter_url and filter_autop in standard_install() (core/profiles/standard/standard.install) - for the Basic format.

indigoxela commented 4 months ago

@leeksoup many thanks for providing this pull request! :pray:

However, there's a little TODO left. One test (ViewsHandlerAreaTextTest) fails in all three PHP versions. It seems that this test relies on the filter being on. Probably unrelated, but still the test needs some update. I'm guessing, the test expects some <br> being there, but there aren't. (But I didn't dig deeper, yet.)

leeksoup commented 4 months ago

@indigoxela you're welcome. :)

Thanks for the explanation. I saw that some tests failed but had no idea what to do with that information.

indigoxela commented 4 months ago

I saw that some tests failed but had no idea what to do with that information.

They need fixing. :wink:

Two tests have failures, both because of expected <p>, which isn't there anymore, as the default format has the autop filter off.

  1. LocaleMultilingualFieldsFunctionalTest

The xpath search fails, as it currently seeks for the paragraph inside the div. File: core/modules/locale/tests/locale.test

  1. ViewsHandlerAreaTextTest

The test actually compares the output of different filters, check_markup() uses the fallback format by default, but the test should actually correctly compare with the same format applied - default_format(). File: core/modules/views/tests/handlers/views_handler_area_text.test

@leeksoup this issue turns out to be a bit more of a deep-dive into our testing framework for you. :wink: Are you familiar with Simpletest and/or xpath? Are you willing to accept the challenge to fix the failing functional tests? It's no witchcraft, but still a bit more, than you might have expected. :wink:

leeksoup commented 4 months ago

They need fixing. 😉

I should have guessed that. 😏

Are you familiar with Simpletest and/or xpath? Are you willing to accept the challenge to fix the failing functional tests? It's no witchcraft, but still a bit more, than you might have expected. 😉

No, I have no familiarity with those. Yes, more than I expected for sure. I don't think I can invest the time right now to figure it out and fix them. Sorry. :cry:

indigoxela commented 4 months ago

I don't think I can invest the time right now to figure it out and fix them.

That's OK, I already suspected, that this might be a little over the top. :wink:

Still, many thanks for starting this topic by opening an issue. :+1: