Opened 3 years ago

Closed 5 months ago

#11956 closed Bug (fixed)

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

Reported by: a.delura Owned by: t.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 j.swiderski

  • Keywords IE Blink added
  • Status changed from new to confirmed
  • Version set to 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 19 months ago by m.lewandowski

  • Milestone set to CKEditor 4.5.5

comment:3 Changed 19 months ago by m.lewandowski

  • Milestone changed from CKEditor 4.5.5 to CKEditor 4.5.6

comment:4 Changed 19 months ago by t.jakut

  • Owner set to t.jakut
  • Status changed from confirmed to assigned

comment:5 Changed 19 months ago by t.jakut

  • Status changed from assigned to review

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 18 months ago by a.nowodzinski

  • Status changed from review to review_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 18 months ago by m.lewandowski

  • Milestone changed from CKEditor 4.5.6 to CKEditor 4.5.7

comment:8 Changed 18 months ago by t.jakut

  • Status changed from review_failed to review
  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 18 months ago by a.nowodzinski

  • Status changed from review to review_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 18 months ago by t.jakut

  • Status changed from review_failed to review

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 16 months ago by m.lewandowski

  • Milestone changed from CKEditor 4.5.7 to CKEditor 4.5.8

comment:12 Changed 14 months ago by m.lewandowski

  • Milestone changed from CKEditor 4.5.8 to CKEditor 4.5.9

comment:13 Changed 13 months ago by m.lewandowski

  • Milestone changed from CKEditor 4.5.9 to CKEditor 4.5.10

comment:14 Changed 11 months ago by m.lewandowski

  • Milestone changed from CKEditor 4.5.10 to CKEditor 4.5.11

Moving tickets to the next milestone.

comment:15 Changed 10 months ago by m.lewandowski

  • Milestone changed from CKEditor 4.5.11 to CKEditor 4.6.1

comment:16 Changed 6 months ago by m.lewandowski

  • Milestone changed from CKEditor 4.6.1 to CKEditor 4.6.2

comment:17 Changed 5 months ago by m.lewandowski

  • Milestone CKEditor 4.6.2 deleted
  • Priority changed from Normal to Nice to have (we want to work on it)

Moving to the nice to have list.

comment:18 Changed 5 months ago by k.krzton

  • Milestone set to CKEditor 4.7.0

Rebased on the current major, targeting 4.7.0.

comment:19 Changed 5 months ago by k.krzton

  • Status changed from review to review_passed

LGTM!

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

comment:20 Changed 5 months ago by k.krzton

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

Merged to major with c390920.

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