Opened 8 years ago

Closed 7 years ago

#4830 closed Bug (fixed)

[IE] Editing image with keyboard does not retain the properties of the image when displaying the image dialog

Reported by: Damian Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.3
Component: General Version: SVN (CKEditor) - OLD
Keywords: IBM Confirmed Review+ Cc: Frederico Caldeira Knabben

Description (last modified by Frederico Caldeira Knabben)

To reproduce:

  1. Insert an image using the image dialog
  2. Using the keyboard, highlight the image
  3. Select the image action from the context or toolbar

Result: the image properties are not populated in the image dialog

Expected result: the dialog should be populated with the image's properties.

Note: using the mouse to select the image, does work as expected.

Tested on IE 6 & 7

Attachments (2)

4830.patch (5.6 KB) - added by Garry Yao 7 years ago.
4830_2.patch (5.6 KB) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 8 years ago by Frederico Caldeira Knabben

Description: modified (diff)
Milestone: CKEditor 3.1CKEditor 3.3

Changed 7 years ago by Garry Yao

Attachment: 4830.patch added

comment:2 Changed 7 years ago by Garry Yao

Keywords: Confirmed added
Owner: set to Garry Yao
Status: newassigned
Version: SVN (CKEditor)

Note that the problem not only affect all IE versions, but also influence Firefox and other browsers, it should be considered as major defect for accessibility.

comment:3 Changed 7 years ago by Garry Yao

Keywords: Review? added

The buggy thing is about the way we've used to figure out selected element in editor:

selection.getStartElement();		// Are you kidding?
selection.getSelectedElement();			// Maybe?
selection.getRanges()[0].getEnclosedNode();		// Closer?

Proposing a patch that bring more robust implementation to selection::getSelectedElement. Note that the patch includes part of the changes been proposed at #4513, which is similar to this problem.

comment:4 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review+ added; Review? removed

In dom/range.js:
There are three "if( " instead of "if ( "

The "if( this.collapsed )" doesn't need an else clause, you can put all the following code at the same level due to the return when it's true. You can then move the

    !mode && ( mode = CKEDITOR.SHRINK_TEXT ); 

after that line if you want.

And you can put the moveStart, moveEnd and currentElement declarations at the 1259 line

In selection/plugin.js: does it needs the

else 
    throw '';

part?

I would put the

var node = 
 	CKEDITOR.tools.tryThese( 

part in a single line.

comment:5 Changed 7 years ago by Alfonso Martínez de Lizarrondo

The patch also fixes #2570

Changed 7 years ago by Garry Yao

Attachment: 4830_2.patch added

comment:6 Changed 7 years ago by Garry Yao

Cc: Frederico Caldeira Knabben added
Keywords: Review? added; Review+ removed

Please leave the review role of this ticket to Fred.

comment:7 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

Please be sure to have design tests in place for the shrink function.

comment:8 in reply to:  4 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [5254] and [5255] and [5256].

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