Opened 10 years ago

Closed 10 years ago

#3549 closed Bug (fixed)

[IE8] Empty dropdowns

Reported by: ppx Owned by: Tobiasz Cudnik
Priority: Normal Milestone: CKEditor 3.0
Component: General Version: 3.0 Beta 2
Keywords: IE Confirmed Review+ Cc:

Description

IE8's compatibility mode there is no problem, but not in compatibility mode button will be under the font settings problem

Attachments (6)

3549.patch (517 bytes) - added by Tobiasz Cudnik 10 years ago.
3549_2.patch (1.6 KB) - added by Tobiasz Cudnik 10 years ago.
3549_3.patch (611 bytes) - added by Tobiasz Cudnik 10 years ago.
3549_4.patch (2.4 KB) - added by Tobiasz Cudnik 10 years ago.
3549_5.patch (665 bytes) - added by Tobiasz Cudnik 10 years ago.
3549_6.patch (644 bytes) - added by Tobiasz Cudnik 10 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 10 years ago by ppx

Owner: set to ppx

comment:2 Changed 10 years ago by Tobiasz Cudnik

Owner: changed from ppx to Tobiasz Cudnik
Status: newassigned

comment:3 Changed 10 years ago by Tobiasz Cudnik

Keywords: IE Confirmed added
Summary: The issue of IE8[IE8] Empty dropdowns

I confirm that issue exists in following modes:

  • IE8 in IE7 strict mode
  • IE8 in quirks mode
  • IE8 compatibility in IE8 strict mode

comment:4 Changed 10 years ago by Tobiasz Cudnik

Priority: HighNormal

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3549.patch added

comment:5 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added

IE8 seems to add strange .null_container class to iframe's html element. This patch overloads visibility property from this class using inline style.

comment:6 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review- added; Review? removed

Problem seem to be created in _source/plugins/panel/plugin.js L132 and this workaround is unnecessary.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3549_2.patch added

comment:7 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

The real problem was with CKEDITOR.dom.element.getAttribute function and it's IE workarounds.

I've introduced new env.ie7Compat property and changed env.ie8 to reflect usage of IE8 independently from compatibility mode.

Patch fixes this issue but can create new issues in places depending on env.ie8, although i haven't found any.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3549_3.patch added

comment:8 Changed 10 years ago by Tobiasz Cudnik

I'm proposing yet another patch with alternative approach for all IE versions, which doesn't need modifications in env object. Although i'm not sure if it's better then second one.

comment:9 Changed 10 years ago by Garry Yao

Keywords: Review- added; Review? removed

The patch failed the following TCs for element.html on IE8 standard mode.

  • test_setAttribute1,2
  • test_setAttributes
  • test_removeClass1,2

comment:10 Changed 10 years ago by Garry Yao

Besides,it's better to avoid using guard with dom attributes result, since falsy values e.g. "" or 0 are also possible results.

   return this.$[ name ] || standard.call( this, name );

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3549_4.patch added

comment:11 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

Although none of failed tests was related to the patch itself and was failing also without it i've prepared wider patch for element.js which passes element.html in all IE8 variations (there are 8 of them).

I could have some doubts about test itself, because it expects case-insensitive class names, which isn't fully correct.

comment:12 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The changes are good, but it looks like they are not needed in IE8 standards mode. So, it would be better to not force the IE workarounds for that case.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3549_5.patch added

comment:13 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

5th patch fixes issue using IE8 API when possible and fallbacks only when needed. It was possible to achieve using changes from [3537].

Dropdown content is visible on IE8 modes.

comment:14 Changed 10 years ago by Garry Yao

Keywords: Review- added; Review? removed

Do you think:

( !CKEDITOR.env.ie8 || CKEDITOR.env.ie7Compat || CKEDITOR.env.ie6Compat )

is same as:

( CKEDITOR.env.ie7Compat || CKEDITOR.env.ie6Compat )

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3549_6.patch added

comment:15 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

Actually yes, thats true. ie7Compat and ie6Compat are not IE8-only flags, so env.ie8 can be omitted.

comment:16 Changed 10 years ago by Garry Yao

Keywords: Review+ added; Review? removed

Please make sure the patch trunk contains BOM is removed when committing.

comment:17 Changed 10 years ago by Garry Yao

And, very important, though not related to this specific tickets, please have the setAttribute fixed in the same way also.

comment:18 Changed 10 years ago by Tobiasz Cudnik

Resolution: fixed
Status: assignedclosed

Fixed with [3625]. element.html test failes moved to #3679.

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