Ticket #4513 (closed Bug: fixed)

Opened 5 years ago

Last modified 5 years ago

Link selection - not always correct

Reported by: gryphius Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.3
Component: General Version: 3.0
Keywords: Confirmed Review+ Cc: pomu@…

Description

Hello!

When I create a link in CKEditor 3.0 and want to edit it, there are several methods of selecting the link - i.e. double-click it or a simple click. They all work fine.

But if I click on the left side of a link, holding down the mouse-button, mark it to the right side of the link, release the button and then click "edit" - the URL field is always empty.

in almost all browsers - Firefox 2.0.0.20, 3.5 or IE 8 for example, I can see this behaviour.

Thanks!

Attachments

4513.patch (4.4 KB) - added by garry.yao 5 years ago.
4513_2.patch (5.3 KB) - added by garry.yao 5 years ago.
4513_3.patch (7.2 KB) - added by garry.yao 5 years ago.
4513_4.patch (7.3 KB) - added by garry.yao 5 years ago.
4513_5.patch (4.0 KB) - added by garry.yao 5 years ago.

Change History

comment:1 Changed 5 years ago by garry.yao

  • Keywords link select selection removed
  • Status changed from new to closed
  • Resolution set to invalid

I click on the left side of a link, holding down the mouse-button, mark it to the right side of the link

It's not the correct way for select a link, and it's trivial.

comment:2 Changed 5 years ago by fredck

  • Status changed from closed to reopened
  • Keywords Confirmed added
  • Resolution invalid deleted
  • Milestone set to CKEditor 3.2

This is definitely a bug.

comment:3 Changed 5 years ago by garry.yao

Screencast for reproduce: http://screencast.com/t/wROj4SXN

comment:4 Changed 5 years ago by pomu0325

  • Cc pomu@… added

Changed 5 years ago by garry.yao

comment:5 Changed 5 years ago by garry.yao

  • Keywords Review? added

More precise link detection from range.

comment:6 Changed 5 years ago by garry.yao

  • Status changed from reopened to new
  • Owner set to garry.yao

comment:7 Changed 5 years ago by garry.yao

  • Status changed from new to assigned

comment:8 follow-up: ↓ 11 Changed 5 years ago by fredck

  • Keywords Review- added; Review? removed
  • The range::trim change looks like a duplication of range::optimize. Am I wrong? Other than that, the documentation added is not understandable. And finally, the verb "trim" means really "cut out", so it's expected to have text nodes split.
  • The changes at line 87 bring no benefit. They just make the code harder to read and longer.

comment:9 Changed 5 years ago by garry.yao

The changes at line 87 bring no benefit. They just make the code harder to read and longer.

That change was to guarantee the 'String' type of 'href' when the element is an anchor, so it's needed.

comment:10 Changed 5 years ago by garry.yao

Another R- reason for 4513.patch, where the link element is not detected in the following case:

[<b><a href="#">li]nk</a></b>]

Changed 5 years ago by garry.yao

comment:11 in reply to: ↑ 8 Changed 5 years ago by garry.yao

Replying to fredck:

  • The range::trim change looks like a duplication of range::optimize. Am I wrong? Other than that, the documentation added is not understandable. And finally, the verb "trim" means really "cut out", so it's expected to have text nodes split.

You're definitely correct, we just need range::optimize here instead of changing range::trim.

comment:12 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

Providing a new patch that targeting the above issues and also take context-menu listener into consideration.

comment:13 Changed 5 years ago by fredck

  • Keywords Review- added; Review? removed

The getSelectedLink method should not make DOM changes.

Changed 5 years ago by garry.yao

comment:14 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

Introduce an unobtrusive way to implement 'getSelectedLink' in previous patch, by establishing a 'CKEDITOR.dom.range::shrink' method, of course TCs are available:
run OR view source.

comment:15 Changed 5 years ago by fredck

  • Milestone changed from CKEditor 3.2 to CKEditor 3.3

comment:16 Changed 5 years ago by fredck

  • Keywords Review- added; Review? removed

That's a very good fix. There are just some small notes:

  • The "else" statement is not needed at line 1246 of range.js, because the "if" is using "return" on it.
  • Please keep more attention to the spacing after the "if" statement, to avoid things like [5149], [5150] and [5152].

Changed 5 years ago by garry.yao

comment:17 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

comment:18 Changed 5 years ago by fredck

  • Keywords Review- added; Review? removed

This patch must be re-proposed, considering that range::shrink has been introduced with [5255].

Changed 5 years ago by garry.yao

comment:19 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

Updated the patch with trunk.

comment:20 Changed 5 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:21 Changed 5 years ago by garry.yao

  • Status changed from assigned to closed
  • Resolution set to fixed

Fixed with [5304].

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