Bug 21131 - Changing and restoring a WYSIWYG preference can result in unexpected behaviour
Summary: Changing and restoring a WYSIWYG preference can result in unexpected behaviour
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-30 04:25 UTC by David Cook
Modified: 2019-10-14 19:58 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21131 - Changing and restoring a WYSIWYG preference can result in unexpected behaviour (2.59 KB, patch)
2018-07-30 04:49 UTC, David Cook
Details | Diff | Splinter Review
Bug 21131 - Changing and restoring a WYSIWYG preference can result in unexpected behaviour (2.64 KB, patch)
2018-08-01 17:39 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21131: Remove the TinyMCE.isDirty() check that caused unexpected behaviour (2.68 KB, patch)
2018-08-01 17:51 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2018-07-30 04:25:16 UTC
If you're using the WYSIWYG editor for system preferences, the TinyMCE editor will update the system preference text editor when the content is different to the content that the editor started with when the page was loaded.

If you change the content and then change it back to the original content, the editor will actually use the last "different" content.

Eg:

TinyMCE Editor: 123a
Textarea: 123a

TinyMce Editor: 123
Textarea: 123

TinyMce Editor: 123a
Textarea: 123

This is due to (my) overzealous use of the "TinyMCE.isDirty()" function. I'll just strip it out and everything will be fine.
Comment 1 David Cook 2018-07-30 04:49:32 UTC
Created attachment 77322 [details] [review]
Bug 21131 - Changing and restoring a WYSIWYG preference can result in unexpected behaviour

When using the WYSIWYG editor for system preferences, the editor only
updates the textarea element if the content is different from the
original content. This means if you change a system preference and
then change it back to the original content, the textarea will have
the 2nd last change you made.

This patch removes the TinyMCE.isDirty() check, which was responsible
for comparing the original and changed content. Every input/keydown/dragend
or TinyMCE command will cause the textarea element to be updated and
trigger the input event which causes the "modified" class to be added
to the element, so that the system preference can be saved.

__TEST PLAN__

_Before applying_
0. Change "UseWYSIWYGinSystemPreferences" to "Show"
1. Change "opaccredits" to "123a"
2. Click "Save all OPAC preferences"
3. Reload the page
4. Change "opaccredits" to "123"
5. Change "opaccredits" to "123a"
6. Click "Save all OPAC preferences"
7. Reload the page
8. Note that "opaccredits" says "123"

_Apply the patch_

_After applying_
1. Change "opaccredits" to "123a"
2. Change "opaccredits" to "123"
3. Click "Save all OPAC preferences"
4. Reload the page
5. Note that "opaccredits" says "123" (and not "123a")
6. Change "opaccredits" to "1234"
7. Click "Save all OPAC preferences"
8. Reload the page
9. Note that "opaccredits" says "1234"
Comment 2 Owen Leonard 2018-08-01 17:39:15 UTC
Created attachment 77389 [details] [review]
Bug 21131 - Changing and restoring a WYSIWYG preference can result in unexpected behaviour

When using the WYSIWYG editor for system preferences, the editor only
updates the textarea element if the content is different from the
original content. This means if you change a system preference and
then change it back to the original content, the textarea will have
the 2nd last change you made.

This patch removes the TinyMCE.isDirty() check, which was responsible
for comparing the original and changed content. Every input/keydown/dragend
or TinyMCE command will cause the textarea element to be updated and
trigger the input event which causes the "modified" class to be added
to the element, so that the system preference can be saved.

__TEST PLAN__

_Before applying_
0. Change "UseWYSIWYGinSystemPreferences" to "Show"
1. Change "opaccredits" to "123a"
2. Click "Save all OPAC preferences"
3. Reload the page
4. Change "opaccredits" to "123"
5. Change "opaccredits" to "123a"
6. Click "Save all OPAC preferences"
7. Reload the page
8. Note that "opaccredits" says "123"

_Apply the patch_

_After applying_
1. Change "opaccredits" to "123a"
2. Change "opaccredits" to "123"
3. Click "Save all OPAC preferences"
4. Reload the page
5. Note that "opaccredits" says "123" (and not "123a")
6. Change "opaccredits" to "1234"
7. Click "Save all OPAC preferences"
8. Reload the page
9. Note that "opaccredits" says "1234"

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Katrin Fischer 2018-08-01 17:51:18 UTC
Created attachment 77390 [details] [review]
Bug 21131: Remove the TinyMCE.isDirty() check that caused unexpected behaviour

When using the WYSIWYG editor for system preferences, the editor only
updates the textarea element if the content is different from the
original content. This means if you change a system preference and
then change it back to the original content, the textarea will have
the 2nd last change you made.

This patch removes the TinyMCE.isDirty() check, which was responsible
for comparing the original and changed content. Every input/keydown/dragend
or TinyMCE command will cause the textarea element to be updated and
trigger the input event which causes the "modified" class to be added
to the element, so that the system preference can be saved.

__TEST PLAN__

_Before applying_
0. Change "UseWYSIWYGinSystemPreferences" to "Show"
1. Change "opaccredits" to "123a"
2. Click "Save all OPAC preferences"
3. Reload the page
4. Change "opaccredits" to "123"
5. Change "opaccredits" to "123a"
6. Click "Save all OPAC preferences"
7. Reload the page
8. Note that "opaccredits" says "123"

_Apply the patch_

_After applying_
1. Change "opaccredits" to "123a"
2. Change "opaccredits" to "123"
3. Click "Save all OPAC preferences"
4. Reload the page
5. Note that "opaccredits" says "123" (and not "123a")
6. Change "opaccredits" to "1234"
7. Click "Save all OPAC preferences"
8. Reload the page
9. Note that "opaccredits" says "1234"

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 4 Katrin Fischer 2018-08-01 17:52:31 UTC
Hi David, we standardized the commit messages a bit more:
https://wiki.koha-community.org/wiki/Commit_messages
- Start with Bug XXXX:
- Describe the fix in the subject, not the problem :)
Comment 5 David Cook 2018-08-02 00:19:57 UTC
(In reply to Katrin Fischer from comment #4)
> Hi David, we standardized the commit messages a bit more:
> https://wiki.koha-community.org/wiki/Commit_messages
> - Start with Bug XXXX:
> - Describe the fix in the subject, not the problem :)

Thanks, Katrin. I actually wrote to koha-devel asking about https://wiki.koha-community.org/wiki/Commit_messages#Examples, when I was writing up this bug report, because I didn't understand the examples. 

But I realize now that the page actually starts with https://wiki.koha-community.org/wiki/Commit_messages#Good_commit_messages. I had no idea!

But according to https://wiki.koha-community.org/wiki/Commit_messages#Subject_line, the prototypical example seems to describe the problem and not the fix :S.
Comment 6 David Cook 2018-08-02 00:21:00 UTC
Btw, I am super duper happy to follow a standard, so long as I actually know what the standard is...

I see in https://wiki.koha-community.org/wiki/Commit_messages#Good_commit_messages_2 that the subject line describes the fix and not the problem though.
Comment 7 Nick Clemens 2018-08-09 12:28:02 UTC
Awesome work all!

Pushed to master for 18.11
Comment 8 Martin Renvoize 2018-08-10 07:46:46 UTC
Pushed to 18.05.x for 18.05.03
Comment 9 Fridolin Somers 2018-09-21 12:06:03 UTC
Pushed to 17.11.x for 17.11.10