Opened 6 years ago

Closed 6 years ago

#6187 closed Bug (fixed)

IE6 skin images 404

Reported by: NikoViitala Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.4.2
Component: UI : Skins Version:
Keywords: Cc:

Description (last modified by alfonsoml)

IE6 (SP2) tries to load skin images two times. In the other time, it tries to search images in wrong path "/js/ckeditor/skins/kama/http://URL/js/ckeditor/skins/kama/icons.png" etc, so those images are redirect to 404. The same seems to happen in skin v2 too. IE > 6 and other browsers doesn't have the bug.

Error log:

"GET /js/ckeditor/config.js?t=A7HG4HT HTTP/1.1" 200 1289
"GET /js/ckeditor/skins/kama/images/sprites_ie6.png?t=A7HG4HT HTTP/1.1" 304
"GET /js/ckeditor/skins/kama/icons.png?t=A7HG4HT HTTP/1.1" 304 304
"GET /js/ckeditor/skins/kama/images/dialog_sides.gif?t=A7HG4HT HTTP/1.1" 304
"GET /js/ckeditor/skins/kama/https://www.xxx.fi/js/ckeditor/skins/kama/icons.png?t=A7HG4HT&t=A7HG4HT HTTP/1.1" 404
"GET /js/ckeditor/skins/kama/https://www.xxx.fi/js/ckeditor/skins/kama/images/sprites_ie6.png?t=A7HG4HT&t=A7HG4HT HTTP/1.1" 404
"GET /js/ckeditor/skins/kama/https://www.xxx.fi/js/ckeditor/skins/kama/images/dialog_sides.gif?t=A7HG4HT&t=A7HG4HT HTTP/1.1" 404
"GET /js/ckeditor/skins/kama/https://www.xxx.fi/js/ckeditor/skins/kama/https://www.tyokyvyntuki.fi/js/ckeditor/skins/kama/icons.png?t=A7HG4HT&t=A7HG4HT&t=A7HG4HT HTTP/1.1" 404

Suggested fix: I deleted these lines (in compressed ckeditor.js or in skin.js) and it's working fine again. Maybe a better approach would be to fix preload function?:

        if ( CKEDITOR.env.ie && CKEDITOR.env.version < 7 )
        {
                // For IE6, we need to preload some images, otherwhise they will be
                // downloaded several times (CSS background bug).
                preload.push( 'icons.png', 'images/sprites_ie6.png', 'images/dialog_sides.gif' );
        }

Attachments (4)

6187.patch (582 bytes) - added by tobiasz.cudnik 6 years ago.
httpd.log (38.6 KB) - added by tobiasz.cudnik 6 years ago.
6187_2.patch (3.7 KB) - added by tobiasz.cudnik 6 years ago.
6187_3.patch (5.5 KB) - added by tobiasz.cudnik 6 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 6 years ago by alfonsoml

  • Description modified (diff)
  • Status changed from new to confirmed

I don't see a 404, but two images are loaded twice: one with the timestamp and the second one without it.

http://nightly.ckeditor.com/5829/_samples/replacebyclass.html
http://nightly.ckeditor.com/5829/ckeditor.js
http://nightly.ckeditor.com/5829/_samples/sample.js
http://nightly.ckeditor.com/5829/config.js?t=A7M74HW
http://nightly.ckeditor.com/5829/skins/kama/icons.png?t=A7M74HW
http://nightly.ckeditor.com/5829/skins/kama/images/sprites_ie6.png?t=A7M74HW
http://nightly.ckeditor.com/5829/skins/kama/images/dialog_sides.gif?t=A7M74HW
http://nightly.ckeditor.com/5829/skins/kama/editor.css?t=A7M74HW
http://nightly.ckeditor.com/5829/lang/es.js?t=A7M74HW
http://nightly.ckeditor.com/5829/skins/kama/icons.png
http://nightly.ckeditor.com/5829/skins/kama/images/sprites_ie6.png
http://nightly.ckeditor.com/5829/contents.css
http://nightly.ckeditor.com/5829/plugins/styles/styles/default.js?t=A7M74HW&t=A7M74HW

So we should certainly look at the reason of this problem.
I guess that the ones without the timestamp are loaded due to the stylesheet references.
And maybe the patch to avoid zoom problems in IE can expand this issue to all IE versions.

comment:2 Changed 6 years ago by fredck

  • Milestone set to CKEditor 3.4.1

comment:3 Changed 6 years ago by gillup

I have the same problem and maybe I can help in term of context.

I have a page with multiple CKEditor windows.

The 404 occurs only when the Setting of Temporary Internet Files is "Verify on Each Visit" (translated from French).

