Opened 3 years ago

Closed 2 years ago

#13548 closed Bug (fixed)

IE: Clicking on element's path disables cut/copy icons.

Reported by: Jakub Ś Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.5.11
Component: General Version: 4.0
Keywords: IE Cc:

Description

  1. Clear editor contents.
  2. Type something inside editor.
  3. Click on body in element's path.

Result: Text is selected but Cut and Copy icons are disabled.

IE is the only browser where we can make those buttons work, so it is not a low priority issue IMO.

If you empty editor, type and then click on body in element's path, buttons get disabled from CKEditor 4.0.
If however you empty editor, type, select some text and then click on body in element's path, buttons get disabled from CKEditor 4.0.2.

Change History (23)

comment:1 Changed 3 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.6

comment:3 Changed 3 years ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

comment:4 Changed 3 years ago by Tade0

The problem here was that in IE the sequence of events when clicking on one of the buttons in the element path was:

  1. Editable loses focus.
  2. Selection is saved.
  3. Saved selection is used to determine whether the buttons should be active.

The problem here of course was that in IE the wrong, outdated selection was fed into the selectionChange event handler.

I solved this by explicitly firing the selectionChange event with the new selection.

Changes pushed to branch:t/13548.

comment:5 Changed 3 years ago by Tade0

Status: assignedreview

comment:6 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.6CKEditor 4.5.7

comment:7 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7CKEditor 4.5.8

comment:8 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:9 Changed 2 years ago by Marek Lewandowski

@t.jakut I'll ask you to review this one.

comment:10 Changed 2 years ago by Tomasz Jakut

Status: reviewreview_failed

The fix seems to be working, however there are some problems with it:

  1. There are no tests. At least a manual test should be provided for such ticket. I'd also prepare some unit test if it won't be too tricky.
  2. Because of the fix, now selectionChange event is fired twice when clicking on elementspath. I'd check if it doesn't cause any trouble.

Despite these two things, fix looks good to me.

I've rebased the branch and pushed two, tiny codestyle fixes.

comment:11 Changed 2 years ago by Tade0

Status: review_failedassigned

comment:12 Changed 2 years ago by Tade0

Status: assignedreview

Added a test here: http://tests.ckeditor.dev:1030/tests/core/command/command#tests%2Fcore%2Fcommand%2Fcommand%20test%20copy%20command%20not%20disabled%20after%20clicking%20on%20elements%20path and rebased.

Changes pushed to branch:t/13548.

As for firing selectionChange twice: I checked for trouble, found nothing so far. I think that given that there are so many selectionChange listeners and all of them have side effects if there were any trouble with firing this event twice, we would know about it by now.

Alternatively the commands could be refreshed explicitly.

comment:13 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.5.10

comment:14 Changed 2 years ago by Tomasz Jakut

Status: reviewreview_failed

The test is failing in IE11:

TypeError: Unable to get property 'checkReadOnly' of undefined or null reference

Additionally it only check "Copy" command and "Cut" one is not tested. It would be also nice to have a manual test.

I've rebased the branch:t/13548 to the newest master.

Last edited 2 years ago by Tomasz Jakut (previous) (diff)

comment:15 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.10CKEditor 4.5.11

Moving tickets to the next milestone.

comment:16 Changed 2 years ago by Tade0

There seems to be a deeper problem here, namely selection.selectElement( element ) results with a selection without ranges. Investigating.

comment:17 Changed 2 years ago by Tade0

Status: review_failedreview

The problem turned out to be embarrassingly obvious: It was impossible to create a ranged selection over a fragment that did not contain text nodes (<p><strong></strong></p>).

Fixed the test, added manual test, rebased with master.

Changes pushed to branch:t/13548.

comment:18 Changed 2 years ago by Tomasz Jakut

Status: reviewreview_failed

There is still no unit test for cut command. Except that fact, everything seems to be working fine.

comment:19 Changed 2 years ago by kkrzton

I was wondering while this fix is only for IE/Edge browsers, wouldn't it be better to wrap the explicit `selectionChange` event into if so it is only fired for IE/Edge browsers?

comment:20 in reply to:  19 Changed 2 years ago by Tade0

Status: review_failedreview

Replying to k.krzton:

I was wondering while this fix is only for IE/Edge browsers, wouldn't it be better to wrap the explicit `selectionChange` event into if so it is only fired for IE/Edge browsers?

Perhaps, but then again selectionChange is fired a lot, sometimes with effectively the same selection, so IMO it doesn't make much of a difference.

Added cut test, rebased with master, changes pushed to branch:t/13548.

comment:21 Changed 2 years ago by Tomasz Jakut

Status: reviewreview_failed

Actually good catch, @k.krzton!

@Tade0, @k.krzton is right – if the fix is only for IE, then it would be better if the event firing is wrapped in simple if. The same goes with unit tests in tests/core/command/command.js. And that should be really the last thing here.

comment:22 Changed 2 years ago by Tade0

Status: review_failedreview

Changes pushed to branch:t/13548.

comment:23 Changed 2 years ago by Tomasz Jakut

Resolution: fixed
Status: reviewclosed

LGFM. Fixed with git:24f2f3c.

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