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

[Piotrek Koszuliński]

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

Additionally, env.isCompatible should be updated.

[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?

[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.

[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.

[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.

[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.

[Piotrek Koszuliński]

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

[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.

[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.

[Piotr Jasiun]

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

[Piotrek Koszuliński]

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 ) ?

3. 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;

5. And this too:

-        if ( CKEDITOR.env.ie && CKEDITOR.env.version < 8 ) {
+        if ( CKEDITOR.env.ie && CKEDITOR.env.quirks ) {

6. 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;

7. 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 ) ) {

8. Changes in /plugins/templates/dialogs/templates.css are incorrect because QM uses them.

9. dialog_ie7.css files can be removed and skin.ua_dialog may be shortened even further in both skins.

[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.

[Piotr Jasiun]

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.

[Piotrek Koszuliński]

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

[Piotr Jasiun]

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.

[Piotrek Koszuliński]

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

[Piotrek Koszuliński]

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.

[Piotr Jasiun]

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

[Piotr Jasiun]

• ​git:a841566
• tests:1d382b2