Opened 7 years ago

Closed 6 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: damo Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.3
Component: General Version: SVN (CKEditor) - OLD
Keywords: IBM Confirmed Review+ Cc: fredck

Description (last modified by fredck)

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 6 years ago.
4830_2.patch (5.6 KB) - added by garry.yao 6 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 years ago by fredck

  • Description modified (diff)
  • Milestone changed from CKEditor 3.1 to CKEditor 3.3

Changed 6 years ago by garry.yao

comment:2 Changed 6 years ago by garry.yao

  • Keywords Confirmed added
  • Owner set to garry.yao
  • Status changed from new to assigned
  • Version set to 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 6 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 follow-up: Changed 6 years ago by alfonsoml

  • 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 6 years ago by alfonsoml

The patch also fixes #2570

Changed 6 years ago by garry.yao

comment:6 Changed 6 years ago by garry.yao

  • Cc fredck added
  • Keywords Review? added; Review+ removed

Please leave the review role of this ticket to Fred.

comment:7 Changed 6 years ago by fredck

  • 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 6 years ago by garry.yao

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

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

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