Strange <span> refactoring

[Dinu]

insertHtml('<span><br><br><span>foo</span></span>')
is refactored to
<br><br><span><span>foo</span></span>
Only seems to happen with nested spans; makes no sense since none of the tags are blocks that would require a reconsideration of line breaks

[Garry Yao]

Nice catch, but it's our solution of keeping line-breaks out of applied inline style, and it has no wysiwyg impacts.Check #4886 for more details.

[Dinu]

I see, but shouldn't this be applied just to the relevant spans for styles? like only refactor <br>'s is spans that have some _ckeditor_style flag. It does have impact where, for instance, lineHeight differs across the containers. It also has impact in the sense that it will refactor user content that is not CK-generated.

It seems to also have some more hectic effects: try api demo, type in
<span><span>foo<br /></span></span>
And click set contents... result is
<br /><p><span><span>foo</span></span>

[Garry Yao]

Replying to dinu:

It seems to also have some more hectic effects: try api demo, type in
<span><span>foo<br /></span></span>
And click set contents... result is
<br /><p><span><span>foo</span></span>

This's something to check definitely...

[Garry Yao]

Reduced TC for this parser bug:

1. Load the editor with the following content:

   <span>foo<br /></span>
   

2. Check source;

• Actual Result:

  <br />
  <p>
  	<span>foo</span></p>
  

• Expected Result:

  <p>
    <br /><span>foo</span></p>
  

[Dinu]

No, expected result should be <p><span>foo</span><br /></p> if I get your logic right... anyway, that <br /> should not climb over text.

[Dinu]

But I still don't get it, why not just trim any leading or trailing line breaks when aplying styles, as opposed to refactoring whole code for this

[Dinu]

4886_2 would work excellent with a very small adjustment: exclude trailing <br /> only if it is preceded by content (e.g. don't trim empty lines). IMHO <br /> refactoring makes sense only in the context of actual line-break logic (making good with <p> and <div> blocks).

[Dinu]

With CSS3, I can imagine a number of uses that would be upset by <br>'s flying around (like special <br> styling, automatic line numberings, special rendering of consecutive br's, etc). Although in a narrow approach is just a small glitch, maybe from a design POV it deserves some attention. Sorry for the consecutive postings, I'm a compulsive writer :)

[Garry Yao]

Replying to dinu:

No, expected result should be <p><span>foo</span><br /></p> if I get your logic right... anyway, that <br /> should not climb over text.

My fault, let me come with a clearer TC:

<span>foo<br /></span>bar

=>

<p><span>foo</span><br /></p>

[Garry Yao]

Re-propose a cleaner way to have #4886 fixed.

[Krzysztof Studnik]

In CKE 3.2 result was:

<p>
	<span>foo</span></p>

[Dinu]

Looks dandy for my complaint. Haven't checked if it fixes #4886 but there sure ain't any more <br /> refactoring where I can see it.

[Dinu]

Will this make it into trunk? Seems 3.5.1 introduces even more "pending br" handling

[Garry Yao]

Update: as of [6391] also fix #4886, [5103] must be reverted.

Ticket TC added with [6426].