Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#6459 closed defect (fixed)

[with patch, positive review] make it so control-shift-enter sends a blank line to tinymce

Reported by: was Owned by: boothby
Priority: minor Milestone: sage-4.2
Component: notebook Keywords:
Cc: jason Merged in:
Authors: Mitesh Patel Reviewers: Karl-Dieter Crisman, Jason Grout, William Stein
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

From Pat LeSmithe:

Control-enter is bound to spliteval_cell.  To make control-shift-enter, say, insert a line break, try augmenting notebook.py's tinyMCE.init()'s setup with some code ripped from the Safari plug-in:

     // Around line 1840 of sage/server/notebook/notebook.py
     setup : function(ed) {
         ed.onKeyDown.add(function(ed, e) {
             // Make ctrl-shift-enter insert a line break.  Copied from
the Safari plug-in.
             if (e.keyCode == 13 && e.shiftKey && e.ctrlKey) {
                 // Workaround for missing shift+enter support,
http://bugs.webkit.org/show_bug.cgi?id=16973
                 var dom = ed.dom, s = ed.selection, r = s.getRng(), br;

                 // Insert BR element
                 r.insertNode(br = dom.create('br'));

                 // Place caret after BR
                 r.setStartAfter(br);
                 r.setEndAfter(br);
                 s.setRng(r);

                 // Could not place caret after BR then insert an nbsp
entity and move the caret
                 if (s.getSel().focusNode == br.previousSibling) {

s.select(dom.insertAfter(dom.doc.createTextNode('\u00a0'), br));
                     s.collapse(1);
                 }

                 // Scroll to new position, scrollIntoView can't be
used due to bug: http://bugs.webkit.org/show_bug.cgi?id=16117
                 ed.getWin().scrollTo(0,
dom.getPos(s.getRng().startContainer).y);

                 tinymce.dom.Event.cancel(e);
             }
         });
         // Make shift-enter quit editing.  This is the "old" code.
         ed.onKeyDown.add(function(ed, e) {
             if (key_enter_shift(key_event(e))) {
                 $(ed.formElement).submit();
             }
         })
     }

This seems to work on Linux in Firefox, Opera, and the Qt 4.5 WebKit
demo browser (e.g., /usr/lib64/qt4/demos/browser/browser).

Attachments (3)

trac_6459_tinymce_line_break.patch (4.0 KB) - added by mpatel 12 years ago.
trac_6459_tinymce_line_break_v2.patch (7.4 KB) - added by mpatel 12 years ago.
Tweaked and rebased against #6568. Apply only this patch.
trac_6459_tinymce_line_break_v3.patch (7.8 KB) - added by mpatel 11 years ago.
Initialization code moved to tinymce.js. Rebased for #7196. Apply only this patch.

Download all attachments as: .zip

Change History (17)

Changed 12 years ago by mpatel

comment:1 follow-up: Changed 12 years ago by mpatel

  • Authors set to Mitesh Patel
  • Summary changed from make it so control-shift-enter sends a blank line to tinymce to [with patch, needs review] make it so control-shift-enter sends a blank line to tinymce

Tested in Cr3, FF3.5, and O9 on Linux and Cr2, FF3.5, IE8, O9, and S4 on XP.

comment:2 in reply to: ↑ 1 Changed 12 years ago by mpatel

Replying to mpatel:

Tested in Cr3, FF3.5, and O9 on Linux and Cr2, FF3.5, IE8, O9, and S4 on XP.

Confirmed, in the course of reviewing the new TinyMCE spkg at #6143.

comment:3 Changed 12 years ago by jason

  • Cc jason added

comment:4 Changed 12 years ago by kcrisman

  • Summary changed from [with patch, needs review] make it so control-shift-enter sends a blank line to tinymce to [with patch, partial positive review, needs review] make it so control-shift-enter sends a blank line to tinymce

This does make FF3.5 work with this on OSX.5. Interestingly, this key-binding already worked in Safari 4 (and continues to now). Also, Ctrl-Enter appears to have same effect as Enter currently, so it already does something different than in an evaluation cell - for what that's worth.

I can't give a full positive review because I don't have access to any other browsers, nor am I competent to make sure it doesn't introduce a nasty bug, but partial positive review. The rest of a review should not be hard.

comment:5 Changed 12 years ago by mpatel

Unfortunately, neither the existing bindings nor the simple Ctrl-Enter combination work consistently across browsers and platforms. But if Ctrl-Enter happens to work, that's great!

For what it's worth: The patch still works in the latest versions of the browsers I list above. (Cr3 is now Cr4 on Linux.)

Changed 12 years ago by mpatel

Tweaked and rebased against #6568. Apply only this patch.

comment:6 Changed 12 years ago by mpatel

  • Summary changed from [with patch, partial positive review, needs review] make it so control-shift-enter sends a blank line to tinymce to [with patch, needs review] make it so control-shift-enter sends a blank line to tinymce

Version 2:

  • Applies to #6568.
  • Fixes a problem with inserting a blank line when text is selected. I noticed this problem in Safari on Windows XP. This also fixes a related problem in Opera on Windows XP and Linux.
  • Adds a few comments and dedents/indents.

I think this ticket should be reviewed again (sorry!), since I don't have access to a Mac machine.

comment:7 Changed 11 years ago by kcrisman

  • Summary changed from [with patch, needs review] make it so control-shift-enter sends a blank line to tinymce to [with patch, partial positive review, needs review] make it so control-shift-enter sends a blank line to tinymce

Still works on Mac. I still think needs review by someone who understands javascript and someone other than author who uses non-Mac. mpatel, maybe you can ping some of the people on the thread where "Pat LeSmithe?" makes the comment in the ticket description? This shouldn't be hard to review for them

comment:8 Changed 11 years ago by mpatel

Changed 11 years ago by mpatel

Initialization code moved to tinymce.js. Rebased for #7196. Apply only this patch.

comment:9 Changed 11 years ago by mpatel

Remark: The TinyMCE developers are emphatic about using P elements instead of BR elements. If the control-shift-enter feature is broken, breaks, or otherwise causes problems, we might instead follow their CSS suggestion, p {margin:0; padding: 0;}, inside rich text cells.

comment:10 Changed 11 years ago by jason

I think the issue here was making multi-paragraph list items. Maybe we could just make it so that pressing control-shift-enter in a list makes a new paragraph inside of an <li>.

It really seems that there should be an easier way of doing this. I guess we are hampered by shift-enter, ctrl-enter, and alt-enter having more global meaning.

comment:11 follow-up: Changed 11 years ago by mpatel

On lists: I was working from this sage-devel post. The current patch should work inside lists, too.

On simplicity: I think many of TinyMCE's source lines are devoted to similar tricks, although they may well be far more effective.

Just let me know what I should do.

comment:12 in reply to: ↑ 11 Changed 11 years ago by mpatel

Replying to mpatel:

Just let me know what I should do.

In particular, a SageNB-rebased patch for #5447 will also touch sagenb/data/sage/html/notebook/head.tmpl.

comment:13 Changed 11 years ago by was

  • Resolution set to fixed
  • Reviewers set to kcrisman, jason, was
  • Status changed from needs_review to closed
  • Summary changed from [with patch, partial positive review, needs review] make it so control-shift-enter sends a blank line to tinymce to [with patch, positive review] make it so control-shift-enter sends a blank line to tinymce

Emphatic positive review.

This is awesome!

Merged into sagenb-3.0.2, hence sage-4.2.

comment:14 Changed 11 years ago by kcrisman

  • Reviewers changed from kcrisman, jason, was to Karl-Dieter Crisman, Jason Grout, William Stein
Note: See TracTickets for help on using tickets.