Bug 26942

Summary: TinyMCE in the News Tool is still doing some types of automatic code cleanup
Product: Koha Reporter: Lucas Gass <lucas>
Component: ToolsAssignee: Lucas Gass <lucas>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: amitddng135, andrew, dcook, fridolin.somers, kyle, sudrland, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.05,20.05.11
Attachments: Bug 26942: Allow style and link tags in NewsEditor TinyMCE
Bug 26942: Allow style and link tags in NewsEditor TinyMCE
Bug 26942: Allow style and link tags in NewsEditor TinyMCE

Description Lucas Gass 2020-11-05 18:55:50 UTC
To recreate:

-Make sure the sys pref 'NewsToolEditor' is set to TinyMCE
-Go to any news item and add this line:  <link href="https://fonts.googleapis.com/css?family=Open+Sans” rel=“stylesheet">
-Save the news item
-Re-open the same news item again in TinyMCE.
-The editor has removed that line. Annoying!
Comment 1 Lucas Gass 2020-11-05 19:24:11 UTC
TinyMCE doesn't automatically takes out HTML tags like <link>, <style>, and <meta> because those are not valid HTML tags inside the HTML body. They belong in the HTML <head>.

Perhaps we need a new system preference that would allow librarians to insert things into the HTML <head>. We should be able to easily add this to doc-head-open.inc
Comment 2 David Cook 2020-11-05 23:09:14 UTC
(In reply to Lucas Gass from comment #1)
> TinyMCE doesn't automatically takes out HTML tags like <link>, <style>, and
> <meta> because those are not valid HTML tags inside the HTML body. They
> belong in the HTML <head>.
> 

That should say "TinyMCE automatically takes out HTML tags like...", right?

That is annoying...
Comment 3 Lucas Gass 2020-11-06 00:40:34 UTC
(In reply to David Cook from comment #2)
> (In reply to Lucas Gass from comment #1)
> > TinyMCE doesn't automatically takes out HTML tags like <link>, <style>, and
> > <meta> because those are not valid HTML tags inside the HTML body. They
> > belong in the HTML <head>.
> > 
> 
> That should say "TinyMCE automatically takes out HTML tags like...", right?
> 
> That is annoying...

David, correct. I meant it DOES automatically take out HTML tags like...


Thanks for clarifying.
Comment 4 David Cook 2020-11-06 01:43:10 UTC
(In reply to Lucas Gass from comment #3)
> David, correct. I meant it DOES automatically take out HTML tags like...
> 
> 
> Thanks for clarifying.

Just to clarify again. I was saying that the TinyMCE behaviour was annoying and not the typo heh. 

If you do a patch for this one, I'd be happy to test it. (Probably not something I'll look at writing my own fix for any time soon thought.)
Comment 5 Lucas Gass 2021-01-22 22:53:49 UTC
Created attachment 115715 [details] [review]
Bug 26942: Allow style and link tags in NewsEditor TinyMCE

To Test:
-Make sure the sys pref 'NewsToolEditor' is set to TinyMCE
-Go to any news item and add this line:  <link href="https://fonts.googleapis.com/css?family=Open+Sans” rel=“stylesheet">
-Save the news item
-Re-open the same news item again in TinyMCE.
-The editor has removed that line. Annoying!
-Apply patch
-Try again, the <link> should stay now
Comment 6 David Cook 2021-03-26 04:52:16 UTC
Just used this in a hot fix. So handy. I'll run up a KTD and sign this off pronto...
Comment 7 Michael Sutherland 2021-03-26 12:34:52 UTC
This is plainly awful. As a web developer, I do not want some WYSIWYG editor telling me I do not know how to write HTML and start taking out HTML I put in for a reason, such as a <div> or <script> and then have everything break because the system does not like my HTML. It's not incorrect to have certain tags in the body and we've done it that way precisely because we do not have access to add code in the <head>. I understand some of us are a little more savvy and technically able to write HTML than others and this is attempting to make something work for everyone, because clearly, not everyone knows how to write HTML. However, this development is not at all 'handy'. And, I do not particularly like that everything was moved out of system preferences into the news section.
Comment 8 Michael Sutherland 2021-03-26 13:42:10 UTC
If this is meant to fix those issues with the WYSIWYG editing code then it should be a higher priority. What I intended to say is that the WYSIWYG making coding changes is not helpful, especially when it does not have a way to log and recover lost code due to those changes.
Comment 9 Kyle M Hall 2021-03-26 14:59:42 UTC
It looks like we could optionally disable validation in TinyMCE: https://stackoverflow.com/questions/7058623/tinymce-completely-disable-validation
Comment 10 Lucas Gass 2021-03-26 15:15:55 UTC
(In reply to Kyle M Hall from comment #9)
> It looks like we could optionally disable validation in TinyMCE:
> https://stackoverflow.com/questions/7058623/tinymce-completely-disable-
> validation

verify_html: false is already set when initializing TinyMCE. Maybe we should add these other lines to the config:

 
    cleanup_on_startup: false,
    trim_span_elements: false,
    cleanup: false,
    convert_urls: false
Comment 11 Amit Gupta 2021-03-27 08:20:00 UTC
Created attachment 118885 [details] [review]
Bug 26942: Allow style and link tags in NewsEditor TinyMCE

To Test:
-Make sure the sys pref 'NewsToolEditor' is set to TinyMCE
-Go to any news item and add this line:  <link href="https://fonts.googleapis.com/css?family=Open+Sans” rel=“stylesheet">
-Save the news item
-Re-open the same news item again in TinyMCE.
-The editor has removed that line. Annoying!
-Apply patch
-Try again, the <link> should stay now
Comment 12 Katrin Fischer 2021-03-27 15:16:43 UTC
(In reply to Lucas Gass from comment #10)
> (In reply to Kyle M Hall from comment #9)
> > It looks like we could optionally disable validation in TinyMCE:
> > https://stackoverflow.com/questions/7058623/tinymce-completely-disable-
> > validation
> 
> verify_html: false is already set when initializing TinyMCE. Maybe we should
> add these other lines to the config:
> 
>  
>     cleanup_on_startup: false,
>     trim_span_elements: false,
>     cleanup: false,
>     convert_urls: false

I'd like to see this investigated. Now that both editors can be used alongside, it's very bad when one changes what was written with the other. At least this is unexpected behaviour.
Comment 13 Katrin Fischer 2021-03-27 15:28:46 UTC
Created attachment 118902 [details] [review]
Bug 26942: Allow style and link tags in NewsEditor TinyMCE

To Test:
-Make sure the sys pref 'NewsToolEditor' is set to TinyMCE
-Go to any news item and add this line:  <link href="https://fonts.googleapis.com/css?family=Open+Sans" rel="stylesheet">
-Save the news item
-Re-open the same news item again in TinyMCE.
-The editor has removed that line. Annoying!
-Apply patch
-Try again, the <link> should stay now

Signed-off-by: Amit Gupta <amitddng135@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 David Cook 2021-03-29 02:34:34 UTC
Apologies for not doing more last week. I got caught up with other things, but glad to see it sparked more action!
Comment 15 Jonathan Druart 2021-04-01 16:53:48 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 16 Lucas Gass 2021-04-02 18:28:24 UTC
(In reply to Michael Sutherland from comment #7)
> This is plainly awful. As a web developer, I do not want some WYSIWYG editor
> telling me I do not know how to write HTML and start taking out HTML I put
> in for a reason, such as a <div> or <script> and then have everything break
> because the system does not like my HTML. It's not incorrect to have certain
> tags in the body and we've done it that way precisely because we do not have
> access to add code in the <head>. I understand some of us are a little more
> savvy and technically able to write HTML than others and this is attempting
> to make something work for everyone, because clearly, not everyone knows how
> to write HTML. However, this development is not at all 'handy'. And, I do
> not particularly like that everything was moved out of system preferences
> into the news section.

I think Michael brings up a good point here. There is no way to insert scripts, links to Google fonts, meta tags into the HTML head and many bypassed this by adding those things to the OPACHeader. 

I have filed bug 28080
Comment 17 Fridolin Somers 2021-04-07 13:19:28 UTC
Pushed to 20.11.x for 20.11.05
Comment 18 Andrew Fuerste-Henry 2021-04-24 12:51:16 UTC
I'm still loosing my link on 20.05 with this patch. Can you please take a look, Lucas?
Comment 19 Andrew Fuerste-Henry 2021-04-24 13:39:14 UTC
Talked to Lucas, I'd misunderstood the test plan. Pushed to 20.05.x for 20.05.11
Comment 20 Victor Grousset/tuxayo 2021-04-24 16:47:51 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.