Opened 11 years ago

Closed 11 years ago

#3529 closed Bug (fixed)

[IE] Wrong find matching in IE8 quirks

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

Description

Wrong find matching in IE8 quirks.

TC

  1. Use following content:
    <p>
    	^a foo a</p>
    <p>
    	a foo a</p>
    <p>
    	&nbsp;</p>
    
    
  2. Open Find dialog
  3. Search for "foo"

Result:

<p>
	a ^foo a^</p>
<p>
	a foo a</p>
<p>
	&nbsp;</p>

Expected result:

<p>
	a ^foo^ a</p>
<p>
	a foo a</p>
<p>
	&nbsp;</p>

Does not take place in IE8 strict.

Attachments (3)

3529.patch (2.4 KB) - added by Garry Yao 11 years ago.
3529_2.patch (2.5 KB) - added by Garry Yao 11 years ago.
3529_3.patch (2.8 KB) - added by Garry Yao 11 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: newassigned

comment:2 Changed 11 years ago by Garry Yao

Owner: changed from Tobiasz Cudnik to Garry Yao
Status: assignednew

As discussed with Tobias, I'll take over this ticket to avoid overlapping at several recent related tickets.

comment:3 Changed 11 years ago by Garry Yao

Keywords: Review? added
Status: newassigned

This is an uncovered case of #3436, yet another browser sniffing mistake, the culprit is that the detection result of CKEDITOR.env.version == 7 under IE8 quirk will skip the fix for CKEDITOR.dom.text::splitText.
The idea fix should use feature detection instead.

Changed 11 years ago by Garry Yao

Attachment: 3529.patch added

comment:4 Changed 11 years ago by Martin Kou

Keywords: Review+ added; Review? removed

Good job.

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

Keywords: Review- added; Review+ removed

Warning: that trick to detect IE8 isn't safe.

in Firefox 3.5

Object.prototype.toString.call( self.JSON )  !=  "[object JSON]"

is false, so it will follow the IE8 code path although it doesn't needs it. As other browsers implement native JSON support they will also fail the test

comment:6 Changed 11 years ago by Martin Kou

Oh, you're right, I haven't considered that.

comment:7 Changed 11 years ago by Frederico Caldeira Knabben

CKEDITOR.env should support all our browser detection needs.

Changed 11 years ago by Garry Yao

Attachment: 3529_2.patch added

comment:8 Changed 11 years ago by Garry Yao

Keywords: Review? added; Review- removed

Proposing a better way to detect this feature and centralize it into CKEDITOR.env.

comment:9 Changed 11 years ago by Martin Kou

The new detection method is more accurate for sure, but I'm not sure if the increased core code size is worth it.

comment:10 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

That's correct. We don't need to bloat CKEDITOR.env. It "already" offers all we need, and for this case, it's enough to check for IE8 inside the spli function. My previous concerns were about using the JSON object for detection.

This fix must be as simple as possible, and even the newly proposed detection system can be simplified.

Changed 11 years ago by Garry Yao

Attachment: 3529_3.patch added

comment:11 Changed 11 years ago by Garry Yao

Keywords: Review? added; Review- removed

I've reverted the detect to browser sniffing way, which include part of Tobiasz's approach at #3549 here for detecting ie8 with documentMode.

comment:12 Changed 11 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:13 Changed 11 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3509].

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