Opened 6 years ago

Closed 6 years ago

#4705 closed enhancement (fixed)

[with patch and spkg, positive review] Make in-line wysiwyg editor for text cells using TinyMCE

Reported by: jason Owned by: jason
Priority: major Milestone: sage-3.3
Component: notebook Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

This ticket splits off part of #4267.

It depends on #4704

Attachments (3)

tinymce-editable.patch (18.6 KB) - added by jason 6 years ago.
safari-fix.patch (2.9 KB) - added by jason 6 years ago.
apply on top of previous patch
ghost-text.patch (657 bytes) - added by jason 6 years ago.

Download all attachments as: .zip

Change History (40)

Changed 6 years ago by jason

comment:2 Changed 6 years ago by jason

  • Summary changed from Make in-line wysiwyg editor for text cells using TinyMCE to [with patch and spkg] Make in-line wysiwyg editor for text cells using TinyMCE

comment:3 Changed 6 years ago by jason

  • Owner changed from boothby to jason
  • Status changed from new to assigned

comment:4 Changed 6 years ago by jason

  • Summary changed from [with patch and spkg] Make in-line wysiwyg editor for text cells using TinyMCE to [with patch and spkg, needs review] Make in-line wysiwyg editor for text cells using TinyMCE

comment:5 Changed 6 years ago by TimothyClemans

  • Summary changed from [with patch and spkg, needs review] Make in-line wysiwyg editor for text cells using TinyMCE to [with patch and spkg, positive review] Make in-line wysiwyg editor for text cells using TinyMCE
  • Type changed from defect to enhancement

Works for me Apply #4674, #3767, #4704, #4705

comment:6 Changed 6 years ago by mabshoff

This needs to be tested with a wide variety of browsers and OS combinations before this goes in. So please list what browser/OS combo worked for you.

It will likely have to wait for 3.4 anyway.

Cheers,

Michael

comment:7 Changed 6 years ago by TimothyClemans

  • Summary changed from [with patch and spkg, positive review] Make in-line wysiwyg editor for text cells using TinyMCE to [with patch and spkg, needs work] Make in-line wysiwyg editor for text cells using TinyMCE

SHIFT-CLICK on new cell bar doesn't work in Safari on Mac.

comment:8 Changed 6 years ago by jason

This is likely due to bugs in the extendedclicks jquery plugin. The author is willing to work on this, we just need to isolate the problem.

comment:9 Changed 6 years ago by jason

The bugs in Safari are not from the extendedclicks jquery plugin. Timothy let me have access to his mac the other day and we determined that it has to do with Safari not evaluating javascript in any html that is inserted into the DOM on-the-fly. Basically, in this patch, the new cell bar code has been changed from a mousedown event in the html code to a separate javascript function. That javascript function is not evaluated by default in Safari (but is in FF). So we need to add the html to the DOM, and then explicitly evaluate the javascript. There is code to something similar to this, and I'm working on modifications to the patch.

Changed 6 years ago by jason

apply on top of previous patch

comment:11 Changed 6 years ago by jason

  • Summary changed from [with patch and spkg, needs work] Make in-line wysiwyg editor for text cells using TinyMCE to [with patch and spkg, needs review] Make in-line wysiwyg editor for text cells using TinyMCE

safari-fix seems to take care of all of the Safari issues.

Okay, everyone, hit this with everything that you have. I've also updated the tinyMCE spkg (available from the above URL) to make the default font style inherit from the notebook.

comment:12 Changed 6 years ago by TimothyClemans

Works with FF 3.0.5 and Safari on Mac OS X.

comment:13 Changed 6 years ago by jason

To test this on supported browsers, we should test it on Firefox, Opera, and Safari. I've tested it on FF 3.0.5 on Ubuntu 8.10 and the tinyMCE spkg and this patch seem to work okay.

comment:14 follow-up: Changed 6 years ago by jason

Here are complete instructions to apply and test the tinymce patch:

Apply the following patches from 4674, 3767, 4704, and 4705, in order:

jsmath-spkg.patch
jquery-and-friends-spkg.2.patch
jquery-javascript-cleanup.patch
tinymce-editable.patch
safari-fix.patch

Install the following packages from the above tickets. You may have to do ./sage -f <spkg URL> to force an installation if you've already installed an spkg.

http://sage.math.washington.edu/home/jason/notebook/jquery-1.2.6.spkg
http://sage.math.washington.edu/home/jason/notebook/jqueryui-1.6r807svn.spkg
http://sage.math.washington.edu/home/jason/notebook/jsmath-3.6a.spkg
http://sage.math.washington.edu/home/jason/notebook/tinyMCE-3.2.0.2.spkg

Optionally, you can also install the jsmath image fonts by installing:
http://sage.math.washington.edu/home/jason/notebook/jsmath-image-fonts-1.3p0.spkg

Now you should be able to shift-click on a new cell bar to create a text cell using tinyMCE. You should also be able to double-click on an existing text cell to edit it in-place.

comment:15 Changed 6 years ago by mhampton

  • Summary changed from [with patch and spkg, needs review] Make in-line wysiwyg editor for text cells using TinyMCE to [with patch and spkg, one positive review, needs review] Make in-line wysiwyg editor for text cells using TinyMCE

I tested this on Safari and Firefox on OS X (10.5). Everything looks good to me so far. I will keep using this and see if I run into any problems.

-Marshall

comment:16 Changed 6 years ago by mabshoff

The spkg looks good to me. A slightly cleaned up version is at

http://sage.math.washington.edu/home/mabshoff/spkgs/tinyMCE-3.2.0.2.p0.spkg

Cheers,

Michael

comment:17 in reply to: ↑ 14 ; follow-up: Changed 6 years ago by ddrake

I followed the instructions in comment 14 and there's at least one small problem. If I shift-click on the new cell bar, I get the TinyMCE editor thingy. If I click "cancel" without entering any text, the tooltip text ("Doubleclick to edit...") gets put into a text cell at that location.

If I choose "Edit" at the top, the text "Doubleclick to edit..." doesn't show up in the notebook, and if I click "Save changes", the extraneous text goes away.

This happens in Firefox, Opera, IE7, and Chrome. I applied all the patches and spkgs to 3.2.3.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 6 years ago by jason

Replying to ddrake:

I followed the instructions in comment 14 and there's at least one small problem. If I shift-click on the new cell bar, I get the TinyMCE editor thingy. If I click "cancel" without entering any text, the tooltip text ("Doubleclick to edit...") gets put into a text cell at that location.

If I choose "Edit" at the top, the text "Doubleclick to edit..." doesn't show up in the notebook, and if I click "Save changes", the extraneous text goes away.

This happens in Firefox, Opera, IE7, and Chrome. I applied all the patches and spkgs to 3.2.3.

Yes, that sounds like the intended behavior. Whether or not that should be what happens may be up for debate, though. The idea is that if you create a text cell, but put nothing in it, it is impossible to edit by double-clicking. If you make any edits to the cell, though, the filler text goes away.

comment:19 in reply to: ↑ 18 Changed 6 years ago by ddrake

Replying to jason:

Replying to ddrake:

I followed the instructions in comment 14 and there's at least one small problem. If I shift-click on the new cell bar, I get the TinyMCE editor thingy. If I click "cancel" without entering any text, the tooltip text ("Doubleclick to edit...") gets put into a text cell at that location.

If I choose "Edit" at the top, the text "Doubleclick to edit..." doesn't show up in the notebook, and if I click "Save changes", the extraneous text goes away.

This happens in Firefox, Opera, IE7, and Chrome. I applied all the patches and spkgs to 3.2.3.

Yes, that sounds like the intended behavior. Whether or not that should be what happens may be up for debate, though. The idea is that if you create a text cell, but put nothing in it, it is impossible to edit by double-clicking. If you make any edits to the cell, though, the filler text goes away.

Hrm, it may be intended, but I don't like it at all. When I clicked "cancel", I wanted the entire business -- text cell and all -- to go away and, well, be cancelled. I had done one thing (shift-click) and when I clicked cancel, I wanted that one thing to be undone. I do see your point though, that shift-clicking (1) creates a text cell, and (2) starts the tinymce editor on it -- and the cancel button cancels (2).

I'm also bothered by the text being "ghostly", in that you see it when viewing the notebook normally, but it doesn't show up in the usual text-based "Edit".

comment:20 Changed 6 years ago by ddrake

I have tested this and it appears to work fine with the following browser/OS combinations:

Ubuntu 8.10:

  • Firefox 3.0

Win XP SP3:

  • Firefox 3.1 beta
  • IE 7
  • Opera (whatever the current version is)
  • Chrome

OS X 10.5:

  • Firefox 3.0
  • Safari

Just to screw around, I tried iCab, which edited existing fields just fine, but the shift-click on the new cell bar didn't work. I think we can safely ignore the iCab browser market. :)

comment:21 Changed 6 years ago by was

I'm also bothered by the text being "ghostly", in that you see it when
viewing the notebook normally, but it doesn't show up in the usual text-based "Edit".

