Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#8602 closed Bug (fixed)

Unnecessary calls to images without baseHref

Reported by: pm_ele Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.6.3
Component: General Version:
Keywords: HasPatch IE Cc:

Description

When displaying images using relative urls and config.baseHref, everytime I switch to wysiwyg mode there is a call made to the image using only the relative url and totally ignoring the baseHref. Obviously this fails into a 404 (or some other error code depending on the environnement). The image is still displayed, but there is also this unnecessary 404 call. Please note also that if you're testing this on local static files (i mean something like C:/some_directories/mypage.html) the call won't show up on Firebug, you will need to run it on a server like wamp.

You can reproduce it easily on the online demo with Firebug :

  • go to http://ckeditor.com/demo
  • go to source mode
  • remove all source and paste only the following :
     <img src="logo3w.png"/>
    
  • type in Firebug console :
    CKEDITOR.instances.editor1.config.baseHref = 'http://www.google.fr/images/srpr/';
    
  • clear all firebug console and network panel
  • hit the wysiwyg mode

you should see the Google logo, and a 404 showing up in both console et network panels.

I've located the where the call is made, it's in plugins/htmldataprocessor/plugin.js, in the toHtml function :

    // Call the browser to help us fixing a possibly invalid HTML
    // structure.
    var div = new CKEDITOR.dom.element( 'div' );
    // Add fake character to workaround IE comments bug. (#3801)
    div.setHtml( 'a' + data );
    data = div.getHtml().substr( 1 );

The data is submitted to the browser, causing it to interpret the <img> tags without taking the config.baseHref into account.

I've found a solution by using a regexp on the data : I turn all the src attributes into data attributes and back just before and after submitting the data to the browser. I'm not sure it's best way but I'm using it on the ckeditor on my website and it works fine. I can't make a patch file but here's how I've altered the code :

    //Call the browser to help us fixing a possibly invalid HTML
    // structure.        
    var fakeSrcAttribute = 'data-cke-faked-src';
    data = data.replace(/(<img[^>]+)src(\s*=)/ig, '$1' + fakeSrcAttribute + '$2');        
    var div = new CKEDITOR.dom.element( 'div' );
    // Add fake character to workaround IE comments bug. (#3801)
    div.setHtml( 'a' + data );
    data = div.getHtml().substr( 1 );
    data = data.replace(new RegExp('(<img[^>]+)' + fakeSrcAttribute + '(\\s*=)', 'ig'), '$1src$2');

Attachments (2)

8602.patch (8.3 KB) - added by Frederico Caldeira Knabben 5 years ago.
8602_2.patch (8.8 KB) - added by Frederico Caldeira Knabben 5 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by pm_ele

Keywords: IE7 added

I just noticed : the fix I'm suggesting also solves much bigger bug I had in IE7.

In IE7, images with relatives urls and baseHref were simply not displayed. It seems that submitting the data to the browser caused IE7 to try to replace the relative urls in src attributes by incorrect absolute urls. The suggested fix solves that bug too.

comment:2 Changed 5 years ago by Jakub Ś

Keywords: HasPatch added; baseHref 404 images IE7 removed
Status: newconfirmed

This part about 404 is actually a duplicate of #6368. Yet I will not close it as it contains potential fix to perhaps two issues.

@pm_ele Could you explain me this part with IE7? I have seen many tickets about IE7, baseHref and relative URL but could not reproduce any.

Take this URL: /intl/en_ALL/images/logo.gif and this baseHref config.baseHref = 'http://www.google.com/intl/en_ALL/images/'; for example. They work.

@pm_ele is it possible to reproduce this issue in CKEditor, without any webapp? If so could you provide us with scenario to do so?

comment:3 Changed 5 years ago by dan seto

Awesome fix

comment:4 Changed 5 years ago by Jakub Ś

@abcito just like I have written in #5042.

Does this bug occur in plain CKEditor downloaded form our site or only when included in your webapp?

Any chance for a TC here?

comment:5 Changed 5 years ago by Frederico Caldeira Knabben

Keywords: IE added

I'm able to reproduce this issue with IE9+Compat. It has nothing to do with #6368.

Changed 5 years ago by Frederico Caldeira Knabben

Attachment: 8602.patch added

comment:6 Changed 5 years ago by Frederico Caldeira Knabben

Owner: set to Frederico Caldeira Knabben
Status: confirmedreview

The suggested fix makes sense, but it can be done in a a single pass.

The attached patch includes also a minor new feature. Note that the numbers on CKEDITOR.rdn will be optimized by the packager.

comment:7 Changed 5 years ago by Garry Yao

Status: reviewreview_failed

If this patch is to be considered, [7379] could be reverted.

Changed 5 years ago by Frederico Caldeira Knabben

Attachment: 8602_2.patch added

comment:8 Changed 5 years ago by Frederico Caldeira Knabben

Status: review_failedreview

True... killing two birds with one stone so ;)

comment:9 Changed 5 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.3

comment:10 Changed 5 years ago by Garry Yao

Status: reviewreview_passed

comment:11 Changed 5 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Fixed with [7413].

comment:12 Changed 5 years ago by Alfonso Martínez de Lizarrondo

Just a minor note: the example says alert( CKEDITOR.rnd ); but the used name is "rdn" instead.

Personally, I think that "rnd" might be better because rdn isn't an abbreviation of random.

comment:13 in reply to:  12 Changed 5 years ago by Frederico Caldeira Knabben

Replying to alfonsoml:

Nice observation. In fact, the example showed what I wanted to have, but I've spread the typo everywhere.

Renamed with [7417].

comment:14 Changed 5 years ago by Garry Yao

http://ckeditor.t/tt/4475/1.html The above tt is regressed, post fixed with [7418].

comment:15 Changed 5 years ago by dan seto

Hmm applied the patch and I have an image

<img alt="masthead" src="static/assets/banner900x120.gif" style="width:900px; height:120px;" /> (in source)

toggled to WYSIWYG then back to source and I get this

<img alt="mastheadsrc=&quot;static/assets/banner900x120.gif&quot;" src="static/assets/banner900x120.gif" style="width: 900px; height: 120px" />

thoughts?

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