Ticket #6771 (review Bug)

Opened 4 years ago

Last modified 3 years ago

Strange <span> refactoring

Reported by: dinu Owned by: garry.yao
Priority: Normal Milestone:
Component: Core : Output Data Version: 3.3
Keywords: Cc:

Description

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

Attachments

6771.patch (3.7 KB) - added by garry.yao 4 years ago.
6771_2.patch (2.4 KB) - added by garry.yao 3 years ago.

Change History

comment:1 Changed 4 years ago by garry.yao

  • Status changed from new to closed
  • Resolution set to invalid

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.

comment:2 follow-up: ↓ 3 Changed 4 years ago by 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>

comment:3 in reply to: ↑ 2 Changed 4 years ago by garry.yao

  • Status changed from closed to reopened
  • Resolution invalid deleted

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

comment:4 Changed 4 years ago by garry.yao

  • Status changed from reopened to confirmed
  • Component changed from General to Core : Output Data

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>
    

comment:5 follow-up: ↓ 9 Changed 4 years ago by 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.

comment:6 Changed 4 years ago by 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

comment:7 Changed 4 years ago by 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).

comment:8 Changed 4 years ago by 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 :)

comment:9 in reply to: ↑ 5 Changed 4 years ago by 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>

Changed 4 years ago by garry.yao

comment:10 Changed 4 years ago by garry.yao

  • Keywords HasPatch added

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

comment:11 Changed 4 years ago by krst

  • Version changed from 3.4.3 (SVN - trunk) to 3.3

In CKE 3.2 result was:

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

comment:12 Changed 4 years ago by 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.

comment:13 Changed 3 years ago by dinu

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

Changed 3 years ago by garry.yao

comment:14 Changed 3 years ago by garry.yao

  • Owner set to garry.yao
  • Keywords HasPatch removed
  • Status changed from confirmed to review

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

Ticket TC added with [6426].

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy