Opened 11 years ago

Closed 11 years ago

#2918 closed Bug (fixed)

Initial values for checkboxes are wrong

Reported by: Martin Kou Owned by: Martin Kou
Priority: Normal Milestone: CKEditor 3.0
Component: UI : Dialogs Version: SVN (FCKeditor) - Retired
Keywords: Review+ Cc:

Description

To reproduce the bug:

  1. Open replacebyclass.html in Firefox.
  2. Open the link dialog.
  3. Switch to target tab, and choose popup target. You should see 8 checkboxes which are not checked.
  4. Cancel and close the link dialog.
  5. Repeat step 3.
  6. But this time, all the checkboxes are ticked, which is wrong.

Attachments (3)

2918.patch (629 bytes) - added by Martin Kou 11 years ago.
2918_2.patch (731 bytes) - added by Martin Kou 11 years ago.
test-checkbox-checked.patch (645 bytes) - added by Garry Yao 11 years ago.
Patch used for setup test

Download all attachments as: .zip

Change History (18)

Changed 11 years ago by Martin Kou

Attachment: 2918.patch added

comment:1 Changed 11 years ago by Martin Kou

Keywords: Review? added

The bug was found to be due to the checkbox object constructor saving default value as an array instead of as a simply boolean value.

comment:2 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

You may change the fix to the following and commit it directly:

!!elementDefinition.checked

comment:3 Changed 11 years ago by Martin Kou

Fixed with [3068].

comment:4 Changed 11 years ago by Martin Kou

Resolution: fixed
Status: newclosed

comment:5 Changed 11 years ago by Martin Kou

Resolution: fixed
Status: closedreopened

Ok.. turns out we're having another problem now.

If one of the checkboxes in the link dialog has a default value set to true...

default : true

Then the first opening of the dialog would display that checkbox as checked, yet subsequent openings would display is as unchecked.

The cause of this bug is because of the checkbox UI element constructor is looking for the 'checked' attribute for the default value, yet other parts of the plugin assumes the attribute should be called 'default'.

Changed 11 years ago by Martin Kou

Attachment: 2918_2.patch added

comment:6 Changed 11 years ago by Martin Kou

Keywords: Review? added; Review+ removed

comment:7 Changed 11 years ago by Garry Yao

Resolution: fixed
Status: reopenedclosed

Fixed with [3054].

comment:8 Changed 11 years ago by Garry Yao

Resolution: fixed
Status: closedreopened

Oops, make a mistake here, this ticket has nothing to do with [3054].

Changed 11 years ago by Garry Yao

Attachment: test-checkbox-checked.patch added

Patch used for setup test

comment:9 Changed 11 years ago by Garry Yao

A probably related bug would be set checked:true in checkbox definition doesn't work. Procedures, confirmed on FF3:

  1. Setup by apply the test-checkbox-checked ;
  2. Open _samples\api_dialog.html;
  3. Open link dialog;
  4. Found the checkbox is left as unchecked.

comment:10 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:11 Changed 11 years ago by Garry Yao

Keywords: Review- added; Review+ removed

Perhaps the patch doesn't handle the test case in comment:9, to take care the initial values, the override would result in something like:

			var overrides =
	            {
		            'default' :!!elementDefinition[ 'default' ]
	            };
	            if ( typeof elementDefinition.checked !== 'undefined' )
				overrides.initValue = !!elementDefinition.checked;

comment:12 Changed 11 years ago by Martin Kou

Oh, actually I've discussed with Fred about that yesternight before submitting the patch. To make the checked attribute work, you'd have to override the initial value related functions in addition to the change you proposed, which increases code complexity without much benefit in return. So the checked attribute in checkbox element definition was to be changed to 'default' instead.

comment:13 Changed 11 years ago by Martin Kou

Keywords: Review? added; Review- removed

comment:14 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:15 Changed 11 years ago by Martin Kou

Resolution: fixed
Status: reopenedclosed

Fixed with [3076].

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