Opened 6 years ago

Closed 4 years ago

#6666 closed Bug (fixed)

Do not use element.all property (CKEditor entirely broken in latest Opera)

Reported by: tony Owned by: fredck
Priority: Normal Milestone: CKEditor 3.6.3
Component: General Version: 3.4.2
Keywords: Opera Cc: hallvord@…, garryyao

Description

The editor demo at http://ckeditor.com/demo will not load in Opera. I am using Opera version 11.00 build 1060 available from http://my.opera.com/desktopteam/blog/ and CKEditor 3.4.2 (revision 6041.)

The error in Opera is :

JavaScript - http://ckeditor.com/demo
Timeout thread: delay 10 ms
Uncaught exception: TypeError: Cannot convert 'i.all' to object
Error thrown at line 18, column 3309 in <anonymous function: unselectable>() in http://ckeditor.com/apps/ckeditor/3.4.2/ckeditor.js?1289422309:
    while(j=i.all[k++])
called from line 136, column 0 in <anonymous function: build>(n, o) in http://ckeditor.com/apps/ckeditor/3.4.2/ckeditor.js?1289422309:
    B.getChild([1,0,0,0,0]).unselectable();
called from line 24, column 1776 in <anonymous function>() in http://ckeditor.com/apps/ckeditor/3.4.2/ckeditor.js?1289422309:
    z.build(x);
called from line 22, column 1760 in <anonymous function: load>(u, v) in http://ckeditor.com/apps/ckeditor/3.4.2/ckeditor.js?1289422309:
    k.call(l,q);
called via Function.prototype.call() from line 21, column 2595 in <anonymous function: load>(z) in http://ckeditor.com/apps/ckeditor/3.4.2/ckeditor.js?1289422309:
    m.call(n,s,t);
called via Function.prototype.call() from line 21, column 2528 in <anonymous function: load>(l, m, n, o, p) in http://ckeditor.com/apps/ckeditor/3.4.2/ckeditor.js?1289422309:
    u(true);
called from line 22, column 1216 in <anonymous function: load>(j, k, l) in http://ckeditor.com/apps/ckeditor/3.4.2/ckeditor.js?1289422309:
    a.scriptLoader.load(o,function(u,v){if(v.length)throw '[CKEDITOR.resourceManager.load] Resource name "'+p[v[0]].join(',')+'" was not found at "'+v[0]+'".';for(var w=0;w<u.length;w++){var x=p[u[w]];for(var y=0;y<x.length;y++){var z=x[y];q[z]=this.get(z);m[z]=1;}}k.call(l,q);},this);
called from line 24, column 1643 in <anonymous function>(x) in http://ckeditor.com/apps/ckeditor/3.4.2/ckeditor.js?1289422309:
    a.themes.load(y,function(){var z=x.theme=a.themes.get(y);z.path=a.themes.getPath(y);z.build(x);if(x.config.autoUpdateElement)v(x);});
called from line 24, column 1245 in <anonymous function>() in http://ckeditor.com/apps/ckeditor/3.4.2/ckeditor.js?1289422309:
    u(x);
called from line 21, column 2595 in <anonymous function: load>(z) in http://ckeditor.com/apps/ckeditor/3.4.2/ckeditor.js?1289422309:
    m.call(n,s,t);

I have attached a screen shot of the problem.

Attachments (3)

snapshot.png (71.8 KB) - added by tony 6 years ago.
Screen shot
6666.patch (646 bytes) - added by Ms2ger 5 years ago.
Use getElementsByTagName
6666_2.patch (1.5 KB) - added by fredck 5 years ago.

Download all attachments as: .zip

Change History (18)

Changed 6 years ago by tony

Screen shot

comment:1 Changed 6 years ago by fredck

  • Cc hallvord@… added

I've tested Opera builds 1026, 1055 and 1060 with the online demo, nightly builds and local trunk copies. All is working well.

Maybe Hallvord can give is a final confirmation.

comment:2 Changed 6 years ago by tony

Fred, are you using the windows version? I suspect maybe a bug in opera's js rendering now if you're not seeing it but if I can narrow it down to just the Linux version, then I've got extra info I can report to the Opera people.

I've tried several times now, clearing cache, closing and then opening the browser, nightly demos, yet still the same results. CKEditor is not showing for me. :(

comment:3 Changed 6 years ago by hallvord@…

This is because we've experimentally removed element.all support in browser.js: http://my.opera.com/sitepatching/blog/show.dml/21280702

If you set opera:config#Browser%20JavaScript to 0, CKEditor should load fine again.

Now, the browser.js patch is merely testing the water prior to doing a proper core fix which we intended to ship later. The intention was to quickly get a feel for how many compat problems we'd run into (and man, we were really successful at that :-)).

We wanted to remove element.all because supporting it causes problems on some sites. I would like to ask if you can adjust your browser sniffing / object detection here and not make Opera use the method that depends on element.all (though I also fully realise that this code in an app with the number of legacy installations CKEditor has might mean that we can not ship any core fix and are stuck supporting element.all).

comment:4 Changed 6 years ago by hallvord@…

BTW: just to make this clear, the browser.js file with this patch was only released for preview/alpha builds (10.70, 11), so regular release builds everywhere are not and will not be affected by this experiment.

comment:5 Changed 6 years ago by hallvord@…

To track the progress / planning of the core fix, look at CORE-13301 "Remove the HTMLElement.all property" (should be available to those CKEditor developers who have access to Opera's BTS)

comment:6 Changed 6 years ago by fredck

  • Keywords Opera added
  • Status changed from new to confirmed

@Hallvord, can you give some hints on how to patch Opera to remove element.all, so we can try it here and eventually bring a fix to the editor, even if Opera will have that change only in the future?

comment:7 Changed 6 years ago by hallvord@…

Sure Fred, what we did in browser.js was as simple as this:

HTMLElement.prototype.__defineGetter__('all', function(){});

Place that either in a user.js or temporarily paste it in the editor's code.

comment:8 Changed 5 years ago by garry.yao

  • Cc garryyao added

comment:9 Changed 5 years ago by hallvord@…

  • Summary changed from CKEditor entirely broken in latest Opera to Do not use element.all property (CKEditor entirely broken in latest Opera)

comment:10 Changed 5 years ago by hallvord@…

BTW, this is where it needs fixing: http://dev.ckeditor.com/browser/CKEditor/trunk/_source/core/dom/element.js#L1219 I suggest iterating over a collection created with element.getElementsByTagName('*') instead.

Changed 5 years ago by Ms2ger

Use getElementsByTagName

comment:11 Changed 5 years ago by Ms2ger

  • Keywords HasPatch added

Changed 5 years ago by fredck

comment:12 Changed 5 years ago by fredck

  • Owner set to fredck
  • Status changed from confirmed to review

I wonder why we have things like sibling.$.all || sibling.$.getElementsByTagName( '*' ) in our range code, but the web tells me that we should not have problems with IE6+.

Thanks for the patch, Ms2ger.

comment:13 Changed 5 years ago by Ms2ger

Fred, any news here?

comment:14 Changed 4 years ago by garry.yao

  • Keywords HasPatch removed
  • Status changed from review to review_passed

comment:15 Changed 4 years ago by fredck

  • Milestone set to CKEditor 3.6.3
  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [7405].

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