Opened 12 years ago

Closed 12 years ago

#9386 closed Bug (fixed)

[Webkit] Missing case for filler char in collapsed selection

Reported by: Garry Yao Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.5
Component: Core : Selection Version: 3.6.5
Keywords: Cc:

Description (last modified by Frederico Caldeira Knabben)

There's still some logic hole in filler char creating detection, which breaks the following TC:

http://ckeditor.t/tt/7912/1.html

Attachments (3)

9386.patch (5.2 KB) - added by Garry Yao 12 years ago.
9386_2.patch (5.7 KB) - added by Garry Yao 12 years ago.
9386_3.patch (8.4 KB) - added by Garry Yao 12 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 12 years ago by Garry Yao

Owner: set to Garry Yao
Status: newreview

The patch expanded the filler creation check, in considering of hidden element.

Besides two commonly requested traversal utility are proposed in range.js.

Last edited 12 years ago by Garry Yao (previous) (diff)

Changed 12 years ago by Garry Yao

Attachment: 9386.patch added

comment:2 Changed 12 years ago by Garry Yao

The above patch breaks some current TCs.

Considering that it's not easy to enumerate all possible (see code comments) DOM structure where an empty fix is required, so the proposal takes a new approach - rely on range::moveToElementEditStart/End to check whether the filler is needed, and I've opened t/9386@test that cited all known cases for now.

Changed 12 years ago by Garry Yao

Attachment: 9386_2.patch added

comment:3 Changed 12 years ago by Frederico Caldeira Knabben

Description: modified (diff)

comment:4 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

I'm against the changes on CKEDITOR.dom.walker.invisible. They sound like a hack to achieve the fix here, at the price of making the function counterintuituve.

How should I expect an empty text node not to be invisible just because it may eventually be inside a visible element?... something like this: <p>Text<br>[Empty Text Node]</p>

Changed 12 years ago by Garry Yao

Attachment: 9386_3.patch added

comment:5 Changed 12 years ago by Garry Yao

Status: review_failedreview

The current walker::invisible impl has taken into consideration for your cited case, a text node's visibility can be considered as determinated by it's parent.

In order to simply the review and test run, I would combine the #9384 and #9386 patches, which are considered as revalent.

comment:6 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:7 Changed 12 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7624].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy