Opened 7 years ago

Closed 7 years ago

#7023 closed Bug (fixed)

IE: JS error when Selection field inserted on page

Reported by: Krzysztof Studnik 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 7 years ago.
select.gif (155 bytes) - added by Garry Yao 7 years ago.
7023_2.patch (1015 bytes) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by Wiktor Walc

Milestone: CKEditor 3.5.1
Status: newconfirmed

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 7 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 7 years ago by Wiktor Walc

Milestone: CKEditor 3.5.1CKEditor 3.5.2

comment:4 Changed 7 years ago by Wiktor Walc

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

comment:5 Changed 7 years ago by Wiktor Walc

Cc: naresh.sivaraman added
Keywords: Oracle added

#7135 was marked as duplicate

Changed 7 years ago by Garry Yao

Attachment: 7023.patch added

Changed 7 years ago by Garry Yao

Attachment: select.gif added

comment:6 Changed 7 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

comment:7 Changed 7 years ago by Sa'ar Zac Elias

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 7 years ago by Frederico Caldeira Knabben

Keywords: Discussion removed
Status: reviewreview_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 7 years ago by Garry Yao

Attachment: 7023_2.patch added

comment:9 Changed 7 years ago by Garry Yao

Status: review_failedreview

comment:10 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 7 years ago by Garry Yao

Status: review_failedreview

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 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 7 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [6594].

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