Opened 5 years ago

Closed 5 years ago

#11422 closed Task (fixed)

FF3.x, IE6-7, Opera12.x leftovers in code

Reported by: Piotrek Koszuliński Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.4.0
Component: General Version:
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

We dropped support for all these browsers. Time to clean our code base.

Additionally, env.isCompatible should be updated.

Change History (24)

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

Description: modified (diff)

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

Summary: FF3.x, IE6-7, Opera leftovers in codeFF3.x, IE6-7, Opera12.x leftovers in code

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

Description: modified (diff)
Status: newconfirmed

comment:4 Changed 5 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:5 Changed 5 years ago by Piotr Jasiun

What do you think about removing env.ie9Compat, env.ie8Compat, env.ie7Compat and env.ie6Compat. These are deprecated since CKE 4.0 (at least).

What with env.opera? It won't be useful anymore so it could be removed, marked as deprecated or replaced with new regexp checking if navigator.userAgent contains OPR/.

And what we do with env.air? Should I remove AIR leftovers as well?

comment:6 Changed 5 years ago by Piotr Jasiun

I removed almost all FF2 and FF3 leftovers. The last place where .cke_browser_gecko18 appears is compressed css in tt/4463 in test project. Since this test does not work I will not modify it. IMO it should be removed.

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

Better to leave ieXCompat flags. Even our code still uses them and will use unless we totally remove QM support and cleanup the rest of code what's a subject for separate ticket.

CKEDITOR.env.opera can be removed.

comment:8 Changed 5 years ago by Piotr Jasiun

I removed all hacks for Opera and env.opera itself. I found that we had 5 times something like this:

// TODO: Check if really needed for Gecko+Mac.
if ( CKEDITOR.env.opera || ( CKEDITOR.env.gecko && CKEDITOR.env.mac ) )
	template += ' onkeypress="return false;"';

Now we have:

// TODO: Check if really needed.
if ( CKEDITOR.env.gecko && CKEDITOR.env.mac )
	template += ' onkeypress="return false;"';

Someone with Mac should check if we still need these hacks.

Changes in t/11422 and corresponding test branch.

comment:9 Changed 5 years ago by Piotr Jasiun

A finished and pushed IE cleanup, so this task is basically done.

Because we dropped support for IE6 & IE7 but we still support IE QM we have a lot of checking if browser is in IE QM and this checking was implementend in multipile ways (unsing env.ie && env.quirks or env.ie6Compat or document.documentMode). So I introduced new variable: env.ieQuirks and replace all of the above.

I'm not sure if there is a point in keeping env.quirks == true for non-IE browsers. Maybe env.quirks should be true only in IE 8 and IE 9 QM (as env.ieQuirks works now).

I also created ticked for ieXCompat: #11473.

Last edited 5 years ago by Piotr Jasiun (previous) (diff)

comment:10 Changed 5 years ago by Piotr Jasiun

Status: assignedreview

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

Status: reviewreview_failed

This ticket was about cleaning up dropped browsers code. Not about cleaning up env.quirks and dozens of places where it is used.

comment:12 Changed 5 years ago by Piotr Jasiun

The problem with quirks appeared after removing IE 6 & IE 7 support, because most of hacks now apply only to QM (instead of IE 7, IE 6 and IE QM). I think that it would be misleading if we keep if ( env.ie6Compat ) in our code only because of QM (env.ie6Compat is true in QM) and it is better to replace it with if ( env.ieQuirks ). Also after clean up we can save some bites (compressed ckeditor is 322 bits smaller with env.ieQuirks).

Anyway I introduced env.ieQuirks in separate commit so it could be easily removed.

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

Thanks for clarification why you did this change - it of course makes sense. But it doesn't mean that it has to be done in unrelated ticket. So please extract these changes to separate ticket and let's discuss whether we want to make them now.

comment:14 Changed 5 years ago by Piotr Jasiun

Status: review_failedreview