It's clear that the URL's construction is at fault. Something should be cleared before the url to request is sent.

The 404 are like that : http://myurl/ckeditor/skins/kama/'''targetedRessource?'''t=timestamp http://myurl/ckeditor/skins/kama/'''''http://myurl/ckeditor/skins/kama/'''targetedRessource?'''t=timestamp'''&''t=timestamp http://myurl/ckeditor/skins/kama/''http://myurl/ckeditor/skins/kama/'''''http://myurl/ckeditor/skins/kama/'''targetedRessource?'''t=timestamp'''&''t=timestamp''&t=timestamp

It seems to me that in constructing the url, each CKEditor instance uses a variable from the previous instance (or a global, shared CKEDITOR variable ?) without clearing it.

comment:4 Changed 6 years ago by gillup

Sorry for the formatting mixup, what I meant was :

myurl/ckeditor/skins/kama/targetedRessource?t=timestamp
myurl/ckeditor/skins/kama/myurl/ckeditor/skins/kama/targetedRessource?t=timestamp&t=timestamp
myurl/ckeditor/skins/kama/myurl/ckeditor/skins/kama/myurl/ckeditor/skins/kama/targetedRessource?t=timestamp&t=timestamp&t=timestamp

or roughly : URL (1st instance) URL/URL (2nd instance) URL/URL/URL (3rd instance...)

comment:5 Changed 6 years ago by gillup

skin.js line34:

		var appendSkinPath = function( fileNames )
		{
			for ( var n = 0 ; n < fileNames.length ; n++ )
			{
				fileNames[ n ] = CKEDITOR.getUrl( paths[ skinName ] + fileNames[ n ] );
			}
		};

It seems that each instance execute this method. Multiple executions causes filename[n] to grow with an added paths[skinName] each time.

comment:6 Changed 6 years ago by gillup

In core/skins.js line 58 (V3.4.0) :

Check if preloaded if not then

preload (uses appendSkinPath) preloaded = 1

end if

Maybe while an instance is preloading, another instance comes around and because the preloading isn't finished and preloaded still undefined...

I tested with preloaded = 1 before executing the preload (thus prohibiting another instance from executing preload/appendSkinPath) -> no more 404.

comment:7 Changed 6 years ago by tobiasz.cudnik

  • Owner set to tobiasz.cudnik
  • Status changed from confirmed to assigned

Changed 6 years ago by tobiasz.cudnik

comment:8 Changed 6 years ago by tobiasz.cudnik

After investigating the issue i can tell following things:

  • Preloader's lock is badly designed leaving time hole, which results in duplicate preloading of same images (and each preload prepends URL)
  • Preloading doesn't resolve IE6 cache issue as we're still having dozens of 304 for icons.png etc (check attached httpd.log).
  • Issue is randomly reproducible on source and always on the release using a page with multiple instances with same skin.
  • Double request to icons.png (with timestamp from JS and without it from CSS) should be fixed using CKReleaser, which should update filename with current timestamp during release.

I'm attaching the patch which solves 404s, although it could result in using image before it will be preloaded, but as i've pointed out preloading doesn't really work now.

I will be preparing next patches to fix the issue, probably by extending CKEDITOR.imageCacher with event system allowing multiple sources to bind to an preload-complete event.

Changed 6 years ago by tobiasz.cudnik

Changed 6 years ago by tobiasz.cudnik

comment:9 Changed 6 years ago by tobiasz.cudnik

  • Status changed from assigned to review
  • Version 3.4.1 deleted

This patch should keep preloading tight, without any time holes. I've also fixed 304s by enabling IE6 image background caching (which was commented out in our source).

There is #6347 for double request to image files because of timestamp.

comment:10 follow-up: Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

The patch is good, but if "document.execCommand( 'BackgroundImageCache', false, true )" resolves the ie6 image request problem, why should we still keep skin preload?

Changed 6 years ago by tobiasz.cudnik

comment:11 in reply to: ↑ 10 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

Replying to garry.yao:

The patch is good, but if "document.execCommand( 'BackgroundImageCache', false, true )" resolves the ie6 image request problem, why should we still keep skin preload?

Well, it's a good question. We don't need it anymore for IE6. Although i don't think we should loose this feature completely, as it might be useful in the future.

comment:12 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

As a decision from a group call, R+ for 6187_2.patch while we hold the removal of preloader with #6502.

comment:13 Changed 6 years ago by garry.yao

  • Owner changed from tobiasz.cudnik to garry.yao
  • Status changed from review_failed to review

comment:14 Changed 6 years ago by garry.yao

  • Status changed from review to review_passed

comment:15 Changed 6 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [5982].

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