Opened 7 years ago

Closed 7 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 7 years ago.
9386_2.patch (5.7 KB) - added by Garry Yao 7 years ago.
9386_3.patch (8.4 KB) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 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 7 years ago by Garry Yao (previous) (diff)

Changed 7 years ago by Garry Yao

Attachment: 9386.patch added

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

Attachment: 9386_2.patch added

comment:3 Changed 7 years ago by Frederico Caldeira Knabben

Description: modified (diff)

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

Attachment: 9386_3.patch added

comment:5 Changed 7 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 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:7 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7624].

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