You are here: Foswiki>Tasks Web>Item2254 (04 Oct 2010, KennethLavrsen)Edit Attach

Item2254: Fix to Item1798 introduces cursor problems for Moz browsers

Priority: Normal
Current State: Closed
Released In: 1.1.0
Target Release: minor

Applies To: Extension
Component: TinyMCEPlugin, WysiwygPlugin
Branches:

Reported By: PaulHarvey
Waiting For: Main.PaulHarvey, Main.MichaelTempest
Last Change By: KennethLavrsen

Item1798 introduces forced_root_blocks: false to fix a problem where extraneous <p> tags are added.

However this introduces strange "cursor jumping" behaviour, especially when working with bullets (easy to reproduce: see TestTopicWithDefaultTMCE, and the "fixed" TINYMCEPLUGIN_INIT at TestTopicWithForceRootBlocks).

The above test topics demonstrate easy to reproduce cursor jumping problems when trying to add newlines after a bullet list.

I have users telling me it sometimes happens with Heading lines as well, although I can't reproduce that.

Setting forced_root_blocks: false also inhibits a work-around the moxiecode guys had for the infamous cursor-trapped-in-tables where you couldn't get the cursor beyond (or before) a table at the end (or beginning) of a document in non-IE browsers.

I will try to raise a bug upstream; in the meantime, we should decide if there is anything we can do to alleviate this problem for now.

-- PaulHarvey - 15 Oct 2009

Moxiecode FAQ explaining new behaviour.

Threads of interest: p tags around hr,table,div etc., cursor trapped in tables work-around introduces <p/> elements, If you have troubles placing the cursor after the last table.. ([a plugin created before the official fix) (download)

-- PaulHarvey - 15 Oct 2009

This is limited to gecko browsers.

Problem replicated on these browsers:

Iceweasel/Firefox 3.0.12 (Linux)
Firefox 3.5.3 (Windows XP SP3)
Epiphany 2.22.3 (Linux)

Unable to replicate on:

Opera 10.00 build 4585 (Linux)
Opera 10.00 build 1750 (Windows XP SP3)
Google Chrome 3.0.195.27 (Windows XP SP3)
Safari 4.0.3 (531.9.1) (Windows XP SP3)
Internet Explorer 6.0.2900.5512
Internet Explorer 8.00.6001

-- PaulHarvey - 15 Oct 2009

bug reported upstream

-- PaulHarvey - 15 Oct 2009

Added comment on sf.net where I found a few lines of code that can be commented out to hide this problem.

-- PaulHarvey - 15 Oct 2009

I've been poring over ForceBlocks.js (specifically insertPara where the problem is).

There's code there that apparently affects Gecko browsers (though there's no specific sniffing) which works around the problem that it seems impossible to get the caret position in situations where (as far as I can tell) non-block elements like <br/> end up inside <body>. Sounds like a gecko bug to me, but I don't know much... Compare to chrome, which the DOM browser seems to show insertion of DIVs.

Commenting out the code that forces the cursor to jump to the top of the document results in a series of <br/> newlines after an li or ol list, as opposed to <p> paras normally.

Not sure where to go from here. As a few important cursor bugs seem to be fixed with forced_root_blocks not disabled, I am feeling more and more that perhaps we should look at how we can perhaps tune WysiwygPlugin to cope with TMCE's default init parameters.

-- PaulHarvey - 16 Oct 2009

I agree - the right way to go is to make WysiwygPlugin produce (and consume) HTML more to TMCE's liking. I am nervous of a change like that, but I think it is needed.

-- MichaelTempest - 16 Oct 2009

Actually, this is a very sticky problem. I've been trying to think of how WysiwygPlugin could handle the tables issue without forced_root_blocks: false and it's not coming to me...

I have a regrettably adventurous patch which I doubt upstream would merge, that could fix the bullets situation with forced_root_blocks: false, but we'd still have to put up with the cursor getting trapped in tables problem I suppose.

-- PaulHarvey - 16 Oct 2009

Downgraded to Normal, this bug has been with us since 1.0.6.

-- PaulHarvey - 18 Oct 2009

