Opened 17 years ago

Closed 16 years ago

#1229 closed Bug (fixed)

<pre> should be merged when applied to multiple paragraphs

Reported by: Vialis Owned by: Martin Kou
Priority: Normal Milestone: FCKeditor 2.6.3
Component: Core : Styles Version: FCKeditor 2.4.3
Keywords: Confirmed Review+ Cc:

Description

When I use the format "Formatted" every newline leads to a new "Formatted" text block. I would expect that all text that is entered will be shown in the samen text block.

Attachments (4)

1229.patch (3.9 KB) - added by Martin Kou 16 years ago.
1229_2.patch (6.2 KB) - added by Martin Kou 16 years ago.
1229_3.patch (9.8 KB) - added by Frederico Caldeira Knabben 16 years ago.
1229_4.patch (9.8 KB) - added by Martin Kou 16 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 17 years ago by Frederico Caldeira Knabben

Component: GeneralCore : Styles
Keywords: Discussion added
Milestone: FCKeditor 2.6
Summary: FCKeditor and Mediawiki: when using format "Formatted" every newline leads to a new "Formatted" text block<pre> should be merged when applied to multiple paragraphs

We could prospect the following:

Initial structure:

    <p>Line 1</p>
    <p>Line 2</p>

After Format change:

    <pre>Line 1
    Line 2</pre>

The problem here is, what should happen when switching from <pre> back to <p>? I would expect it to generate a single <p> with <br> for each line break, just like this:

    <p>Line 1<br />
    Line 2</p>

But them people will complain that the transformation to <pre> and back to <p> destroys the initial structure. But it sounds like the way to go in any case. Any thoughts on this?

comment:2 Changed 16 years ago by Martin Kou

How about making the <pre> -> <p> logic like the conversion login in the "Paste As Text" dialog:

  • If two lines are separated by one line break, change the line break to <br>.
  • If two lines are separated by two line breaks, change the line break to a new block.

Note that some <pre> -> <p> conversion logic has been written in the patch to #1355, it already does the <br> conversion, but it doesn't do the new block conversion yet.

comment:3 Changed 16 years ago by Frederico Caldeira Knabben

Your idea sounds good Martin.

Btw, my previous example was wrong. The following is correct instead:

Initial structure:

    <p>Line 1</p>
    <p>Line 2</p>

After Format change:

    <pre>Line 1

    Line 2</pre>

comment:4 Changed 16 years ago by Frederico Caldeira Knabben

Keywords: Confirmed added; Discussion removed

comment:5 Changed 16 years ago by Martin Kou

Owner: set to Martin Kou
Status: newassigned

Changed 16 years ago by Martin Kou

Attachment: 1229.patch added

comment:6 Changed 16 years ago by Martin Kou

Keywords: Review? added

comment:7 Changed 16 years ago by Martin Kou

Keywords: Review? removed

Review request retracted because it was found that the patch can be simplified after a discussion with Fred.

comment:8 Changed 16 years ago by Martin Kou

Resolution: fixed
Status: assignedclosed

Fixed with [1790].

Click here for more info about our SVN system.

comment:9 Changed 16 years ago by Martin Kou

Resolution: fixed
Status: closedreopened

comment:10 Changed 16 years ago by Martin Kou

The previous fixed message was for #922, not this ticket.

Changed 16 years ago by Martin Kou

Attachment: 1229_2.patch added

comment:11 Changed 16 years ago by Martin Kou

Keywords: Review? added

comment:12 Changed 16 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The merging is broken on IE (requires the outerHTML trick to set the innerHTML). There is also part of the logic that I couldn't easily understand, and therefore would require some discussion.

Changed 16 years ago by Frederico Caldeira Knabben

Attachment: 1229_3.patch added

comment:13 Changed 16 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed
Milestone: FCKeditor 2.6FCKeditor 2.6.1

I've attached a new patch for it, derived from Martin's patch. I should discuss about it with Martin at this point to be sure things are ok, possibly merging parts of his patch to it to come with a final solution.

We'll need to do that calmly, so I'm postponing it a bit to not block the release of the 2.6.

comment:14 Changed 16 years ago by Martin Kou

Two flaws in the patch found:

  1. The way _CheckAndMergePre() calculates its own previous block instead of getting the previous block from the FCKDomRangeIterator loop makes it possible for non-selected <pre> blocks to get merged.
  2. While the logic of the _CheckAndSplitPre() in 1229_3.patch looks identical to the one in 1229_2.patch, it has a very subtle bug - cursor advancement is incorrectly handled when lastNewBlock exists. The bug causes _CheckAndSplitPre() to be unable to split a converted block to more than 2 parts, and also causes it to rearrange the order of the blocks, which is very bad.

I've included a fixed patch for review.

Changed 16 years ago by Martin Kou

Attachment: 1229_4.patch added

comment:15 in reply to:  14 Changed 16 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

Replying to martinkou:

The way _CheckAndMergePre() calculates its own previous block instead of getting the previous block from the FCKDomRangeIterator loop makes it possible for non-selected <pre> blocks to get merged.

That was actually the intention of it. But anyway, this is subject to interpretation. So, let's put it in production and catch the feedback. This is a strong benefit anyway.

comment:16 Changed 16 years ago by Martin Kou

Resolution: fixed
Status: reopenedclosed

Fixed with [2220].

Click here for more info about our SVN system.

comment:17 Changed 16 years ago by CKL

Resolution: fixed
Status: closedreopened

Hi

It seems that there is again a bug with <pre> tag in mediawiki Test on : http://mediawiki.fckeditor.net/index.php/Sandbox enter the following text then switch several times between wikitext

<pre> Hello Wiki TS User Hello Wiki TS User Hello Wiki TS User </pre> <pre> Bonjour Utilisateur Wiki TS Bonjour Utilisateur Wiki TS Bonjour Utilisateur Wiki TS Bonjour Utilisateur Wiki TS </pre>

comment:18 Changed 16 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: reopenedclosed

@CKL,

This is an old ticket that has already been fixed and released. Please open a new ticket for it.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy