Opened 10 years ago

Last modified 10 years ago

#6771 review Bug

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 (2)

6771.patch (3.7 KB) - added by Garry Yao 10 years ago.
6771_2.patch (2.4 KB) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 10 years ago by Garry Yao

Resolution: invalid
Status: newclosed

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 Changed 10 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 10 years ago by Garry Yao

Resolution: invalid
Status: closedreopened

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 10 years ago by Garry Yao

Component: GeneralCore : Output Data
Status: reopenedconfirmed

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 Changed 10 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 10 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 10 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 10 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 10 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 10 years ago by Garry Yao

Attachment: 6771.patch added

comment:10 Changed 10 years ago by Garry Yao

Keywords: HasPatch added

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

comment:11 Changed 10 years ago by Krzysztof Studnik

Version: 3.4.3 (SVN - trunk)3.3

In CKE 3.2 result was:

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

comment:12 Changed 10 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 10 years ago by Dinu

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

Changed 10 years ago by Garry Yao

Attachment: 6771_2.patch added

comment:14 Changed 10 years ago by Garry Yao

Keywords: HasPatch removed
Owner: set to Garry Yao
Status: confirmedreview

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 – 2020 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy