Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto adding # to color from colordialog #580

Merged
merged 12 commits into from
Jul 5, 2017
Merged

Auto adding # to color from colordialog #580

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
508eecb
Bug fix.
Jun 28, 2017
bc2ed66
Adding unit and manual tests.
Jun 29, 2017
43d5a42
Move fix to colordialog from colorbutton.
Jun 30, 2017
e339434
Revert focus change. Becasue tests will change this one won't be nece…
Jun 30, 2017
82cbcda
Different solution requires different test.
Jun 30, 2017
df8a63b
Adding unit and manual tests.
Jun 30, 2017
dbb8016
Get back this change. Prevent of throwing errors in test on IE8
Jul 4, 2017
e463657
Remake test to be oneliners.
Jul 4, 2017
818bd17
Correct typo and make better note description.
Jul 4, 2017
ba184c6
Update test case names and impove manual test description.
Jul 5, 2017
dbf8d8e
Tests: rephrased manual test description.
f1ames Jul 5, 2017
2b6bd45
Changelog entry. [skip ci]
f1ames Jul 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions plugins/colordialog/dialogs/colordialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ CKEDITOR.dialog.add( 'colordialog', function( editor ) {
html: ' '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of the changes in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change allows on writing unit test for this bug-fix.

Generally writing test for it was very tricky. Problem appeared when ok button is pressed then method clearHighlight() is called. What throw an error because unit test doesn't highlight colour in dialog as user does.
So I just add if-statement which check if focus is defined and also rearrange variable declaration to pass JShint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Color dialog hasn't got any unit test earlier.
This change:

if ( focused ) {
	focused.removeClass( focusedColorLightCls );
	focused.removeClass( focusedColorDarkCls );
}

Allows on making nice unit test which set up values but doesn't touch focused colour. So test don't throw up errors when only value exists without focusing it.

};

var numbering = function( id ) {
return CKEDITOR.tools.getNextId() + '_' + id;
},
hicolorId = numbering( 'hicolor' ),
hicolorTextId = numbering( 'hicolortext' ),
selHiColorId = numbering( 'selhicolor' ),
table;

function clearSelected() {
$doc.getById( selHiColorId ).removeStyle( 'background-color' );
dialog.getContentElement( 'picker', 'selectedColor' ).setValue( '' );
Expand Down Expand Up @@ -85,8 +93,10 @@ CKEDITOR.dialog.add( 'colordialog', function( editor ) {
}

function clearHighlight() {
focused.removeClass( focusedColorLightCls );
focused.removeClass( focusedColorDarkCls );
if ( focused ) {
focused.removeClass( focusedColorLightCls );
focused.removeClass( focusedColorDarkCls );
}
setHighlight( false );
focused = null;
}
Expand Down Expand Up @@ -257,14 +267,6 @@ CKEDITOR.dialog.add( 'colordialog', function( editor ) {
appendColorCell( oRow.$, '#ffffff' );
}

var numbering = function( id ) {
return CKEDITOR.tools.getNextId() + '_' + id;
},
hicolorId = numbering( 'hicolor' ),
hicolorTextId = numbering( 'hicolortext' ),
selHiColorId = numbering( 'selhicolor' ),
table;

createColorTable();

// Load CSS.
Expand Down
14 changes: 11 additions & 3 deletions plugins/colordialog/plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,24 @@ CKEDITOR.plugins.colordialog = {
* @member CKEDITOR.editor
*/
editor.getColorFromDialog = function( callback, scope ) {
var onClose = function( evt ) {
var onClose,
releaseHandlers,
bindToDialog;

onClose = function( evt ) {
releaseHandlers( this );
var color = evt.name == 'ok' ? this.getValueOf( 'picker', 'selectedColor' ) : null;
// Adding `#` character to hexadecimal 3 or 6 digit numbers to have proper color value (#565).
if ( /^[0-9a-f]{3}([0-9a-f]{3})?$/i.test( color ) ) {
color = '#' + color;
}
callback.call( scope, color );
};
var releaseHandlers = function( dialog ) {
releaseHandlers = function( dialog ) {
dialog.removeListener( 'ok', onClose );
dialog.removeListener( 'cancel', onClose );
};
var bindToDialog = function( dialog ) {
bindToDialog = function( dialog ) {
dialog.on( 'ok', onClose );
dialog.on( 'cancel', onClose );
};
Expand Down
56 changes: 56 additions & 0 deletions tests/plugins/colordialog/colordialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/* bender-tags: editor */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to always put unit tag if it is an unit test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we discussed this once again, if unit tag is not needed (not used by bender) it should be skipped.

/* bender-ckeditor-plugins: colordialog,wysiwygarea */

( function() {
'use strict';

bender.editor = true;

bender.test( {
assertColor: function( inputColor, outputColor ) {
var editor = this.editor;

editor.once( 'dialogShow', function( evt ) {
var dialog = evt.data;
dialog.setValueOf( 'picker', 'selectedColor', inputColor );
dialog.getButton( 'ok' ).click();

} );

editor.getColorFromDialog( function( color ) {
resume( function() {
assert.areSame( outputColor, color );
} );
} );
wait();
},

'test colordialog add hash to colors 6 digits': function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think test names would be more readable like:

  • test colordialog add hash to colors - 6 digits
  • test colordialog add hash to colors: 6 digits
  • test colordialog add hash to colors with 6 digits color

this.assertColor( '123456', '#123456' );
},

'test colordialog add hash to colors 3 digits': function() {
this.assertColor( 'FDE', '#FDE' );
},

'test colordialog does not add hash 1 digit': function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails on IE8.

// IE8 don't allow on totally wrong values of attributes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IE8 doesn't allowing invalid values means the attribute will not be set? I think it might be more clear with something like:
IE8 doesn't allow to use invalid color values. or IE8 doesn't set invalid color values. ?

if ( CKEDITOR.env.ie && CKEDITOR.env.version < 9 ) {
assert.ignore();
}
this.assertColor( '1', '1' );
},

'test colordialog does not add hash color name': function() {
this.assertColor( 'red', 'red' );
},

'test colordialog does not add hash rgb': function() {
this.assertColor( 'rgb(10, 20, 30)', 'rgb(10, 20, 30)' );
},

'test colordialog does not add hash empty value': function() {
this.assertColor( '', '' );
}
} );
} )();
15 changes: 15 additions & 0 deletions tests/plugins/colordialog/manual/addhash.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<textarea id='editor1'>
<table border="1" cellspacing="1" cellpadding="1">
<tr>
<td>Cell 1</td><td>Cell 2</td>
</tr>
</table>
</textarea>

<script>
if ( bender.tools.env.mobile ) {
bender.ignore();
} else {
CKEDITOR.replace( 'editor1' );
}
</script>
13 changes: 13 additions & 0 deletions tests/plugins/colordialog/manual/addhash.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
@bender-tags: tc, feature, colordialog, 565, 4.8.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do not use feature tag (unless it was added recently)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it was discussed some time ago there should either bug or feature tag? Or does it change since then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm not up to date - if it was discussed with @mlewand or @Comandeer in the last 2-3 weeks it means it should be that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was discussed around 12 - 13 June on ckeditor4-chat. I made some kind of sum up for us as pinned post to ckeditor4-chat.

@bender-ui: collapsed
@bender-ckeditor-plugins: table,tabletools,colordialog,toolbar,wysiwygarea,sourcearea,contextmenu
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is sourcearea plugin really needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful when you wish to check what was included to attribute of tag. E.g. if there will be some invalid string or colour hard to distinguish from others, then checking source might be helpful.


1. Right click into table cell.
1. Select `Cell -> Cell Properties`.
1. Choose background color.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean to click Choose button and then type color in a colordialog? It might be a little unclear, and typing color without hash in Cell Properties dialog does not add hash (and color when closing dialog) so it may seem it is broken in this case.
To make it more clear you may use: Click Choose next to Background Color or something similar.

1. Type color in hexadecimal 6-digit format **without** leading `#`. E.g. (`ABC123`).
1. Confirm it.
1. Inserted color should contain additional `#` at the beginning.
1. Repeat above steps for color with hexadecimal 3-digits format. E.g. (`FFF`).

**Note:** You can also check with colors in different formats (rgb, text) or invalid hex strings. For such colors `#` should not be added.