Opened 3 years ago

Closed 3 years ago

#11822 closed Bug (fixed)

[Webkit] Anchors editing broken in some cases

Reported by: Olek Nowodziński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.4.1
Component: General Version: 4.4.0
Keywords: Cc:

Description

  1. Set the following HTML
    <p>f<a id="x" name="x">o</a>o</p>
    
  2. Double-click the anchor.

Expected: Existing anchor is edited.

Actual: A new anchor is edited.

git:b5acf14a92926 is the first bad commit.

Change History (8)

comment:1 Changed 3 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.1
Status: newconfirmed

comment:2 Changed 3 years ago by Piotrek Koszuliński

Not as critical as it might seem. It's only reproducible if anchor has adjacent non-white-space text nodes. Most likely doubleclick first cause expanding selection to whole word and then it's too late for anchor feature to react.

comment:3 Changed 3 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:4 Changed 3 years ago by Olek Nowodziński

Status: assignedreview

Pushed fix to branch:t/11822 (+tests).

This issue was much easier than suggested in comment:2. Basically git:b5acf14a restricted selection#selectElement( evt.data.link ) to elements to be edited in link dialog only, while non-empty anchors (empty ones are faked with img) also must be selected.

comment:5 Changed 3 years ago by Piotrek Koszuliński

I haven't noticed this while reviewing related tickets in 4.4, but the logic of doubleclick listeners (0,10,20) is tangled. Am I right that image2 could listen with priority <10, and link with the default one? I would be glad if we could revert this commit git:b5acf14a. Is it possible?

comment:6 in reply to:  5 Changed 3 years ago by Olek Nowodziński

Replying to Reinmar:

I haven't noticed this while reviewing related tickets in 4.4, but the logic of doubleclick listeners (0,10,20) is tangled. Am I right that image2 could listen with priority <10, and link with the default one? I would be glad if we could revert this commit git:b5acf14a. Is it possible?

Yes, it is possible. However, if Image2 listened with priority < 10 and canceled the event before it reaches listeners in link plugin, then it would need to handle this logic, which is what I wanted to avoid. My idea was to do it gently, so the process of link discovery (evt.data.dialog = 'link' vs 'anchor') remains on the Link plugin-side. And thanks to 0,10,20 listeners, it is possible – there is no redundant code, which would need attention, if something has changed in Link plugin. Also, if we decide to bring support for Image2+anchors in the future, there's nothing to do there, because Link plugin takes care of invoking the right dialog etc.

comment:7 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_passed

comment:8 Changed 3 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Fixed on master with git:70a8947 on dev and c0d14c0 on tests.

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