search

backdrop / backdrop-issues

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

[UX] Change the name of the "Full HTML" text format to "Raw HTML" #4499

Closed stpaultim closed 3 years ago

stpaultim commented 4 years ago

This has been discussed elsewhere, but I can't find an issue that focuses attention on this potential fix to what I think is a serious UX bug, so I opened this issue.

Description of the bug

Calling this a bug, because it's a serious UX bug in my opinion. As I've described in issue #1519, I find it highly confusing to have one filter called "Filtered HTML" that provides limited WYSIWYG options and then another text format called "Full HTML" which does not provide any.

I understand that this perception is a misunderstanding of the purpose of the "Full HTML" text format, which is why I propose changing the name. This solutions has been discussed in other issues (#1519) with some general agreement.

This is a complicated issue issue and is related to several other issues being discussed. In #1519, I proposed simply removing this text format from standard install profile as a temporary fix until larger issues were addressed. That solution was met with much disapproval (for understandable reasons).

My own view is that leaving this text format as it is, is a serious UX bug and such I would like to formally propose another potential fix (which has been discussed in other places) to change this text format from "Full HTML" to "RAW HTML."

Steps To Reproduce

https://github.com/backdrop/backdrop-issues/issues/1519#issuecomment-517901209

Additional information

In this comment: https://github.com/backdrop/backdrop-issues/issues/1519#issuecomment-569705723 @jlfranklin suggests that we change the name, but keep the old machine name. @jenlampton later questioned whether or not that is necessary.

If we adopt this fix, then I think we can close #1519.

RELATED ISSUE: [UX] Rename name "Filtered HTML" input format to "Basic" - https://github.com/backdrop/backdrop-issues/issues/1188

Status or Next Steps:

Issue Advocate = @stpaultim

jenlampton commented 4 years ago

I love this suggestion. On 100% of my sites I rename the filter to "HTML code". Raw is just as good.

klonos commented 4 years ago

I'm supportive of this too 👍 ...it could be a combination of the two names @jenlampton and @stpaultim call this thing: "Raw HTML code" 😅

jackaponte commented 4 years ago

Heh---I had no idea that Full HTML was actually meant to be a Raw HTML text format. I thought it was just a bug that the WYSIWYG was disabled. Always thought "Full HTML" in Drupal meant "Unrestricted HTML tags, but still a WYSIWYG." Does seem like a good idea to clear up the confusion!

jenlampton commented 4 years ago

The issue here is that we ship with these two formats out of the box, and we should name them appropriately.

Full HTML was actually meant to be a Raw HTML text format

As with all things Drupal "meant to be" is in the eye of the beholder... I'm not sure it was "meant to be" Raw HTML, but since that's what it is, we should tell people that.

Always thought "Full HTML" in Drupal meant "Unrestricted HTML tags, but still a WYSIWYG."

Drupal has never had a WYSIWYG until D8 -- so formats in Drupal were never about what happened on the front end (editor) only what happened on the back end (filters). In that sense, you are absolutely correct about the Unrestricted HTML tags bit.

I thought it was just a bug that the WYSIWYG was disabled.

In general, it's not a safe thing to do to add an visual editor onto a format with no HTML tag filtering because it gives editors a false sense of security -- that's why we don't do it in core (so, not a bug). That said, if you want to add an editor to this format you certainly can! But once it has an editor, maybe change it's name so it's not Raw HTML :)

klonos commented 4 years ago

...renamed the issue to clarify what the goal is.

klonos commented 4 years ago

...also, this is a UX task as is (still legitimate for inclusion in bug fix releases); if we want it to be a bug, then the issue title should be something like [UX] Text formats: the "Full HTML" label is confusing/misleading.

jenlampton commented 4 years ago

I think @quicksketch would disagree with changing the human name without also changing the machine name.

And I don't think we can change the human name on existing sites because we won't know what people have chosen to do with this format, if it has a rich text editor it won't be raw anymore.

How about we change both on new installs only?

klonos commented 4 years ago

How about we change both on new installs only?

Yeah, I wouldn't mind that 👍 ...let's discuss this during our next dev meeting please.

jackaponte commented 4 years ago

How about we change both on new installs only?

Agreed, @jenlampton! Especially because I've got a few existing sites using Full HTML with a WYSIWYG and those site editors might notice and get confused!

stpaultim commented 4 years ago

How about we change both on new installs only?

If I understand this correctly. The 'full html' ~install profile~ text format is not created by any module in core, it's generated by the standard install profile during site creation. So, if I understand this correctly, this issue will only effect new sites for the most part.

It's not clear to me if there are other implications built into core. I believe that someone raised the issue of maintaining the old machine name, in case there are references to the install profile in core or contrib code. BUT, I'm not sure that is a real issue - @jenlampton has suggested that it might not be. Does anyone else have any ideas about how the name of this text format might affect core or existing contrib modules?

If not, then it's an almost trivial change and will only effect new sites.

Am I missing anything?

https://github.com/backdrop/backdrop/blob/1.x/core/profiles/standard/standard.install

..renamed the issue to clarify what the goal is.

I'm not sure that the goal of the issue is to ONLY change the human readable name. That was simply a comment raised in the prior issue that I included in the summary so that it does not get lost. The goal is to change the name, however that works best.

jenlampton commented 4 years ago

Does anyone else have any ideas about how the name of this text format might affect core or existing contrib modules?

In core, the string full_html only appears in tests, in documentation (in a .api.php file) and in the install profile. Changing the name shouldn't affect anything critical.

There are a few of contrib modules that do look a format with this machine name specifically:

This was what came up in a search through all the contrib modules I've worked on -- so only 134 of the 676 for Backdrop. Many of these modules are smart about it, and check if that format exists before they use it:

if (filter_format_load('full_html')) {

In core we have the function filter_default_format() that helps contrib get the default filter format for the current user. I don't know why biblio and eu_cookie_compliance aren't using that, but that might be the correct fix in some of these cases.

indigoxela commented 4 years ago

Ehm... regarding "good first issue"...

There are a lot of tests that need to get fixed, which makes this all harder. Also, we need to consider the potential of this issue to be a contrib-breaker. I belief, we have easier ones. :wink:

Egarbeil commented 4 years ago

I think it would create confusion for end users. The person editing the node does not actually type in html code. That's what I think of when you say Raw HTML. Full HTML format does strip out some tags and is not source code to end users. Full HTML still uses the ckeditor WSIWYG editor and does strip out some tags. This part can also be configured. I think it would be confusing to call it Raw HTML code because the end user does not type in the actual HTML code. We've had to create a text format is RAW HTML (We call it pretty much that or PHP Code, which it isn't) that does not allow ckeditor and is strictly code and does no filtering (think scripts like constant contact forms, etc..)

stpaultim commented 4 years ago

Full HTML still uses the ckeditor WSIWYG editor and does strip out some tags.

@Egarbeil - I think that is part of the confusion. Full HTML does not use CKEDITOR and does not strip out any tags. Between this issue and https://github.com/backdrop/backdrop-issues/issues/1519 there seems to be widespread consensus that the current name "Full HTML" is actually causing a great deal of confusion. We're looking for a way to alleviate that confusion and I'm open to other ideas.

Full_HTML___Backdrop1

Egarbeil commented 4 years ago

Whenever I’ve used Full HTML, it always uses ckeditor by default and always has on Drupal 6 and 7. For us, Full HTML was always used with all available HTML code, but no PHP or script code, etc. allowed, plus a bunch of enabled filters.. This is what it looks like for me in Backdrop. In Drupal it was 2 steps, first configure the text format, then configure the ckeditor settings associated with that format (always 2 - Full and Filtered).

Filtered HTML is extremely limited (no format or styles, no tables, no images or file uploads etc) and used primarily by site visitors or comments.

We have another Format that I think corresponds to what you’re talking about and we called it Raw HTML or PHP Code, which we primarily use for embedding constant contact forms, etc. This format is not used by anyone but administrators. It does not use ckeditor.

On some sites, we had four - an extra one just for Display Suite which had different settings than the others. That was not the standard for us though.

Warmest regards,

Elisabeth Garbeil, MBA PMP Brainwrap LLC "Being on the web doesn't have to make your head hurt." www.brainwrap.com (810) 560-7181 egarbeil@brainwrap.com

On Oct 8, 2020, at 7:01 PM, Tim Erickson notifications@github.com wrote:

Full HTML still uses the ckeditor WSIWYG editor and does strip out some tags.

@Egarbeil https://github.com/Egarbeil - I think that is part of the confusion. Full HTML does not use CKEDITOR and does not strip out any tags. Between this issue and #1519 https://github.com/backdrop/backdrop-issues/issues/1519 there seems to be widespread consensus that the current name "Full HTML" is actually causing a great deal of confusion. We're looking for a way to alleviate that confusion and I'm open to other ideas.

https://user-images.githubusercontent.com/3144571/95521851-34c1f880-0990-11eb-9afe-9c0eedc19048.png — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/4499#issuecomment-705866662, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ4HZVXSYF5DQOT4LSZ2J2LSJZAGNANCNFSM4PHWLAVA.

indigoxela commented 4 years ago

@stpaultim There's confusion, I agree, but it might be on your side... :wink:

Full HTML in Drupal and Backdrop is usually the one that doesn't restrict allowed tags. Hence the name. In contrast to Filtered HTML, which does restrict tags.

Raw HTML implies that this format has no editor. Which is wrong. Any admin can add the editor to it and then it's not "raw" anymore.

Anyway, I have no strong feelings about this change.

klonos commented 4 years ago

Full HTML in Drupal and Backdrop is usually the one that doesn't restrict allowed tags. ...

Correct 👍

Raw HTML implies that this format has no editor. Which is wrong.

No it's not. OOTB that is exactly what happens.

Any admin can add the editor to it and then it's not "raw" anymore.

Yup, and until that happens, there is a UX WTF. We shouldn't be assuming that Backdrop is being used by people that have been using Drupal/Backdrop for a long time and know what needs to be done. The UX issue here has been described perfectly by @stpaultim in #1519, and I find it to be a valid observation/problem:

I find it very confusing that I have a text editor for Filtered HTML, but that the text editor goes away when I switch to Full HTML. I expect to have more CKEditor options available with Full HTML.

It seems to me that another option would be to enable CKEditor in the "Full HTML" format OOTB, since that would solve the above UX problem (if people want to enter "raw" HTML, they can still use the "source" button CKEditor provides). Perhaps combine this with renaming "Full HTML" to "Unfiltered HTML" or "Unrestricted HTML".

I will take some time to read through this entire issue here, as well as #1519, and then try to sum up all these issues, in hope that it would help us reach a conclusion re solution. I would like to do that in a separate meta-issue, which simply lists all the UX issues. Although specific solutions can be proposed in the discussion, I would like us to avoid making that issue solution-specific (unlike #1519 and this issue here) until we have reached consensus.

stpaultim commented 4 years ago

I plan to advocate for this change. I am hoping that we have enough consensus to make this change quick enough that I'll be able to move on and advocate for another issue (maybe Backup and Migrate in core).

I believe that we already have more opinions than usual on this issue and that most people either support or are ambivalent about this change. I went through this issue and #1519 and listed everyone that has commented and a summary of what I understand to be their opinions. I encourage anyone to correct my understanding if I have it wrong, elaborate if you think it helpful, or add your voice to the mix.

@Graham-72 - Doesn’t mind change @olafgrabienski - Supports name change (had concerns about removing filter) @jfranklin - Agrees with change to human readable name, suggested we keep machine name the same. @jenlampton - supports name change (strongly?) @docwilmot - undecided (??) @klonos - supports name change (strongly?) @findlabnet - (??) @indigoxela - “I have no strong feelings about this change” @egarbeil - Raised reservations about this change. @jackaponte - Seems supportive of the idea @stpaultim - is advocating for this issue, but also interested in more opinions

NEXT STEPS:

QUESTIONS:

docwilmot commented 4 years ago

I support the name change but not the machine name, until 2.0.

findlabnet commented 4 years ago

I have no feelings about the need for this change.

stpaultim commented 3 years ago

Discussed this during this meeting on October 22 - https://youtu.be/sXcWUz_Vdws?t=1485 (24:45 into video, discussion of this issue lasts about 8 minutes).

According to @quicksketch, contrib there shouldn't be any hard-coded references to any text formats in contrib modules, since there are api function to get the default or fallback text format.

According to @quicksketch, we should definitely change the machine name. Apparently, there are two failing tests and if we can get them working, this issue is ready for final review.

If anyone (@docwilmot or @jlfranklin) has any strong concerns about the machine name, please speak up now. Possible, watch the segment of the video above where we discussed that and pretty much determined that this will only effect new sites and that changing the machine name should not be a problem.

stpaultim commented 3 years ago

I'm looking back through the PR. Basically, it makes changes in 3 places.

1) The install profile. These changes will only effect new sites. 2) It makes some changes to TESTS. Are tests ever run on existing sites? If so, then these tests might fail if run on an existing site. Is this something that we should be concerned about? 3) It makes changes to these files in the core/modules/filter core/modules/filter/filter.api.php core/modules/filter/filter.theme.inc

I would think that these changes to the filter module may break some existing sites. Can anyone provide me with some additional feedback?

I would assume that the worst case is that we modify the PR to work with either the old Full HTML format or the new RAW HTML format.

stpaultim commented 3 years ago

As of right now, tests are passing. This just needs manual testing, code review, and feedback on items #2 and #3 in previous comment.

klonos commented 3 years ago

Are tests ever run on existing sites? If so, then these tests might fail if run on an existing site. Is this something that we should be concerned about?

They might be, if there is some CI that runs the full test suite as part of code deployments. I'd argue that that is not the 80% use case for Backdrop though. Test would equally have failed if the format was disabled/removed though, so 🤷

Another thought re tests is whether they are specifically testing the full/raw html format, or if that has been randomly selected when these tests were initially written. So:

quicksketch commented 3 years ago

Doing a full text search on the code in the backdrop-contrib group, the following modules would either have their tests broken or have config values that no longer work if we renamed the full_html format in the standard profile:

setup_example
eu_cookie_compliance
mimemail
demo
mandrill
services
restws
prism
examples
privatemsg
style_settings
champion
feed_import
ckeditor
contest
ip_geoloc
ghost commented 3 years ago

It sounds to me like changing the machine name of the text format can/will break things and should therefore wait until v2.x. The question in today's dev meeting then became: should the human-readable name change or not. To answer this, I defer to Phil:

Phil says...

In light of Phil's wisdom, I think users shouldn't have to wait for 2.x just because the machine name can't change yet. I therefore support changing the human-readable name, but leaving the machine name as-is.

klonos commented 3 years ago

...users shouldn't have to wait for 2.x just because the machine name can't change yet.

Totally agree 👍

philsward commented 3 years ago

Formal request to change the human-readable name to close this issue and create a new issue to change the machine name for a future (currently undetermined) release.

Effectively split this into two parts: 1) Frontend display 2) Backend machine name

stpaultim commented 3 years ago

This was discussed again during the recent dev meeting (see 23 minutes into https://www.youtube.com/watch?v=C5R3sXrkhxw).

The feeling at this meeting was that we should proceed with renaming the display name for this text format and leave the machine name as it is.

We would then open a new issue to change the machine name in 2.0 OR possibly before that. The criteria for changing the machine name before that would be that someone put the leg work in to look closer at the impact that changing the machine name would have on contrib modules. If someone were able to demonstrate that it was not serious OR submit pull requests fixing those cases where it might cause problems, then we could possibly make the change before 2.0.

stpaultim commented 3 years ago

Ready for testing and code review (again)!

I changed the machine names back to full_html. Kept all references to Display name as "Raw HTML" and edited comments to be more clear about what is happening, example:

      // Log in as an administrator and edit the comment to use Raw HTML (full_html), so
      // that the comment text itself is not filtered at all.
stpaultim commented 3 years ago

I created a follow-up issue. Would love for someone to review the follow-up issue and make sure that it makes sense and reflects our current sentiment about this issue. https://github.com/backdrop/backdrop-issues/issues/4806

philsward commented 3 years ago

Quick idea, is it worth adding to the comments for that API that "full_html will go away in favor of raw_html in the future"

That way if someone comes in to look at that API, they are aware of the impending change that "might" break their code someday.

Maybe this just needs to be added to the web-based api docs? Trying to cover bases for future compatibility.

ghost commented 3 years ago

Code looks good to me.

klonos commented 3 years ago

Code looks good to me too 👍

PS: I initially had the urge to rename the names of variables like $full, $full_html_format, but then considered that this is an issue tagged with [UX] - not with [DX] (which is what #4806 should be tagged with)

ghost commented 3 years ago

Thanks @stpaultim, @klonos, @jenlampton, @indigoxela, @Egarbeil, @quicksketch & @philsward! (hope I didn't miss anyone) I've merged https://github.com/backdrop/backdrop/pull/3365 into 1.x for 1.18.0.