Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
stpaultim opened this issue Jul 26, 2020 · 35 comments · Fixed by backdrop/backdrop#3365
Closed

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

stpaultim opened this issue Jul 26, 2020 · 35 comments · Fixed by backdrop/backdrop#3365

Comments

@stpaultim
Copy link
Member

stpaultim commented Jul 26, 2020

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

#1519 (comment)

Additional information

In this comment: #1519 (comment) @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" - #1188


Status or Next Steps:


Issue Advocate = @stpaultim

@jenlampton
Copy link
Member

jenlampton commented Jul 26, 2020 via email

@klonos klonos changed the title Change "Full HTML" text format to "Raw HTML" [UX] Change "Full HTML" text format to "Raw HTML" Jul 26, 2020
@klonos klonos changed the title [UX] Change "Full HTML" text format to "Raw HTML" [UX] Rename "Full HTML" text format to "Raw HTML" Jul 26, 2020
@klonos
Copy link
Member

klonos commented Jul 26, 2020

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
Copy link

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
Copy link
Member

jenlampton commented Jul 27, 2020

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 klonos changed the title [UX] Rename "Full HTML" text format to "Raw HTML" [UX] Change the human-readable label of the full_html text format from "Full HTML" to "Raw HTML" Jul 28, 2020
@klonos
Copy link
Member

klonos commented Jul 28, 2020

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

@klonos
Copy link
Member

klonos commented Jul 28, 2020

...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
Copy link
Member

jenlampton commented Jul 28, 2020 via email

@klonos
Copy link
Member

klonos commented Jul 28, 2020

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
Copy link

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
Copy link
Member Author

stpaultim commented Jul 28, 2020

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 jenlampton changed the title [UX] Change the human-readable label of the full_html text format from "Full HTML" to "Raw HTML" [UX] Change the name of the "Full HTML" text format to "Raw HTML" Jul 28, 2020
@jenlampton
Copy link
Member

jenlampton commented Jul 28, 2020

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:

  • biblio: appears in a default config file (unsafe, tricky to fix but possible)
  • eu_cookie_compliance: text for the banner message is saved using this format (safely)
  • services: uses it in a test (unsafe -- could be fixed easily)

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
Copy link
Member

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. 😉

@Egarbeil
Copy link

Egarbeil commented Oct 8, 2020

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
Copy link
Member Author

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 #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
Copy link

Egarbeil commented Oct 8, 2020 via email

@indigoxela
Copy link
Member

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

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
Copy link
Member

klonos commented Oct 9, 2020

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
Copy link
Member Author

stpaultim commented Nov 30, 2020

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

klonos commented Nov 30, 2020

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:

  • if the test specifically tests the full_html format, then it should specifically test the new raw_html format now.
  • if the test randomly uses the full_html format, to test generic filter/format functionality, then it shouldn't matter which format is used for the test really.

@quicksketch
Copy link
Member

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
Copy link

ghost commented Dec 4, 2020

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
Copy link
Member

klonos commented Dec 4, 2020

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

Totally agree 👍

@philsward
Copy link

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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.
#4806

@philsward
Copy link

philsward commented Dec 14, 2020

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
Copy link

ghost commented Dec 14, 2020

Code looks good to me.

@klonos
Copy link
Member

klonos commented Dec 14, 2020

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
Copy link

ghost commented Jan 1, 2021

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

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants