Ticket #1229 (closed Bug: fixed)

Opened 22 months ago

Last modified 10 months ago

<pre> should be merged when applied to multiple paragraphs

Reported by: Vialis Owned by: martinkou
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

1229.patch (3.9 KB) - added by martinkou 16 months ago.
1229_2.patch (6.2 KB) - added by martinkou 16 months ago.
1229_3.patch (9.8 KB) - added by fredck 16 months ago.
1229_4.patch (9.8 KB) - added by martinkou 15 months ago.

Change History

  Changed 21 months ago by fredck

  • keywords Discussion added
  • summary changed from FCKeditor and Mediawiki: when using format "Formatted" every newline leads to a new "Formatted" text block to <pre> should be merged when applied to multiple paragraphs
  • component changed from General to Core : Styles
  • milestone set to FCKeditor 2.6

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?

  Changed 17 months ago by martinkou

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.

  Changed 17 months ago by fredck

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>

  Changed 17 months ago by fredck

  • keywords Confirmed added; Discussion removed

  Changed 16 months ago by martinkou

  • owner set to martinkou
  • status changed from new to assigned

Changed 16 months ago by martinkou

  Changed 16 months ago by martinkou

  • keywords Review? added

  Changed 16 months ago by martinkou

  • keywords Review? removed

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

  Changed 16 months ago by martinkou

  • status changed from assigned to closed
  • resolution set to fixed

Fixed with [1790].

Click here for more info about our SVN system.

  Changed 16 months ago by martinkou

  • status changed from closed to reopened
  • resolution fixed deleted

  Changed 16 months ago by martinkou

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

Changed 16 months ago by martinkou

  Changed 16 months ago by martinkou

  • keywords Review? added

  Changed 16 months ago by fredck

  • 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 months ago by fredck

  Changed 16 months ago by fredck

  • keywords Review? added; Review- removed
  • milestone changed from FCKeditor 2.6 to FCKeditor 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.

follow-up: ↓ 15   Changed 15 months ago by martinkou

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 15 months ago by martinkou

in reply to: ↑ 14   Changed 12 months ago by fredck

  • 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.

  Changed 12 months ago by martinkou

  • status changed from reopened to closed
  • resolution set to fixed

Fixed with [2220].

Click here for more info about our SVN system.

  Changed 10 months ago by CKL

  • status changed from closed to reopened
  • resolution fixed deleted

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>

  Changed 10 months ago by fredck

  • status changed from reopened to closed
  • resolution set to fixed

@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.