Opened 9 years ago

Closed 9 years ago

#3304 closed Task (fixed)

Remove the domWalker class

Reported by: Frederico Caldeira Knabben Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed Review+ Cc:

Description

The CKEDITOR.dom.walker class has been introduced to replace the CKEDITOR.dom.domWalker class. Now, any part of the code using the domWalker must be changed to use walker instead, and domWalker is to be delete.

Attachments (9)

3304.patch (10.7 KB) - added by Garry Yao 9 years ago.
3304_2.patch (9.9 KB) - added by Garry Yao 9 years ago.
3304_3.patch (11.9 KB) - added by Garry Yao 9 years ago.
3304_4.patch (11.5 KB) - added by Garry Yao 9 years ago.
3304_5.patch (11.4 KB) - added by Garry Yao 9 years ago.
3304_6.patch (23.8 KB) - added by Garry Yao 9 years ago.
3304_tc.patch (7.3 KB) - added by Martin Kou 9 years ago.
3304_7.patch (30.6 KB) - added by Garry Yao 9 years ago.
3304_8.patch (32.1 KB) - added by Garry Yao 9 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 9 years ago by Garry Yao

The new walker class 'CKEDITOR.dom.walker' been introduced at #2876 has changed the way of reverse traversing in the dom tree, comparing of the two algorithms:

  1. CKEDITOR.dom.domWalker is using exactly the reverse sequence of depth-first, pre-order traversal.
  2. CKEDITOR.dom.walker is using right-to-left, post-order traversal which is same as the one used by CKEDITOR.dom.node::getPreviousSourceNode.

While the new way of traversing satisfies many use-case like boundary checking logic within CKEDITOR.dom.range , but the original 'reverse source order' way is somehow still needed for certain cases like whole-word checking logic within find plugin, take the following example:

	1<p>^2^</p>

with the new walker logic, function CKEDITOR.dom.walker::previous will report text node of '1' as result, obviously the result is not good for determining whether text node of '2' is a whole-word matching. So I guess we still need to preserve the original logic of walker regard reverse traversal in some way.

comment:2 Changed 9 years ago by Frederico Caldeira Knabben

It's interesting to see... Martin got that first impression also.

It's also possible to do that by using the "guard" function. Check out the iterator code of the new walker class. It passes the guard function to getPreviousSourceNode and getNextSourceNode, which is then executed for every up and down (and match) in the walk.

comment:3 Changed 9 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

Changed 9 years ago by Garry Yao

Attachment: 3304.patch added

comment:4 Changed 9 years ago by Garry Yao

Keywords: Review? added

The patch is containing the latest patch of #3313 which it depends on for easy reviewing.

Changed 9 years ago by Garry Yao

Attachment: 3304_2.patch added

comment:5 Changed 9 years ago by Garry Yao

New patch comes with the following changes:

  1. Update with current turnk;
  2. Include the emergent part of the fixing on #3051 which has been defered.

comment:6 Changed 9 years ago by Martin Kou

Keywords: Review- added; Review? removed

Two feature problems:

  1. Many commands stopped working after the patch (e.g. list command)
  2. Match cyclic stopped working

Changed 9 years ago by Garry Yao

Attachment: 3304_3.patch added

comment:7 Changed 9 years ago by Garry Yao

Keywords: Review? added; Review- removed

Sorry, I've been forgetting to include certain changes to walker.js which is required here:

  1. walker should by default avoid using 'startFromSibling' to get next source node;
  2. augmenting CKEDITOR.dom.element::isBlockBoundary here as within the old domwalker.js.

comment:8 Changed 9 years ago by Martin Kou

Keywords: Review- added; Review? removed

It is still not working - anything that involves the DOM iterator seems to break paragraphs into pieces now.

e.g.

  1. Open replacebyclass.html.
  2. Put caret in the middle of the default paragraph.
  3. Press "Insert Numbered List" button.
  4. The paragraph is broken into two pieces.

comment:9 Changed 9 years ago by Martin Kou

But on the other hand, the issue with the find dialog seems to be fixed.

comment:10 Changed 9 years ago by Garry Yao

Keywords: Review? added; Review- removed

I've found a new possibility to further simplify the enlarge by block unit powered by the new 'walker', since there's no standalone TCs for enlarge block function, I've tested it with the domiterator test suite.

Changed 9 years ago by Garry Yao

Attachment: 3304_4.patch added

comment:11 Changed 9 years ago by Garry Yao

I've found that we can't rely on CKEDITOR.dom.range::getBoundaryNodes any more for marking boundary, in case of the following range:

^<p id="p1">paragraph<p>^

The function will report (startNode:p1, endNode:p1) as the result, it's right from the semantic perspective, but this info could never be used as position marker, since it's ambigulous for the walker to know whether it's at the beginning OR at the end, if the walker start with this start/end node, both call of 'waker.next()' and 'walker.previous()' will return the same result.
Currently I found a workaround by using bookmark nodes to establish a pseudo margin, like:

<span _fck_bookmark="1" /><p id="p1">paragraph<p><span _fck_bookmark="1"><span _fck_bookmark="1" />

So the walker is quite cleared about the position and it could walk back and forth at will.

Changed 9 years ago by Garry Yao

Attachment: 3304_5.patch added

comment:12 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review? removed

We're introducing important changes to the dom.walker class with #3367. I'm freezing this ticket until that one don't get concluded.

Changed 9 years ago by Garry Yao

Attachment: 3304_6.patch added

comment:13 Changed 9 years ago by Garry Yao

Keywords: Review? added

The patch is including the latest patch from #3477 for easy reviewing.

comment:14 Changed 9 years ago by Martin Kou

Keywords: Review- added; Review? removed

Still Review-

The new DOM range enlarge API isn't working.

For CKEDITOR.ENLARGE_LIST_ITEM_CONTENTS, it's not including the ending <BR> node. This is something being expected by other parts of the code (e.g. the list plugin).

For CKEDITOR.ENLARGE_BLOCK_CONTENTS, it seems to be halting incorrectly in front of <BR> nodes, and also it seems to be not valid for empty blocks.

Changed 9 years ago by Martin Kou

Attachment: 3304_tc.patch added

Changed 9 years ago by Garry Yao

Attachment: 3304_7.patch added

comment:15 Changed 9 years ago by Garry Yao

Keywords: Review? added; Review- removed

Fixing the above TCs.

comment:16 Changed 9 years ago by Martin Kou

Keywords: Review- added; Review? removed

The bugs with CKEDITOR.ENLARG_BLOCK_CONTENTS are still not fixed.

btw I'm actually not too sure about test_enlarge_block5... coz there're a few possible ways for a range to select a block. So even if it fails, as long as it's selecting the empty block, it's ok.

Changed 9 years ago by Garry Yao

Attachment: 3304_8.patch added

comment:17 in reply to:  16 Changed 9 years ago by Garry Yao

Keywords: Review? added; Review- removed

Replying to martinkou:

The bugs with CKEDITOR.ENLARG_BLOCK_CONTENTS are still not fixed.

Sorry for missing the enlarge(blockunit) TCs last time, they're really nice, helping to reveal some hideous bug inside walker.

comment:18 Changed 9 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:19 Changed 9 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3471].

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