Opened 6 years ago

Closed 6 years ago

#3089 closed Bug (fixed)

[IE]plugin:showblock problems

Reported by: garry.yao Owned by: alfonsoml
Priority: Normal Milestone: CKEditor 3.0
Component: General Version: SVN (FCKeditor) - Retired
Keywords: IE8 Review+ Cc:

Description

It's unable to switch off ShowBlock on IE.

Attachments (4)

3089.patch (1.2 KB) - added by garry.yao 6 years ago.
3089_2.patch (1.4 KB) - added by alfonsoml 6 years ago.
Updated patch
3089_3.patch (1.2 KB) - added by alfonsoml 6 years ago.
Updated patch
3089_4.patch (2.8 KB) - added by garry.yao 6 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 years ago by garry.yao

  • Keywords IE8 added; IE removed

IE8 specific problem, which breaks CKEDITOR.dom.element::removeClass

Changed 6 years ago by garry.yao

comment:2 Changed 6 years ago by garry.yao

  • Keywords Confirmed Review? added
  • Owner set to garry.yao
  • Status changed from new to assigned

comment:3 follow-up: Changed 6 years ago by alfonsoml

  • Keywords Review- added; Review? removed

As IE8 is still in beta testing I think that it's better to wait until the final release to verify that this bug is still present instead of adding a workaround that might cause other problems if the behavior of IE8 is changed.

Meanwhile, a ticket should be open at their Connect site with a simple testcase to verify the problem. Can you take care at least of creating the testcase, or would you rather work on other things?

comment:4 in reply to: ↑ 3 Changed 6 years ago by garry.yao

  • Keywords HasPatch Pending added; Confirmed Review- removed

Replying to alfonsoml:

As IE8 is still in beta testing I think that it's better to wait until the final release to verify that this bug is still present instead of adding a workaround that might cause other problems if the behavior of IE8 is changed.

Meanwhile, a ticket should be open at their Connect site with a simple testcase to verify the problem. Can you take care at least of creating the testcase, or would you rather work on other things?

Thanks, but IE8's already in RC, and it's supporting removeAttribute('class') sounds like a standard compliant feature rather than a bug, but it's nice idea to hold our supporting with IE8 until it got released.

comment:5 Changed 6 years ago by alfonsoml

The patch works OK in the final release of IE8, but I would change the detection from version == 8 to version >= 8 (ideally it should work based on feature/bug detection, but that is another task)

Anyway, the block in the hasAttributes function includes a case for '_cke_expando', and I don't know how to test if this is OK in IE8.

Changed 6 years ago by alfonsoml

Updated patch

comment:6 Changed 6 years ago by alfonsoml

I've provided an updated patch that also fixes #3182.

Changes with regards to the original:

  • As I've suggested, the detection is version >=8
  • Added protection in getAttribute to fix #3182
  • in hasAttributes the condition was missing the negation, so it had the reverse effect.

Anyway, I still don't know how to check properly that the rest of the staff like '_cke_expando' works properly.

comment:7 Changed 6 years ago by garry.yao

  • Keywords Review? added; HasPatch Pending removed

I'll adding a TC for '_cke_expando', which could be handled by other ticket.

comment:8 Changed 6 years ago by alfonsoml

  • Keywords Review- added; Review? removed

Sorry, when I used the automated tests they showed that the patch was wrong, the hasAttributes should be executed like any other IE (just due to that _cke_expando).

I'll try to use the tests from now on :-)

Changed 6 years ago by alfonsoml

Updated patch

comment:9 Changed 6 years ago by alfonsoml

  • Keywords Review? added; Review- removed
  • Owner changed from garry.yao to alfonsoml
  • Status changed from assigned to new

Proposed new patch to fix the current testcases.

Changed 6 years ago by garry.yao

comment:10 Changed 6 years ago by garry.yao

  • Keywords Review+ added; Review? removed

R+ for 3089_3, appending TC with the updated patch.

comment:11 Changed 6 years ago by alfonsoml

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

Fixed with [3393]

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