Font options are not toggleable any longer #615

msamsel · 2017-07-06T13:34:50Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

yes

This PR contains

Unit tests
Manual tests

What changes did you make?

Disable possibility to toggle option on drop down list of: font size, font family name, paragraph format
Add (Default) option to font size and font family name, which remove formating from selection
Add new language entry with Default to lang.common file
Fix one unit test which click twice into this same menu option and start to fail after change

close #584

f1ames

Looks good, some minor changes needed.

Have you used langtool to generate language files? I don't see any new entry in meta language files.

f1ames · 2017-07-12T12:25:00Z

plugins/font/plugin.js

 				this.startGroup( lang.panelTitle );

-				for ( var i = 0; i < names.length; i++ ) {
-					var name = names[ i ];
+				// Add `(Default)` item as first element on the drop down list.


// Add (Default) item as a first element on the drop down list.

f1ames · 2017-07-12T12:27:44Z

plugins/font/plugin.js

+					endBoundary,
+					node,
+					bm,
+					defaultValue = 'cke-default';


Same default value is already defined in an init method, maybe it could be extracted somewhere to be more "global"? Especially that it is used also in other places (tests).

f1ames · 2017-07-12T12:40:41Z

plugins/format/plugin.js


-				editor[ style.checkActive( elementPath, editor ) ? 'removeStyle' : 'applyStyle' ]( style );
+				// Always aply style, do not allow on toggle it by clicking on list item (#584).


// Always apply style, do not allow on toggle it by clicking on list item (#584).

f1ames · 2017-07-12T12:52:01Z

plugins/format/plugin.js


-				editor[ style.checkActive( elementPath, editor ) ? 'removeStyle' : 'applyStyle' ]( style );
+				// Always aply style, do not allow on toggle it by clicking on list item (#584).
+				editor.applyStyle( style );


By default all format tags are block elements so reapplying them to partial selection doesn't do anything. However, if you change the config like:

config.format_p = { element: 'span', attributes: { 'class': 'normalPara' } };

You can apply it to partial selection despite the fact it was already applied (so it creates nested tags). This is an edge case IMHO but still we should handle this. You could just check if style is active (as before style.checkActive( elementPath, editor )) and apply it only if it is not already there.

f1ames · 2017-07-12T12:55:49Z

tests/plugins/font/font.js

@@ -170,6 +172,43 @@
 				'<p>x<span style="font-size:12px"><em>foo</em></span><em><span style="font-size:24px">bar</span></em>x@</p>' );
 		},

+		// #584


Is there any test checking if second click (reapplying) of the same font/size doesn't change the content?

I see there is on in a format plugin. Still those are separate plugin with different code so it should be tested for both.

f1ames · 2017-07-12T12:56:26Z

tests/plugins/font/manual/preservestyle.md

+@bender-ui: collapsed
+@bender-ckeditor-plugins: wysiwygarea, toolbar, font, elementspath, sourcearea
+
+1. Select text


Each list item should end with .. Select some text sounds a little better.

msamsel · 2017-07-13T09:59:48Z

I hope it is fixed now :)

f1ames

One failing test on IE8:

Same test throws timeout on IE9-11.

f1ames · 2017-07-25T08:14:43Z

dev/langtool/meta/ckeditor.core/meta.txt

@@ -81,3 +81,4 @@ common.keyboard.36 = Label for the "Home" key.
 common.keyboard.46 = Label for the "Delete" key.
 common.keyboard.224 = Label for the "Command" key.
 common.keyboardShortcut = ARIA message for keyboard shortcuts.
+common.optionDefault = "(Default)" option for drop-down menues of text styling, which removes specific style.


Maybe something like:

Label for "(Default)" option in drop-down text-styling menus. Using such option restores default styling.

f1ames · 2017-07-25T09:04:37Z

plugins/font/plugin.js

 				this.startGroup( lang.panelTitle );