Ok, I created a ticket for this (#11479) and move changes there.

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

Status: reviewreview_failed
  1. Unnecessary bracket: if ( !force && !( CKEDITOR.env.needsBrFiller ) )
  2. This has not been a correct change because env.version is not affected by quirks:
-        return ( CKEDITOR.env.ie && CKEDITOR.env.version < 8 ) ?
+        return ( CKEDITOR.env.ie && CKEDITOR.env.quirks ) ?
  1. This is incorrect change for the same reason:
-            env.cssClass += ' cke_browser_ie' + ( env.quirks || env.version < 7 ? '6' : env.version );
+            env.cssClass += ' cke_browser_ie' + env.version;
  1. And this too:
-        if ( CKEDITOR.env.ie && CKEDITOR.env.version < 8 ) {
+        if ( CKEDITOR.env.ie && CKEDITOR.env.quirks ) {
  1. You can simplify this even more:
-                if ( CKEDITOR.env.webkit || CKEDITOR.env.opera )
+                if ( CKEDITOR.env.webkit )
                     CKEDITOR.document[ CKEDITOR.env.webkit ? 'getBody' : 'getDocumentElement' ]().$.scrollTop = scrollTop;
  1. This is wrong too:
-                        var focusClass = me.type in { 'checkbox': 1, 'ratio': 1 } && CKEDITOR.env.ie && CKEDITOR.env.version < 8 ? 'cke_dialog_ui_focused' : '';
+                        var focusClass = me.type in { 'checkbox': 1, 'ratio': 1 } && CKEDITOR.env.ie && CKEDITOR.env.quirks ? 'cke_dialog_ui_focused' : '';
-                if ( startBlockTag == 'pre' && CKEDITOR.env.ie && CKEDITOR.env.version < 8 )
+                if ( startBlockTag == 'pre' && CKEDITOR.env.ie && CKEDITOR.env.quirks )
-                element = CKEDITOR.env.ie && !( CKEDITOR.document.$.documentMode >= 8 ) ? editor.document.createElement( '<input name="' + CKEDITOR.tools.htmlEncode( name ) + '">' ) : editor.document.createElement(
+                element = CKEDITOR.env.ie && CKEDITOR.env.quirks ? editor.document.createElement( '<input name="' + CKEDITOR.tools.htmlEncode( name ) + '">' ) : editor.document.createElement( 'input' );
-    emptyAnchorFix: CKEDITOR.env.ie && CKEDITOR.env.version < 8,
+    emptyAnchorFix: CKEDITOR.env.ie && CKEDITOR.env.quirks,
-            if ( !CKEDITOR.env.ie || CKEDITOR.env.version > 7 ) {
+            if ( !( CKEDITOR.env.ie && CKEDITOR.env.quirks ) ) {
  1. Changes in /plugins/templates/dialogs/templates.css are incorrect because QM uses them.
  1. dialog_ie7.css files can be removed and skin.ua_dialog may be shortened even further in both skins.

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

BTW. I have to say that I'm surprised by how many lines could be removed. Together with #11377 we'll save few kbytes.

comment:17 Changed 5 years ago by Piotr Jasiun

Status: review_failedreview

I fixed these issues and rebased branch. We decided to remove cke_browser_ie6 so now I QM editor will get only cke_browser_iequirks class instead of both cke_browser_ie6 cke_browser_iequirks. Changes are I separate commits to make review easier. After review I will join them with previous commits.

Last edited 5 years ago by Piotr Jasiun (previous) (diff)

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

Status: reviewreview_failed

There's something wrong with that branch. It contains git:54cbac5 which looks as totally unrelated (and wrong).

comment:19 Changed 5 years ago by Piotr Jasiun

Status: review_failedreview

This commit makes sense here, but in the context of whole branch (with previous changes). I rebased and squashed commits to avoid such confusing commits. Also I rebased branch onto newest major, after merge master into it.

Last edited 5 years ago by Piotr Jasiun (previous) (diff)

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

Status: reviewreview_passed

Please wait with merging to major for confirmation regarding IE7 related changes.

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

Status: review_passedreview_failed

We made a decision to keep all IE7 related hacks and isCompatible==true when version==7. This way we'll keep current, decent CKEditor support for IE7 and IE compatibility mode. However, this is "unofficial" support, so the goal is not to improve it in the future, but to not lose it.

comment:22 Changed 5 years ago by Piotr Jasiun

Status: review_failedreview

I removed IE7 changes and pushed to: t/11422b

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

Status: reviewreview_passed

comment:24 Changed 5 years ago by Piotr Jasiun

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