Bug 14389 - Editing a syspref in a textarea does not enable the Save button
Summary: Editing a syspref in a textarea does not enable the Save button
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jesse Weaver
QA Contact: Marcel de Rooy
URL:
Keywords:
: 10674 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-06-15 18:58 UTC by Magnus Enger
Modified: 2016-12-05 21:23 UTC (History)
12 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 14389 - Editing a syspref in a textarea does not enable the Save button (1.51 KB, patch)
2015-06-15 21:51 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 14389 - Editing a syspref in a textarea does not enable the Save button (1.80 KB, patch)
2015-06-16 13:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14389 - Editing a syspref in a textarea does not enable the Save button (1.49 KB, patch)
2015-06-18 23:54 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14389 - Editing a syspref in a textarea does not enable the Save button (1.64 KB, patch)
2015-06-18 23:59 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 14389 - Editing a syspref in a textarea does not enable the Save button (1.71 KB, patch)
2015-06-19 10:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14389 - Editing a syspref in a textarea does not enable the Save button (1.81 KB, patch)
2015-07-03 07:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2015-06-15 18:58:29 UTC
At least sometimes, editing a syspref in a textarea (like e.g. opaccredits) will not make the "Save" button active. I have observed this on 3.18.7. Others have possibly sighted it on master too. 

Workaround: Add and remove one char from a text input to activate the button.
Comment 1 Magnus Enger 2015-06-15 19:00:06 UTC
Jesse Weaver asked that this be assigned to him.
Comment 2 Jesse Weaver 2015-06-15 21:51:58 UTC Comment hidden (obsolete)
Comment 3 Liz Rea 2015-06-16 03:41:04 UTC
Hi Jesse,

I tested this, and the following action:

highlighting everything, and deleting the contents of a syspref (this case, nologininstructions)

the button didn't activate when I might have expected it to. I can get around it by adding and deleting a space in the syspref.

Also, we're still stuck with the "middle click paste" from linux not triggering the save button, but that might be an even more distant edge case.

Is this something we can fix? This patch is certainly an improvement over the current behaviour.

Liz
Comment 4 Liz Rea 2015-06-16 04:01:49 UTC
Hi Jesse, 

I just tried this with only 'input', and it seems to work in every way I'd expect it to, including the linux middle button paste - what do you think?

Liz
Comment 5 Liz Rea 2015-06-16 04:15:12 UTC
actually forget I said anything :)
Comment 6 Liz Rea 2015-06-16 04:17:07 UTC
oy. it's getting too late for me, comment 4 stands, actually.
Comment 7 Jonathan Druart 2015-06-16 13:37:51 UTC Comment hidden (obsolete)
Comment 8 Liz Rea 2015-06-18 23:54:44 UTC Comment hidden (obsolete)
Comment 9 Jesse Weaver 2015-06-18 23:59:47 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-06-19 10:39:34 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2015-06-20 22:21:34 UTC
Hi Jonathan, should this be Passed QA?
Comment 12 David Cook 2015-06-22 03:52:19 UTC
I was thinking about this bug recently. 

I suppose it still won't work for WYSIWYG editors in some cases, but that's not a community concern (yet). I'll have to do some thinking on that one when I have time...
Comment 13 David Cook 2015-06-22 03:58:06 UTC
(In reply to David Cook from comment #12)
> I was thinking about this bug recently. 
> 
> I suppose it still won't work for WYSIWYG editors in some cases, but that's
> not a community concern (yet). I'll have to do some thinking on that one
> when I have time...

Oh wait. Liz pointed out that it's on "input" and not on "change".

It might work for WYSIWYG editors as well! I'll look into this!
Comment 14 Jonathan Druart 2015-06-22 11:09:55 UTC
(In reply to Katrin Fischer from comment #11)
> Hi Jonathan, should this be Passed QA?

No, I signed it off.
Comment 15 David Cook 2015-06-23 04:09:20 UTC
(In reply to David Cook from comment #13)
> (In reply to David Cook from comment #12)
> > I was thinking about this bug recently. 
> > 
> > I suppose it still won't work for WYSIWYG editors in some cases, but that's
> > not a community concern (yet). I'll have to do some thinking on that one
> > when I have time...
> 
> Oh wait. Liz pointed out that it's on "input" and not on "change".
> 
> It might work for WYSIWYG editors as well! I'll look into this!

(In reply to David Cook from comment #13)
> (In reply to David Cook from comment #12)
> > I was thinking about this bug recently. 
> > 
> > I suppose it still won't work for WYSIWYG editors in some cases, but that's
> > not a community concern (yet). I'll have to do some thinking on that one
> > when I have time...
> 
> Oh wait. Liz pointed out that it's on "input" and not on "change".
> 
> It might work for WYSIWYG editors as well! I'll look into this!

So I've been doing lots of research (as I do). 

Locally, I've added TinyMCE to some system preferences which use HTML. (I plan to upstream it when I find some time.)

I've just been using "keyup" to detect changes to the TinyMCE editor, and then I trigger a "keyup" on the original textarea so that the "Save..." button activates.

It works OK for the most part, but I've noticed a few issues like changes from the editor buttons not being detected or pastes not made through Ctrl+V or Ctrl+Ins.

Anyway... using "input" for an event listener has been absolutely great with Firefox and Chrome. It fires for so many different events. It's so awesome!

Unfortunately, IE doesn't support the "input" event for elements that are "contenteditable". TinyMCE uses a "contenteditable" body element within an iframe element to create its editor, so IE and TinyMCE are just not going to be friends with the "input" event. 

But it looks like the "input" event works well with IE otherwise, so yay! 

Just adding this info here in case its somehow relevant to people in the future (plus it's also just painfully interesting to know).
Comment 16 David Cook 2015-06-23 04:12:04 UTC
Sorry to spam people, but here are some relevant links for what I was talking about:

https://developer.mozilla.org/en-US/docs/Web/Events/input
https://connect.microsoft.com/IE/feedbackdetail/view/794285

Not actually relevant to this bug report though... just making that clear.

This patch looks super good :)
Comment 17 Marcel de Rooy 2015-07-03 07:41:59 UTC
Created attachment 40772 [details] [review]
Bug 14389 - Editing a syspref in a textarea does not enable the Save button

Test plan:
  1. Navigate to the "opaccredits" syspref (or any other textarea, i.e.,
     "Click to Edit", syspref) in the system preferences editor.
  2. Change its contents, by either pasting or typing. The field may not
     be marked as modified, even after you click outside the box.
  3. Apply the patch.
  4. Reload the page and try again; either pasting or typing should mark
     the field as changed and allow you to save.

Signed-off-by: Jesse Weaver <pianohacker@gmail.com>
Confirmed working for normal input, paste and middle-click paste in
Chrome and Firefox in Linux.

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Tomás Cohen Arazi 2015-07-07 18:28:14 UTC
Patch pushed to master.

Thanks Liz!
Comment 19 Chris Cormack 2015-07-13 22:48:11 UTC
Pushed to 3.20.x will be in 3.20.2
Comment 20 Liz Rea 2015-07-20 22:13:56 UTC
Pushed to 3.18.x will be in 3.18.09
Comment 21 Mason James 2015-07-25 13:02:41 UTC
Pushed to 3.16.x, will be in 3.16.13
Comment 22 Jesse Weaver 2016-02-18 20:48:51 UTC
*** Bug 10674 has been marked as a duplicate of this bug. ***