Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#3283 closed Bug (invalid)

'CKEDITOR.dom.node.getPreviousSourceNode/getNextSourceNode' bug

Reported by: Garry Yao Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed Review- Cc:

Description

I've detected the 'previous source node' is incorrect with the attache d TC.

Attachments (5)

3283.patch (590 bytes) - added by Garry Yao 9 years ago.
3283_2.patch (2.0 KB) - added by Garry Yao 9 years ago.
3283_3.patch (2.4 KB) - added by Garry Yao 9 years ago.
test-getPreviousSourceNode-getNextSourceNode.patch (5.5 KB) - added by Garry Yao 9 years ago.
test-getPreviousSourceNode-getNextSourceNode_2.patch (5.5 KB) - added by Frederico Caldeira Knabben 9 years ago.

Download all attachments as: .zip

Change History (15)

Changed 9 years ago by Garry Yao

Attachment: 3283.patch added

comment:1 Changed 9 years ago by Garry Yao

Keywords: Confirmed Review? added
Owner: set to Garry Yao
Status: newassigned

comment:2 Changed 9 years ago by Martin Kou

#2980 may be affected by this change since it depends on the wrong behavior of getPreviousNode() - actually it tries to get the right behavior by feeding it a true argument.

comment:3 Changed 9 years ago by Martin Kou

Keywords: Review- added; Review? removed

It isn't possible to walk backwards in DFS order without using lastChild.

e.g. the following script, when executed within the replacebyclass.html sample, will never give you the correct backwards walk no matter whether you feed true or false to getPreviousSourceNode():

f = frames[0];
c = l = new CKEDITOR.dom.element(f.document.links[0]).
getNextSourceNode();   // The "FCKeditor" text node
while(c = c.getPreviousSourceNode(true))
console.log(c.getOuterHtml ? c.getOuterHtml() : c.getText());

Changed 9 years ago by Garry Yao

Attachment: 3283_2.patch added

comment:4 in reply to:  2 Changed 9 years ago by Garry Yao

Replying to martinkou:

#2980 may be affected by this change since it depends on the wrong behavior of getPreviousNode() - actually it tries to get the right behavior by feeding it a true argument.

This function has a lot of confusing which is error prone, and I've spotted some places which relay on the wrong behavior of this function, let's figure out and fix them after we have a common cognition with the 'correct' behavior.

comment:5 Changed 9 years ago by Garry Yao

Keywords: Review? added; Review- removed

I've come with a new patch which simplified the implementation by utilize 'domWalker' and updated the TCs. Please first give a review to those TCs and then the patch.

comment:6 in reply to:  3 Changed 9 years ago by Garry Yao

Replying to martinkou: I've tested with the new patch's passing your TC Martin, please reconfirm.

comment:7 Changed 9 years ago by Garry Yao

Summary: 'CKEDITOR.dom.node.getPreviousSourceNode' bug'CKEDITOR.dom.node.getPreviousSourceNode/getNextSourceNode' bug

Update the description since the CKEDITOR.dom.node.getNextSourceNode is not right also, we're going to fix it here.

Changed 9 years ago by Garry Yao

Attachment: 3283_3.patch added

comment:8 Changed 9 years ago by Garry Yao

Fixing incorrect result when receives 'nodeType' param.

Changed 9 years ago by Frederico Caldeira Knabben

comment:9 Changed 9 years ago by Frederico Caldeira Knabben

Resolution: invalid
Status: assignedclosed

The original TCs are incorrect. Let me make some clarifications to justify it.

Suppose we have the following structure:

<p><b>A</b>B</p><div>C</div>

These two functions must return the first "touched node", from LTR or RTL, depending on the function.

So getNextSourceNode is supposed to return us the nodes in this order:

<p> -> <b> -> A -> B -> <div> -> C

getPreviousSourceNode instead is not supposed to give the exact opposite of it, but to apply the same logic, reading the code from right to left:

<div> -> C -> <p> -> B -> <b> -> A

In this case, the "touch point" of the elements are their "end boundary".

It's also quite easy to understand the why of it. In the original TCs conception, getPreviousSourceNode should return <p> by both <b> and <div>, which is wrong as we need to different results for each one of them, considering their different locations.

I've attached the correct TCs for it.

comment:10 Changed 9 years ago by Garry Yao

Keywords: Review- added; Review? removed

Now I understood the semantic of 'getPreviousSourceNode' function throughly Fred, I'll update the original TCs.

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