Opened 3 years ago

Closed 8 months ago

#11956 closed Bug (fixed)

[IE, Chrome, Opera] Dialog box doesn't open on two words links with background.

Reported by: Artur Delura Owned by: Tomasz Jakut
Priority: Nice to have (we want to work on it) Milestone: CKEditor 4.7.0
Component: General Version: 4.0
Keywords: IE Blink Cc:

Description

  1. Open editor with following content
    Hello <a href="http://en.wikipedia.org/wiki/Neil_Armstrong">Neil Arm</a> .
    
  2. Select link Neil Arm.
  3. Change selection background color.
  4. Double click Arm word to open link dialog.

Actual result: Dialog won't open.

Please note: Double click on word Neil open dialog properly.

Change History (20)

comment:1 Changed 3 years ago by Jakub Ś

Keywords: IE Blink added
Status: newconfirmed
Version: 4.0

I confirm this issue for Blink and IE8+.

Problem can in fact be reproduced from CKEditor 3.0 but I'm setting 4.0 for it because we don't support CKE 3.x anymore.

comment:2 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5

comment:3 Changed 23 months ago by Marek Lewandowski

Milestone: CKEditor 4.5.5CKEditor 4.5.6

comment:4 Changed 23 months ago by Tomasz Jakut

Owner: set to Tomasz Jakut
Status: confirmedassigned

comment:5 Changed 23 months ago by Tomasz Jakut

Status: assignedreview

It seems that clicking the link which has descendants sometimes report that descendant as a clicked element (especially if link is immediately followed by unlinked text), so I just propose to check not only the clicked element, but also its ancestors to find the clicked link. Pushed fix to branch:t/11956.

comment:6 Changed 22 months ago by Olek Nowodziński

Status: reviewreview_failed
  1. https://github.com/cksource/ckeditor-dev/commit/7e25e68a7338b1bc57fae4db0155986849b8d6aa#diff-d9e496f4ce47ad70bef0ba672713d3a1R293 isn't it exactly the same as CKEDITOR.dom.node.getAscendant? If so, please use the core method.
  1. I'd rather avoid using selection for assertions unless it is necessary (selection is slow and quirky). And since doubleclick listeners in Link Plugin communicate using evt.data object, and as long as the only thing that really matters here is whether a link has been detected, it's easier to fetch evt.data.link and check if The Link has been found, before selection kicks in. So I'd rather see it this way:
    // #11956
    'test select link with descendants on double-click': function() {
    	var bot = this.editorBot,
    		editor = bot.editor;
    
    	editor.once( 'doubleclick', function( evt ) {
    		evt.cancel();
    
    		resume( function() {
    			assert.areSame( editor.document.findOne( 'a' ), evt.data.link, 'Link selected' );
    		} );
    	} );
    
    	bot.setData( '<p>a<a href="http://bar"><span style="background:#f00;">b</span></a>c</p>', function() {
    		editor.fire( 'doubleclick', {
    			element: editor.document.findOne( 'span' )
    		} );
    
    		wait();
    	} );
    }
    
  1. Other than that, I'm cool with the solution :)

comment:7 Changed 22 months ago by Marek Lewandowski

Milestone: CKEditor 4.5.6CKEditor 4.5.7

comment:8 Changed 21 months ago by Tomasz Jakut

Status: review_failedreview
  1. Yes, they're the same. I've changed code to use element.getAscendant.
  2. Test's updated.

Pushed to branch:t/11956.

comment:9 Changed 21 months ago by Olek Nowodziński

Status: reviewreview_failed

I pushed 2 minor commits to the branch. Also found an issue:

  1. Open http://ckeditor.dev/samples/.
  2. Double–click any text, which is not a link.
  3. uncaught TypeError: Cannot read property 'isReadOnly' of null(anonymous function) @ plugin.js:99 is thrown.

comment:10 Changed 21 months ago by Tomasz Jakut

Status: review_failedreview

I've fixed that issue by adding additional check if element is set, as element.getAscendant returns nothing if there is no link.

Pushed branch:t/11956.

comment:11 Changed 20 months ago by Marek Lewandowski

Milestone: CKEditor 4.5.7CKEditor 4.5.8

comment:12 Changed 18 months ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:13 Changed 17 months ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.5.10

comment:14 Changed 15 months ago by Marek Lewandowski

Milestone: CKEditor 4.5.10CKEditor 4.5.11

Moving tickets to the next milestone.

comment:15 Changed 13 months ago by Marek Lewandowski

Milestone: CKEditor 4.5.11CKEditor 4.6.1

comment:16 Changed 10 months ago by Marek Lewandowski

Milestone: CKEditor 4.6.1CKEditor 4.6.2

comment:17 Changed 9 months ago by Marek Lewandowski

Milestone: CKEditor 4.6.2
Priority: NormalNice to have (we want to work on it)

Moving to the nice to have list.

comment:18 Changed 8 months ago by kkrzton

Milestone: CKEditor 4.7.0

Rebased on the current major, targeting 4.7.0.

comment:19 Changed 8 months ago by kkrzton

Status: reviewreview_passed

LGTM!

Updated manual test version tag and added elementspath plugin (so it is more visible what was selected) - aaddd57.

comment:20 Changed 8 months ago by kkrzton

Resolution: fixed
Status: review_passedclosed

Merged to major with c390920.

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