Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#13867 closed Bug (fixed)

CKEditor fails to load on IE8/9 when a polyfill for classList is included

Reported by: sam Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.5.5
Component: General Version: 4.5.4
Keywords: Drupal Cc: theodore@…, wim.leers@…

Description

Steps to reproduce

  1. load a classList polyfill onto the page with polyfill from https://developer.mozilla.org/en/docs/Web/API/Element/classList
  2. load ckeditor onto the same page after the polyfill

Expected result

Everything should work nice and fine.

Actual result

There will be errors on the page about this.$.classList is undefined for elements within the text-editor iframe $(".cke_wysiwyg_frame .cke_reset").

All the features will not work except the very basic textbox.

Other details (browser, OS, CKEditor version, installed

My environment was IE8 on windows 7, but I believe this can happen on ie9 as well. The ckeditor version was 4.5.4.

My obseravtion is that the elements within the iframe need to also detect if classList is available, instead of doing the detection just once during page load.

I traced to this commit. https://github.com/ckeditor/ckeditor-dev/commit/0836608

Change History (16)

comment:1 Changed 8 years ago by Théodore Biadala

Cc: theodore@… added
Keywords: Drupal added

I can confirm that. I'm having difficulties on my IE8 module for Drupal 8, since we add a classList polyfill by default.

Can reproduce on XP-IE8.

comment:3 Changed 8 years ago by Wim Leers

Cc: wim.leers@… added

Reported against Drupal 8 too: https://www.drupal.org/node/2607362

comment:4 Changed 8 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5
Status: newconfirmed

Let's try to solve it in 4.5.5.

comment:5 Changed 8 years ago by Wim Leers

Summary: Ckeditor on IE8/9 fail to load when a polyfill for classList is includedCKEditor fails to load on IE8/9 when a polyfill for classList is included

RE: milestone=4.5.5: perfect, that matches what I wrote/expected in https://www.drupal.org/node/2321583#comment-10527008.

(Also cleaned up the title.)

comment:6 Changed 8 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:7 Changed 8 years ago by Marek Lewandowski

Status: assignedreview

comment:8 Changed 8 years ago by Tomasz Jakut

Status: reviewreview_failed

The fix is not working correctly. Note that IE8 throws 'HTMLElement' is undefined as TypeError. Maybe we should check not only the type of exception, but also the message? Or just rewrite the check to first check if HTMLElement is defined before trying to access its properties.

comment:9 Changed 8 years ago by Marek Lewandowski

Status: review_failedreview

Correct! Additionally descriptors added to IE8 by the polyfill returns typeof unknown which made this approach more complicated.

Instead here is an alternative, and way simpler approach.

comment:10 Changed 8 years ago by Tomasz Jakut

Status: reviewreview_passed

Now everything is OK, good job!

comment:11 Changed 8 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

Fixed with git:a8460e1271 (merged to master).

comment:12 Changed 8 years ago by Olek Nowodziński

(!!!) First of all, an external library in CKEditor project requires an entry in LICENSE.md.

Secondly, I hardly find it necessary because a mockup of the classList library would be enough to prove that https://github.com/ckeditor/ckeditor-dev/commit/a8460e1271#diff-19bd701718906bfcb56a997c506cc452R158 check works correctly.

comment:13 Changed 8 years ago by Wim Leers

+1 to those concerns.

comment:14 Changed 8 years ago by Marek Lewandowski

@a.nowodzinski right! I'll put proper commit today / tomorrow.

comment:15 Changed 8 years ago by Marek Lewandowski

Replaced original snippet with a stubbed object on branch branch:t/13867c. Also I was able to create unit test to keep us safe from regressions. @a.nowodzinski take a look on it to see if you can see any room for improvement. If not we'll merge it to master.

comment:16 Changed 8 years ago by Marek Lewandowski

Branch branch:t/13867c was merged with git:1440de05.

Last edited 8 years ago by Marek Lewandowski (previous) (diff)
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