I do not like either. Actually, I'm very puzzled how it is possible that it wouldn't show up in Edit, since when one quits a notebook server and restarts it later, worksheets are loaded from disk entirely using their plane text representation (i.e., what you see in Edit), so it seems to me that if the text created is indeed "ghostly" as you claim, it will surely vanish when the server is restarted.

comment:22 Changed 6 years ago by mhampton

Maybe there is some confusion here - I think the only "ghostly" stuff is the "Doubleclick to edit..." message. It doesn't appear in a view-page-source window. It does block cell mergers. It goes away after switching to Edit and saving, but not switching to Edit and canceling.

I don't think this annoyance is enough to block the inclusion of this functionality - I think we should put it in and make a follow-up ticket to try to change this. It should be possible to easily delete at least.

comment:23 Changed 6 years ago by mhampton

In trying to track down some of the behavior discussed above, I noticed that normal text cells (i.e. non-ghostly ones, with some content) do not have unique ids. I am not sure if that is relevant to the ghost issue but it does seem like a bad thing and may induce other bugs. I.e. normal executable cells have and outer id such as "cell_outer_1" and an inner id such as "cell_1". The text cells have nested div elements with identical ids, for example both being "cell_text_1".

comment:24 Changed 6 years ago by jason

The ghostly text that is referred to is a TinyMCE setting. Check the configuration for TinyMCE, right after the TinyMCE is included as a javascript file. Somewhere in those configuration settings, there is the text "Double click to edit" (or whatever the text is).

comment:25 Changed 6 years ago by jason

Here are the things that still need to be done to get this in.

  • testing with IE 6 on Windows
  • testing with FF 2.0.x on Windows, OSX and Linux
  • testing with Safari on 10.4
  • testing with Opera 9.5 on OSX, Linux
  • review of the patches themselves
  • review of mabshoff's changes to the spkgs
  • trivial one-line patch to fix the "ghostly text" issue (just make the placeholder string the empty string in cell.py (search for "placeholder" in cell.py).
  • (maybe can wait for another patch): figure out what is going on with divs with the same id. My guess is that it is an issue with setting the innerHTMl of an object, rather than replacing the object itself. When the page is "Edit"ed and then reloaded, the duplicate nested IDs go away. This points to the javascript code that inserts text cells as the problem.

From mabshoff:

In the end having your assurance that you will available to fix some
of the inevitable issues would also be assuring.

You have my assurance.

comment:26 Changed 6 years ago by mhampton

It seems a little too demanding to ask that this works on IE 6 on Windows. Other things don't work on that platform either, or are a little strange. Is that really officially supported? If so there should be a lot of tickets for it.

On Safari on OS 10.4, I had some oddness inserting a text cell at the very top of a worksheet - nothing seemed to happen but when I re-opened it things were OK, the text cell was there. That should be a followup ticket if anyone can reproduce it, but not enough to block this ticket getting merged in my opinion.

Things seem to work fine on firefox 2.0.0.13 on my mac and windows machines.

The ghost text problem is gone, but there should be an easy way to delete an empty cell. Currently if you open the text editor and delete all text, there is still some residual empty text cell that you can't see but which blocks cell mergers. This can be deleted in "Edit" mode for the worksheet but a followup ticket should be made to make it easier. Again, I don't think that should block this ticket.

I used mabshoff's version of the tinymce spkg, seems fine. I have not inspected it in detail.

comment:27 Changed 6 years ago by kcrisman

Experience on Sage 3.2.1 with all patches and spkgs installed, using PPC OSX.4.

General: Works great, including tables and lists. Recommend adding the font-size drop-down menu, perhaps also blockquote, to the standard TinyMCE installation for Sage, since presumably these will often be projected in class and finer control of size outside <h1> style tags would be useful. Only general problems are known issue that Sage text cells are simply area between math cells, and I had some trouble getting added images to actually show up (but that could be me not knowing how to do them right). So overall positive, and I would say positive review for Safari 3.2.1. BUT...

On Firefox 2.0.0.13, encountered *terrible* bug of some kind, was reproducible on my machine. When a larger text cell was opened using TinyMCE the rendering engine somehow did not do the scrolling properly, and multiple (six, seven, more) images of the same text showed up below or above the text cell; in addition, I could no longer find the editing panel in this situation and only by luck with scrolling found the save/cancel area to even exit this surreal landscape. This seems to be a major problem, but I have no idea what the issue is, and am puzzled that Marshall did not encounter it. If this is reproducible, it should probably be a blocker.

comment:28 Changed 6 years ago by mhampton

I could not reproduce the larger text cell problem. Could you post an sws file somewhere that shows that behavior? How big are you talking about? I've pasted in a couple of pages worth of text of different types and didn't have a problem.

comment:29 follow-up: Changed 6 years ago by ddrake

I tested Firefox 2.0.0.20 on Ubuntu Hardy and it appears to work fine. I couldn't reproduce the large text cell problem reported by kcrisman.

Note that Firefox 2.0 has apparently been EOL'ed: http://www.mozilla.com/en-US/firefox/all-older.html. How long should we continue testing on that browser? (I'm not being sarcastic, I'm genuinely curious.)

comment:30 in reply to: ↑ 29 Changed 6 years ago by kcrisman

Replying to ddrake:

I tested Firefox 2.0.0.20 on Ubuntu Hardy and it appears to work fine. I couldn't reproduce the large text cell problem reported by kcrisman.

Note that Firefox 2.0 has apparently been EOL'ed: http://www.mozilla.com/en-US/firefox/all-older.html. How long should we continue testing on that browser? (I'm not being sarcastic, I'm genuinely curious.)

I updated to 2.0.0.20 and the problem persists. However, I downloaded FF3.0.5 and the problem is not there, so it's definitely FF2-specific. My guess is that a lot of people using FF only on occasion (which might be quite a few PC and Mac people, as opposed to Linux) may not even be aware that FF3 is out there (it was very dimly on my radar), so it would be worth at least some testing.

Here is a worksheet - click on the "Here is a problem" area and then scroll up and down, assuming that area appears toward the bottom of your initial browser window.

I am not sure what the problem is - presumably something Aqua+OSX.4+PPC+FF2 related. It would be nice for people to check out whether this problem shows up in any other less heavily used browsers - Konqueror, for example, if that's still in use. If this is the only place it happens, perhaps it's not the worst thing ever - though it IS really hard to use in this one case.

comment:31 follow-up: Changed 6 years ago by mhampton

I guess I can reproduce this - if I edit your "Here is a problem area" text cell, and then try to scroll the entire worksheet, the lower cells get messed up - sort of like tracers. This is not good, but doesn't seem awful to me - if you are editing a text cell, you usually don't need to scroll around the whole worksheet?

Like some other things mentioned in this ticket, I think this deserves a followup ticket but shouldn't block the inclusion of this. I think the only reason to block this patch is if it messes up some existing functionality or doctests.

comment:32 in reply to: ↑ 31 Changed 6 years ago by mabshoff

  • Milestone changed from sage-3.4.1 to sage-3.3

Replying to mhampton:

Like some other things mentioned in this ticket, I think this deserves a followup ticket but shouldn't block the inclusion of this. I think the only reason to block this patch is if it messes up some existing functionality or doctests.

Yes, if the patch here in question does not mess up existing functionality, but has issues with the new functionality with some browsers this can be dealt with via a followup ticket. Getting this in working 99% now is much better than it bitrotting and I think Jason will be much happier, too :)

So someone give this a final positive review and it is in.

Cheers,

Michael

comment:33 follow-up: Changed 6 years ago by mhampton

  • Summary changed from [with patch and spkg, one positive review, needs review] Make in-line wysiwyg editor for text cells using TinyMCE to [with patch and spkg, positive review] Make in-line wysiwyg editor for text cells using TinyMCE

I think now that people have hit this on a variety of platform combinations it should be merged. Whatever alpha or release candidate it comes out on, I think it should be highlighted as something people should pound on, and we can make the necessary followup tickets. But to move this forward it should go in.

comment:34 in reply to: ↑ 33 Changed 6 years ago by kcrisman

Replying to mhampton:

I think now that people have hit this on a variety of platform combinations it should be merged. Whatever alpha or release candidate it comes out on, I think it should be highlighted as something people should pound on, and we can make the necessary followup tickets. But to move this forward it should go in.

I wasn't sure how picky to be. Yes, and mabshoff's comment about Jason not wanting bitrot is certainly reasonable.

Does this mean that

  • testing with IE 6 on Windows
  • testing with Opera 9.5 on OSX, Linux
  • review of the patches themselves
  • review of mabshoff's changes to the spkgs

is all done? I only looked at functionality, not the code.

Let the followup tickets begin!

comment:35 Changed 6 years ago by jason

Most of my entire next week is devoted to Sage at SD12. FYI, I can certainly make followup tickets to this a priority in the next week.

I most certainly second the comment that Jason will be very happy to see this finally go in!

Changed 6 years ago by jason

comment:36 Changed 6 years ago by jason

The ghost-text.patch corrects the ghostly text mentioned above and should be applied on top of the other patches.

comment:37 Changed 6 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged tinymce-editable.patch, safari-fix.patch, ghost-text.patch in Sage 3.3.alpha0.

Note: See TracTickets for help on using tickets.