Opened 14 years ago

Closed 14 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 14 years ago.
4830_2.patch (5.6 KB) - added by Garry Yao 14 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 14 years ago by Frederico Caldeira Knabben

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

Changed 14 years ago by Garry Yao

Attachment: 4830.patch added

comment:2 Changed 14 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 14 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 14 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 14 years ago by Alfonso Martínez de Lizarrondo

The patch also fixes #2570

Changed 14 years ago by Garry Yao

Attachment: 4830_2.patch added

comment:6 Changed 14 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 14 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 14 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 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy