Opened 6 years ago

Closed 6 years ago

#7023 closed Bug (fixed)

IE: JS error when Selection field inserted on page

Reported by: krst Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.5.3
Component: General Version: 3.5.1
Keywords: IE Oracle Cc: naresh.sivaraman

Description

TC

  • Under IE6/7 , from samples, select Replace by class
  • Clear all content
  • Click on the "Selection Field button"
  • create selection field as follows:
    • Name: 1
    • Size: 2
    • Text: Insert Test1, test2, test3 (without entering values)
    • Values: insert v1, v2, v3
  • Press OK button

Expected result:

Selection field is added to edit area

<p>
	<select name="1" size="2"><option value="">test1</option><option value="">test2</option><option selected="selected" value="">test3</option><option value="v1"></option><option value="v2"></option><option value="v3"></option></select></p>

Actual result Selection field is added to edit area, but JS error is thrown

Line 14
char 1708
error 'parentNode' is null or not an object

Error was not shown in CKE v3.5, under IE
Under Firefox 3.6 and Opera11, no error is thrown with CKE v3.5.1.

Attachments (3)

7023.patch (4.5 KB) - added by garry.yao 6 years ago.
select.gif (155 bytes) - added by garry.yao 6 years ago.
7023_2.patch (1015 bytes) - added by garry.yao 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by wwalc

  • Milestone set to CKEditor 3.5.1
  • Status changed from new to confirmed

Regression caused by [6355]. Make sure to check #6999 when fixing this one.

The error occurs whenever a selection field with more than one available option is inserted.

comment:2 Changed 6 years ago by garry.yao

Not a regression of [6355] but just another copy of #4611, the original fix from #4611 is just a workaround, while it just doesn't apply to our new codes, instead of making any similar fix around, I would suggest to present select with fake image, just like we did to the hiddenfield, which could eliminate such issue (any text selection around select element is wrong).

comment:3 Changed 6 years ago by wwalc

  • Milestone changed from CKEditor 3.5.1 to CKEditor 3.5.2

comment:4 Changed 6 years ago by wwalc

Fake element seems like a way to go for me as well. Also for tickets like #7031.

comment:5 Changed 6 years ago by wwalc

  • Cc naresh.sivaraman added
  • Keywords Oracle added

#7135 was marked as duplicate

Changed 6 years ago by garry.yao

Changed 6 years ago by garry.yao

comment:6 Changed 6 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from confirmed to review

comment:7 Changed 6 years ago by Saare

  • Keywords Discussion added

Using fake object might introduce usability issues, regarding size and multi-selection possibility, not seeing the default option etc.

comment:8 Changed 6 years ago by fredck

  • Keywords Discussion removed
  • Status changed from review to review_failed

While fake elements may be a good solution for it, a lot of work needs to be done here to propose it properly. I'm very worried about the lack of wysiwyg experience that it could have. Other than that, users may be used to the current way (we have it that way since FCKeditor 1).

Because of this, I would ask to please focus on this ticket issue exclusively for now, making things work as before. A more drastic solution may be considered in the future, on a dedicated ticket.

Changed 6 years ago by garry.yao

comment:9 Changed 6 years ago by garry.yao

  • Status changed from review_failed to review

comment:10 follow-up: Changed 6 years ago by fredck

  • Status changed from review to review_failed

It looks like all the changes made are not needed. You're simply putting the problematic logic inside the try/catch handler. At this point, it's enough to simply move the 744 inside the try{} block following it, without having to create the "sibling" variable.

comment:11 in reply to: ↑ 10 Changed 6 years ago by garry.yao

  • Status changed from review_failed to review

Replying to fredck:

It looks like all the changes made are not needed.

Can't share it, please conduct the review after actually applying the patch.

comment:12 Changed 6 years ago by fredck

  • Status changed from review to review_passed

You're right. In fact "child" doesn't get assigned if the error happens.

I just feel something is wrong here, but anyway.

comment:13 Changed 6 years ago by garry.yao

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

Fixed with [6594].

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