Opened 10 years ago

Closed 10 years ago

#12858 closed Task (fixed)

[IE12/Spartan] Bring basic compatibility

Reported by: Piotrek Koszuliński Owned by: Artur Delura
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.5.0 Beta
Component: General Version:
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

NOTE

If you downloaded Windows 10 before April, then in order to test IE12 vel Spartan you need to go to about:flags and click "enable" in the "Enable Experimental Web Platform Features" section. Now IE12 is discovered as Chrome (you can verify this in dev tools - there should be "edge", not "11" in the right top corner of the dev tools panel.

You may also need to disable Compatibility View, because by default in the intranet IE starts in this mode.

NOTE2

As for now, Spartan isn't really usable so it will not be possible to bring a full compatibility with it. Dev tools are often unstable (what makes development a nightmare), weird stuff happens and new Windows updates bring rather drastic changes. Therefore, we'll focus now on the biggest changes that we are rather sure that we need to make on our side to have at least some basic level of compatibility. Further changes will be made in separate tickets, most likely after CKEditor 4.5.0.

Find this issue on GitHub

Change History (19)

comment:1 Changed 10 years ago by Jakub Ś

Status: newconfirmed
Type: BugTask

Changed 10 years ago by Piotrek Koszuliński

Attachment: ie8.png added

Changed 10 years ago by Piotrek Koszuliński

Attachment: ie12.png added

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

Description: modified (diff)

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

More info. When IE12 is recognised as Chrome it works very well. I had around 30 red tests after running 1500. However, when I forced discovering it as IE12 I started having errors preventing editor initialisation. A lot more work needs to be put into this.

comment:8 Changed 10 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:9 Changed 10 years ago by Artur Delura

Main problem in IE12 was caused because in current code features were implemented using browser detection instead of feature detection. It's quite straightforward that in new IE some old and unstandarised implementations were removed.

So far I worked out core code (all tests pass) and magicline and wysiwygarea plugin. Current changes in branch:t/12858.

comment:10 Changed 10 years ago by Artur Delura

Status: assignedreview

So far so good.

comment:11 Changed 10 years ago by Artur Delura

So far so good.

In IE12 we got compareDocumentPosition so function CKEditor.dom.element.prototype.contains use it if available. Same for getComputedStyle and CKEditor.dom.element.prototype.getComputedStyle. Improved CKEditor.dom.element.prototype.getTabIndexto be browser independent and based on this what tabIndex` property gives us.

Simplified getStyle internal function because it uses improved CKEditor.dom.element.prototype.getComputedStyle function.

runtimeStyle property has been removed in IE12, so don't rely on it.


Lessons learned:

  • Future detection over browser detection.
  • If something is not a standard but browser specific implementation we should take into consideration that in the future version it might be removed:
    if ( ie ) {
        console.log( element.runtimeStyle );
    }

In future IE it won't work.


Things to be done:

Version 1, edited 10 years ago by Artur Delura (previous) (next) (diff)

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

Status: reviewreview_failed

I pushed branch:t/12858 with one additional commit.

My reds right now:

tests/core/dom/element/element Core editor, unit, dom   115 passed / 1 failed / 1 ignored in 1173ms   
tests/core/focusmanager/editor Core editor, unit   5 passed / 1 failed / 0 ignored in 955ms   
tests/plugins/clipboard/pastedialog Plugins editor, unit, dialog    3 passed / 1 failed / 0 ignored in 829ms   
tests/plugins/find/find Plugins editor, unit    1 passed / 2 failed / 0 ignored in 849ms   
tests/plugins/font/font Plugins     2 passed / 10 failed / 0 ignored in 60987ms   
tests/plugins/image2/editing Plugins editor, unit, widget    5 passed / 1 failed / 0 ignored in 2693ms   
tests/plugins/justify/justify Plugins editor, unit   7 passed / 1 failed / 0 ignored in 1524ms   
tests/plugins/magicline/magicline Plugins editor, unit, magicline   27 passed / 1 failed / 9 ignored in 3504ms   
tests/plugins/widget/widgetselection Plugins editor, unit, widgetcore    7 passed / 3 failed / 0 ignored in 31188ms   
tests/plugins/xml/xml Plugins editor, unit   3 passed / 1 failed / 0 ignored in 49ms  

From all these tests only one is important - that in the http://tests.ckeditor.dev:1030/tests/core/dom/element/element. The method that fails is getDocumentPosition() and it's perfectly visible when doing manual tests because all menus and floating toolbars are mispositioned. This is must have to claim that the support level is ok to close this ticket.

Besides this, the changes that you done, Artur, broke IE8 totally. You must be careful when doing feature detection.

comment:13 Changed 10 years ago by Artur Delura

Status: review_failedassigned

comment:14 Changed 10 years ago by Artur Delura

Status: assignedreview

Little omission, broken everything. Fixed element.getDocumentPosition to behave like in webkit.

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

Description: modified (diff)
Summary: [IE12] Check compatibility[IE12/Spartan] Bring basic compatibility

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

I reinstalled Windows 10 (I wasn't sure if it fully updates right now, so I wanted to be sure that I test the latest version) and I checked the branch:t/12858 again. Now Spartan seems to be more slightly more stable, but it's still far from being ready IMO. Also, it gave me different tests results (compare with comment:12):

tests/core/config/inline Core editor, unit   1 passed / 1 failed / 0 ignored in 183ms   
tests/core/dom/node Core editor, unit, dom   72 passed / 4 failed / 0 ignored in 287ms   
tests/core/dom/range/bookmarks Core editor, unit, dom, range   116 passed / 4 failed / 0 ignored in 509ms   
tests/core/editable/wysiwyg Core editor, unit    5 passed / 1 failed / 0 ignored in 10131ms   
tests/core/focusmanager/editor Core editor, unit   5 passed / 1 failed / 0 ignored in 721ms   
tests/core/selection/selection Core editor, unit   24 passed / 6 failed / 1 ignored in 698ms   
tests/plugins/find/find Plugins editor, unit    2 passed / 1 failed / 0 ignored in 849ms   
tests/plugins/floatingspace/floatingspace Plugins editor, unit   0 passed / 1 failed / 0 ignored in 107ms   
tests/plugins/font/font Plugins     2 passed / 10 failed / 0 ignored in 60950ms   
tests/plugins/image2/link Plugins editor, unit, widget    37 passed / 2 failed / 0 ignored in 7840ms   
tests/plugins/justify/justify Plugins editor, unit   7 passed / 1 failed / 0 ignored in 1138ms   
tests/plugins/link/link Plugins editor, unit   15 passed / 1 failed / 0 ignored in 2520ms
tests/plugins/sourcearea/source Plugins editor, unit   2 passed / 1 failed / 0 ignored in 126ms   
tests/plugins/tab/tab Plugins editor, unit   1 passed / 1 failed / 0 ignored in 216ms   
tests/plugins/xml/xml Plugins editor, unit   3 passed / 1 failed / 0 ignored in 42ms   
tests/tickets/11500/1 Tickets editor, unit   1 passed / 1 failed / 0 ignored in 184ms  

Some tests that were red are now green, but some became red. The most worrisome problems right now are:

We will try to address these problems before CKEditor 4.5.0, although, I'm not sure what to do with the first one because it looks like something is broken in the engine.

Other than that and some random "Unspecified errors" and Spartan crashes, the editor works rather well.

Last edited 10 years ago by Piotrek Koszuliński (previous) (diff)

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

We were curious whether it will be better to recognise Spartan as a next version of IE or as a new browser. I force env.ie to be false and ran some of the tests. Results are not terrible, but not good too. Obviously, far more tests fail, because Spartan is not considered in them. I thought that perhaps manual tests will give better results, but no. Editor works better when Spartan is recognised as IE12. Let's stick to that because this way seems to be far shorter.

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

Description: modified (diff)

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

I fixed this:

https://twitter.com/reinmarpl/status/585165599371894784 what breaks ​https://github.com/ckeditor/ckeditor-dev/blob/master/core/editable.js#L1103-L1109,

And now the tests results are better:

tests/core/dom/node Core editor, unit, dom   72 passed / 4 failed / 0 ignored in 389ms   
tests/core/dom/range/bookmarks Core editor, unit, dom, range   116 passed / 4 failed / 0 ignored in 715ms   
tests/core/focusmanager/editor Core editor, unit   5 passed / 1 failed / 0 ignored in 911ms   
tests/core/selection/selection Core editor, unit   24 passed / 6 failed / 1 ignored in 794ms   
tests/plugins/find/find Plugins editor, unit    2 passed / 1 failed / 0 ignored in 771ms   
tests/plugins/font/font Plugins     2 passed / 10 failed / 0 ignored in 60737ms   
tests/plugins/image2/link Plugins editor, unit, widget    37 passed / 2 failed / 0 ignored in 9572ms   
tests/plugins/link/link Plugins editor, unit   15 passed / 1 failed / 0 ignored in 3201ms
tests/plugins/tab/tab Plugins editor, unit   1 passed / 1 failed / 0 ignored in 217ms   
tests/plugins/xml/xml Plugins editor, unit   3 passed / 1 failed / 0 ignored in 38ms  

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

Resolution: fixed
Status: reviewclosed

Fixed on major with git:3263734.

I reported #13142 and #13143 for issues that I mentioned in comment:16.

Find this issue on GitHub
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