-				for ( var i = 0; i < names.length; i++ ) {
-					var name = names[ i ];
+				// Add `(Default)` item as a first element on the drop down list.


Use consistent naming: drop-down.

f1ames · 2017-07-25T09:12:16Z

tests/plugins/font/font.js

+			this.assertCombo( 'Font', 'cke-default', false, bot, '<p>foo@</p>' );
+		},
+
+		'test reaaply this same font size twice': function() {


reapply

f1ames · 2017-07-25T09:12:25Z

tests/plugins/font/font.js

+
+		},
+
+		'test reaaply this same font family twice': function() {


reapply

f1ames · 2017-07-25T09:14:43Z

tests/plugins/font/manual/preservestyle.md

+@bender-ckeditor-plugins: wysiwygarea, toolbar, font, elementspath, sourcearea
+
+1. Select some text.
+1. Change `font size`, (e.g. `24`).


Redundant comma after size:

Change font size, (e.g. 24).

f1ames · 2017-07-25T09:44:24Z

tests/plugins/format/manual/preserveformatafterclick.md

+@bender-ckeditor-plugins: wysiwygarea, toolbar, format, elementspath, sourcearea
+
+1. Select text
+1. Try to apply multiple times this same format to one selection (different than normal).


Try to apply same format (other than Normal) multiple times.

?

f1ames · 2017-07-25T09:45:16Z

tests/plugins/format/manual/preserveformatafterclick.md

+1. Select text
+1. Try to apply multiple times this same format to one selection (different than normal).
+
+**Expected:** Choosen format should remain this same all the time.


should remain ~~this~~the same all the time.

f1ames · 2017-07-25T09:45:53Z

tests/plugins/format/manual/preserveformatafterclick.md

+
+**Expected:** Choosen format should remain this same all the time.
+
+**Unexpected:** Text format toggle between styled and unstyled (Normal) one.


toggles

f1ames · 2017-07-25T09:46:59Z

tests/plugins/format/plugin.js

@@ -25,5 +25,29 @@ bender.test( {
 		bot.setHtmlWithSelection( '<fieldset><legend>^foo</legend><form>bar</form></fieldset>' );
 		var name = 'Format', combo = ed.ui.get( name );
 		assert.areSame( CKEDITOR.TRISTATE_DISABLED, combo._.state, 'check state disabled when not in context' );
+	},
+
+	// Drop down format menu should not have possibility to toggle option (#584).


Drop-down

f1ames · 2017-07-25T09:47:54Z

tests/plugins/format/plugin.js

+			combo = editor.ui.get( name );
+
+		bot.setHtmlWithSelection( '<p>^foo</p>' );
+		// apply format 1st time


Use full sentences, e.g. Apply format 1st time.

f1ames

Earlier mentioned tests (despite the fact it was adjusted) seems to still be unstable on IE8. When run in group it seems to fail in most cases. When only test suite is run it fails like 50% of times. When the test itself is run it sometimes fails. Seems like some issue with focus, if I click inside a browser window before running in most cases it works.

I see this test was based on an existing one, but the former one seems to work fine. Maybe it affects the second test somehow?

f1ames · 2017-07-26T11:22:51Z

plugins/format/plugin.js

@@ -67,7 +67,10 @@ CKEDITOR.plugins.add( 'format', {
 				var style = styles[ value ],
 					elementPath = editor.elementPath();

-				editor[ style.checkActive( elementPath, editor ) ? 'removeStyle' : 'applyStyle' ]( style );
+				// Always apply style, do not allow on toggle it by clicking on list item (#584).


Always apply style, do not allow to toggle it by clicking on corresponding list item (#584).

?

f1ames · 2017-07-26T11:24:14Z

tests/plugins/font/font.js

@@ -35,8 +48,10 @@

 				this.wait( function() {
 					// Click again to exit the style.
+					// Since 4.8.0 2nd click into this same menu item do not unselect it.


Since 4.8.0, 2nd click on the same menu item does not unselect it.

f1ames · 2017-07-26T11:24:57Z

tests/plugins/font/font.js

@@ -35,8 +48,10 @@

 				this.wait( function() {
 					// Click again to exit the style.
+					// Since 4.8.0 2nd click into this same menu item do not unselect it.
+					// It is required to click into '(Default)' option to reset style (#584).


It is required to click on the '(Default)' option to reset style (#584).

f1ames · 2017-07-26T11:29:42Z

tests/plugins/format/plugin.js

+		bot.combo( name, function( combo ) {
+			assert.areSame( CKEDITOR.TRISTATE_ON, combo.getState(), 'check state ON when opened' );
+			combo.onClick( 'h1' );
+			assert.areSame( '<h1>f^oo</h1>', bot.htmlWithSelection(), 'applied h1 block style' );


applied h1 block style

h1 was not applied here as it was already there. The test (and assert) check if h1 is not removed when applied second time, so maybe preserved h1 block style or something similar?

msamsel · 2017-07-28T10:10:18Z

It seems that sometimes IE8 on some system can have strange behaviour. I removed checking if the format panel is opened and left only checking styles if it's properly applied.

f1ames

LGTM!

Mateusz Samsel added 12 commits July 5, 2017 15:13

Manual test

788a8a0

Sample raw solution. Proof of concept.

d562cdb

Some minor changes to tests and fix.

e40b8b4

Improvement to fix - change statement check

90fbf7b

Fix failing font unit test.

311b25c

Extend test case with new options.

8b397f2

Improvement in description to manual tests.

c05ffc3

Add to common lang file.

eb881a2

Change fix to use common lang file.

31e1237

Fix for paragraph format.

53933d2

Add unit and manual tests to format menu.

28e5575

Little cosmetic change with removing unecessary comments.

cdf19ac

msamsel force-pushed the t/584 branch from dc665b3 to cdf19ac Compare July 6, 2017 13:35

f1ames suggested changes Jul 12, 2017

View reviewed changes

Fixings after review.

65cc8f6

f1ames suggested changes Jul 25, 2017

View reviewed changes

Another review fix pack

5f8f47d

f1ames suggested changes Jul 26, 2017

View reviewed changes

Review fixes

ef160fa

f1ames self-requested a review July 28, 2017 12:53

Fix spacing.

7d08d09

f1ames approved these changes Jul 31, 2017

View reviewed changes

Changelog entry. [skip ci]

18cb926

f1ames merged commit ef464ec into major Jul 31, 2017

f1ames deleted the t/584 branch July 31, 2017 07:49

f1ames mentioned this pull request Sep 14, 2017

Fonts setting should not be toggling options #584

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Font options are not toggleable any longer #615

Font options are not toggleable any longer #615

msamsel commented Jul 6, 2017 •

edited

Loading

f1ames left a comment

f1ames Jul 12, 2017

f1ames Jul 12, 2017

f1ames Jul 12, 2017

f1ames Jul 12, 2017

f1ames Jul 12, 2017

f1ames Jul 12, 2017

f1ames Jul 12, 2017

msamsel commented Jul 13, 2017

f1ames left a comment

f1ames Jul 25, 2017

f1ames Jul 25, 2017

f1ames Jul 25, 2017

f1ames Jul 25, 2017

f1ames Jul 25, 2017

f1ames Jul 25, 2017

f1ames Jul 25, 2017

f1ames Jul 25, 2017

f1ames Jul 25, 2017

f1ames Jul 25, 2017

f1ames left a comment

f1ames Jul 26, 2017

f1ames Jul 26, 2017

f1ames Jul 26, 2017

f1ames Jul 26, 2017

msamsel commented Jul 28, 2017

f1ames left a comment


		editor[ style.checkActive( elementPath, editor ) ? 'removeStyle' : 'applyStyle' ]( style );
		// Always aply style, do not allow on toggle it by clicking on list item (#584).


		Expected: Choosen format should remain this same all the time.

		Unexpected: Text format toggle between styled and unstyled (Normal) one.

Font options are not toggleable any longer #615

Font options are not toggleable any longer #615

Conversation

msamsel commented Jul 6, 2017 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Jul 13, 2017

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Jul 28, 2017

f1ames left a comment

Choose a reason for hiding this comment

msamsel commented Jul 6, 2017 •

edited

Loading