Opened 7 years ago

Closed 4 years ago

#7975 closed Bug (fixed)

Errors when trying to select empty table cell on IE8

Reported by: Jakub Ś Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.4.2
Component: General Version: 3.5.3
Keywords: IBM IE Cc: Satya Minnekanti, Christophe Guillou, irinauru@…

Description (last modified by Jakub Ś)

The below description was copied from bug #7928 comment 3.

Problems using IE 7, 8 & 9 when using the elementspath ! Try the following :

  1. Create new table
  2. Put the cursor in a cell
  3. Click "td" in the elementspath : JAVASCRIPT ERROR

Message: 'undefined' is empty or not an Object
Line: 1578
URI: /ckeditor/_source/core/dom/range.js

This hasn't worked for IE6-10, in CKEditor 3.x, from CKEditor 3.5.3 rev [6559]. In CKEditor 4.x this doesn't work only for IE7 and IE8.

Attachments (1)

7975.patch (857 bytes) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 7 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

Regression of [5463], can't reproduce anymore #5568 on trunk?

Changed 7 years ago by Garry Yao

Attachment: 7975.patch added

comment:3 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

With IE9+Compat:

  1. Load this HTML:
<table><tr><td>Test</td></tr></table>
  1. Click inside the cell.
  2. Click the "td" element in the elements path.

The first character will not get selected.

comment:4 Changed 6 years ago by Jakub Ś

#10122 was marked as duplicate.

comment:5 Changed 6 years ago by Jakub Ś

Description: modified (diff)

comment:6 Changed 4 years ago by Jakub Ś

Cc: Satya Minnekanti Christophe Guillou added
Keywords: IBM added
Milestone: CKEditor 4.4.2

#11991 was marked as duplicate.

comment:7 Changed 4 years ago by Irina

Cc: irinauru@… added

comment:8 Changed 4 years ago by Piotrek Koszuliński

Owner: changed from Garry Yao to Piotrek Koszuliński
Status: review_failedassigned

Pushed branch:t/7975. This solution needs tests (both manual and unit).

comment:9 Changed 4 years ago by Piotrek Koszuliński

Status: assignedreview

Pushed tests to branch:t/7975.

comment:10 Changed 4 years ago by Piotrek Koszuliński

Summary: Errors when creating a table in IEErrors when trying to select empty table cell on IE8

comment:11 Changed 4 years ago by Piotr Jasiun

Status: reviewreview_failed

Are you sure that using cached collapsed and updating it makes sense at all? This collapsed is used twice (here and here) and when I replaced both with range.collapsed everything seems to work fine. Moreover we change the range before we use this cached collapsed for the second time. In this case it is not a problem, but caching part of the range when we manipulate it is a bad practice.

If there is a reason why collapsed is cached there should be a comment, otherwise we should use range.collapsed to avoid this problems in the future.

comment:12 Changed 4 years ago by Piotrek Koszuliński

Status: review_failedreview

You assume that you know what that value means. But none of us really do. There are operations made on range before the cached value is checked, so it's very probable that cached and property diverge often and perhaps that's an intended behaviour.

I don't want to spend a week on understanding this code. It's there for ages and it fixes many browser bugs, so expecting it to be straightforward is unwise.

Therefore, I focused on single case that has been reported in this ticket. My change affects only behaviour in tables and we'll get back to this code when next bug is reported.

comment:13 Changed 4 years ago by Piotr Jasiun

Status: reviewreview_passed

So beside this everything is fine. I rebased branch on the top of the master.

comment:14 Changed 4 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Fixed on master with git:850b2e4.

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