Opened 10 years ago

Closed 10 years ago

Last modified 10 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 10 years ago.
3283_2.patch (2.0 KB) - added by Garry Yao 10 years ago.
3283_3.patch (2.4 KB) - added by Garry Yao 10 years ago.
test-getPreviousSourceNode-getNextSourceNode.patch (5.5 KB) - added by Garry Yao 10 years ago.
test-getPreviousSourceNode-getNextSourceNode_2.patch (5.5 KB) - added by Frederico Caldeira Knabben 10 years ago.

Download all attachments as: .zip

Change History (15)

Changed 10 years ago by Garry Yao

Attachment: 3283.patch added

comment:1 Changed 10 years ago by Garry Yao

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

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

Attachment: 3283_2.patch added

comment:4 in reply to:  2 Changed 10 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 10 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 10 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 10 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 10 years ago by Garry Yao

Attachment: 3283_3.patch added

comment:8 Changed 10 years ago by Garry Yao

Fixing incorrect result when receives 'nodeType' param.

Changed 10 years ago by Frederico Caldeira Knabben

comment:9 Changed 10 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 10 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