Have been trying to make my work-around for this work as a plugin. It's not working out; overriding an internal TMCE function is proving very difficult. So this will have to be a patch to classes/ForceBlocks.js.

-- PaulHarvey - 19 Oct 2009

Added some deeper analysis to Item8274

-- PaulHarvey - 20 Oct 2009

Don't bother with the release files below, they contain an error :(

-- PaulHarvey - 20 Oct 2009

Actually, forced_root_blocks: false also prevents Opera 10 from playing nice as well.

-- PaulHarvey - 21 Oct 2009

Right. The proper way is to just remove forced_root_blocks : false.

And then the fix for the tables (and other) problem in Item1798 is to just ensure that TinyMCE receives all the $content inside a <div>. Bang, no more non-block elments without a block container to confuse web browser node selections

Just to fix the unit tests...

-- PaulHarvey - 21 Oct 2009

I have been running this seemingly without being any worse off. The list item bugs are gone, Opera behaves properly and everything seems happy.

However it seems TMCE's fixes for the cursor being unable to get beyond a table that's positioned at the beginning or end of a document only works if the table is a top-level element.

So I think if we want that bug fixed, we have to only put divs around table + text that immediately follows.

-- PaulHarvey - 21 Oct 2009

I committed Rev 5347 not found. TML2HTML wraps the output in <div></div>. I adjusted the unit tests so that they will pass with this arrangement.

After some review. hopefully CrawfordCurrie and MichaelTempest, I would like to a make release and ask people on foswiki-discuss to test it.

-- PaulHarvey - 22 Oct 2009

I have to spend some time on other things. We won't have a proper fix for this for 1.0.8.

-- PaulHarvey - 22 Oct 2009

I can make this all work, including TMCE's fixes for the cursor being unable to get beyond a table that is positioned at the beginning or end of a document.

Set forced_root_block to its default (i.e. remove the setting from TINYMCEPLUGIN_INIT)
Wrap the output of TML2HTML in a <div>
Open the fullscreen view after editing the topic. (It still works after closing the fullscreen view.)

PaulHarvey proposed 1 and 2, and I support that change. I believe that 3 is evidence of a bug in TMCE or in TinyMCEPlugin - this action also fixes other problems e.g. Item5955.

(To move before a table at the beginning of a document, I had to use Ctrl-left-arrow. Similarly, I had to use Ctrl-right-arrow to move beyond a table at the end of a document. I discovered these by accident. Are they documented somewhere?)

-- MichaelTempest - 21 Nov 2009

I suppose ctrl+arrow is probably a browser thing, rather than TMCE.

It seems gecko's node selection stays internally consistent for the browser GUI itself; in that - even though the Javascript DOM starts reporting nonsense about which node is currently selected with the cursor position - when running with TMCE's cursor-reset-to-top-of-page code disabled, we can still work in the rich text editor just fine (but things like mceInsertContent command won't insert at the cursor position when it's invalid).

So, what am I saying...

Perhaps we can teach WysiwygPlugin to remove the <p> around the table? If the user really wanted a newline after the table, they could insert an empty <p></p> after it.
Perhaps we can teach WysiwygPlugin to wrap tables in their own <div>s.
Speaking of <p>s and tables, should we teach WysiwygPlugin to eat the <p>s that end up in table cells, as per Item5221...

-- PaulHarvey - 22 Nov 2009

Item2447 is also caused by this. Michael, added a comment about your proposal (3) in Item5955.

TML2HTML.pm.diff: Proof of concept div-around-tables+follwing-paras fix

Obviously we can't commit this patch yet, apart from needing more testing:

end-of-div and end-of-para uses the same logic. Refactor those four lines which are copy-pasted everywhere into a common method.
We don't want divs around our tables unless there really is a line of text that immediately follows it.
Needs unit tests.

It does however, allow me to run without forced_root_blocks: false, and it does mean we haven't cheated by wrapping everything in a div, so we are much closer to Moxiecode's recommended usage of TinyMCE which should hopefully mean less bugs

-- PaulHarvey - 05 Dec 2009

Actually, the patch I have recently attached is bogus - it should only merge following text into a common block element if there's only one newline separating the table and the following text. Instead it forces all following text ... but anyway, that's why it's proof of concept.

MichaelTempest: We could feasibly ship Rev 5347 not found as you say as a temporary solution. Have you given it much testing?

I have created Item2471 to implement what I think is the "better" solution...

-- PaulHarvey - 07 Dec 2009

Actually, as of TinyMCE 3.3.3 - we don't get cursor jumping any more when finishing bullet lists in Firefox

-- PaulHarvey - 20 Apr 2010

WysiwygPlugin wraps tables in divs if there are macros before or after the table. TinyMCEPlugin no longer sets force_root_blocks: false.

I have done some testing. More is needed.

-- MichaelTempest - 15 May 2010

Michael, have been using this for two weeks on our production site, so far so good! Overall, trunk WysiwygPlugin+TMCE seems to be an improvement over 1.0.9. Great work

-- PaulHarvey - 05 Jun 2010

I plan no further changes on this task, so I am setting it to "waiting for release"

-- MichaelTempest - 07 Jun 2010

ItemTemplate edit

Summary	Fix to Item1798 introduces cursor problems for Moz browsers
ReportedBy	PaulHarvey
Codebase	1.0.7, 1.0.6, trunk
SVN Range	Foswiki-1.0.7, Sun, 20 Sep 2009, build 5061
AppliesTo	Extension
Component	TinyMCEPlugin, WysiwygPlugin
Priority	Normal
CurrentState	Closed
WaitingFor	PaulHarvey, MichaelTempest
Checkins	Rev 5330 not found Rev 5331 not found distro:d1022e591ac0 Rev 5344 not found Rev 5346 not found Rev 5347 not found Rev 5349 not found distro:16b6c81c76c2 distro:138380ef522b distro:993e2f19d19a distro:1cc0d862dccc distro:05cc5da6d7c5 distro:352e17ba8e28 distro:fe9602f9027d distro:7c3762167836
TargetRelease	minor
ReleasedIn	1.1.0

I	Attachment	Action	Size	Date	Who	Comment
diff	TML2HTML.pm.diff	manage	2 K	06 Dec 2009 - 04:57	PaulHarvey	Proof of concept div-around-tables+follwing-paras fix
md5	TinyMCEPlugin.md5	manage	162 bytes	22 Oct 2009 - 14:37	PaulHarvey	3.2.7 with Rev 5347 not found
sha1	TinyMCEPlugin.sha1	manage	186 bytes	22 Oct 2009 - 14:37	PaulHarvey	3.2.7 with Rev 5347 not found
tgz	TinyMCEPlugin.tgz	manage	1 MB	22 Oct 2009 - 14:38	PaulHarvey	3.2.7 with Rev 5347 not found
zip	TinyMCEPlugin.zip	manage	1 MB	22 Oct 2009 - 14:38	PaulHarvey	3.2.7 with Rev 5347 not found
EXT	TinyMCEPlugin_installer	manage	38 K	22 Oct 2009 - 14:39	PaulHarvey	3.2.7 with Rev 5347 not found
md5	WysiwygPlugin.md5	manage	162 bytes	22 Oct 2009 - 15:06	PaulHarvey	Foswikirev:5347
sha1	WysiwygPlugin.sha1	manage	186 bytes	22 Oct 2009 - 15:06	PaulHarvey	Foswikirev:5347
tgz	WysiwygPlugin.tgz	manage	49 K	22 Oct 2009 - 15:06	PaulHarvey	Foswikirev:5347
zip	WysiwygPlugin.zip	manage	57 K	22 Oct 2009 - 15:09	PaulHarvey	Foswikirev:5347
EXT	WysiwygPlugin_installer	manage	5 K	22 Oct 2009 - 15:11	PaulHarvey	Foswikirev:5347
EXT	test_html	manage	555 bytes	15 Oct 2009 - 05:46	PaulHarvey	Test case to submit to upstream
diff	tinyMCE_3.2.7.diff	manage	2 K	20 Oct 2009 - 05:46	PaulHarvey
diff	tinyMCE_trunk.diff	manage	2 K	20 Oct 2009 - 03:35	PaulHarvey	Patch to trunk rev1250 of tinyMCE