Long image alt text protrudes into editor when image is selected #1435

engineering-this · 2018-01-10T14:58:51Z

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

I've added following styling to hiddenSelection: width:0;height:0

Closes #898

jacekbogdanski

The fix seems to work. However, this PR needs some polishing. Please rebase it into the latest upstream.

There is also a missing unit test.

jacekbogdanski · 2018-07-18T10:14:37Z

tests/plugins/image2/manual/longalt.html

@@ -0,0 +1,7 @@
+<div id="editor" contenteditable="true">


There is no need for conteneditable attribute.

jacekbogdanski · 2018-07-18T10:18:46Z

tests/plugins/image2/manual/longalt.md

+@bender-ui: collapsed
+@bender-ckeditor-plugins: wysiwygarea, image2
+
+## Image with long alt


Please, refactor this file into our adopted test file structure.

jacekbogdanski · 2018-07-18T10:20:53Z

CHANGES.md

@@ -16,6 +16,7 @@ Fixed Issues:
 * [#889](/~https://github.com/ckeditor/ckeditor-dev/issues/889): Fixed: Unclear error message for width and height fields in [Image](https://ckeditor.com/cke4/addon/image) and [Enhanced Image](https://ckeditor.com/cke4/addon/image2) plugins.
 * [#859](/~https://github.com/ckeditor/ckeditor-dev/issues/859): Fixed: Can't edit link after double click on text in link.
 * [#1013](/~https://github.com/ckeditor/ckeditor-dev/issues/1013): Fixed: [Paste from Word](https://ckeditor.com/cke4/addon/pastefromword) does not work correctly with [`config.forcePasteAsPlainText`](https://docs.ckeditor.com/ckeditor4/docs/#!/api/CKEDITOR.config-cfg-forcePasteAsPlainText) property.
+* [#898](/~https://github.com/ckeditor/ckeditor-dev/issues/898): Fixed: Long image alt text protrudes into editor when image is selected.


Update changelog to target upstream minor.

jacekbogdanski

Originally the issue exists inside a fake selection code, not image2. I think that it would be nice to move tests into correct namespace i.e core/selection/fake and refactor test case names thus it's not about invalid alt hiding, but fake selection container.

jacekbogdanski · 2018-07-27T09:28:25Z

CHANGES.md

@@ -11,6 +11,7 @@ Fixed Issues:
 * [#2167](/~https://github.com/ckeditor/ckeditor-dev/issues/2167): Fixed: Matching in [Emoji](https://ckeditor.com/cke4/addon/emoji) plugin is not case insensitive.
 * [#1084](/~https://github.com/ckeditor/ckeditor-dev/issues/1084) Fixed: Using the "Automatic" option with [Color Button](https://ckeditor.com/cke4/addon/colorbutton) on a text with color already defined sets an invalid color value.
 * [#966](/~https://github.com/ckeditor/ckeditor-dev/issues/966): Fixed: Executing [`editor.destroy()`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_editor.html#destroy) during [file upload](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_fileTools_uploadWidgetDefinition.html#onUploading) throws error. Thanks to [Maksim Makarevich](/~https://github.com/MaksimMakarevich)!
+* [#898](/~https://github.com/ckeditor/ckeditor-dev/issues/898): Fixed: Long image alt text protrudes into editor when image is selected.


Add Image reference.

I agree about moving it to proper namespace, but also I want to clarify, that it is unrelated to fake selection. It is related to hidden selection, which refers to hidden elements which will be copied to clipboard on users copy action. In our case it is an alt text.

Please, see where hideSelection function is used. In a unit testing level, it's used inside fake selection and should be tested in this namespace.

jacekbogdanski

Is there any reason for separate unit test file? I like the idea to keep clean namespaces, although it's single test case and we already have hidden container unit tests inside tests/core/selection/fake.js.

jacekbogdanski · 2018-08-28T09:42:17Z

tests/core/selection/hiddenselcontainer.js

+			} else {
+				// Other browers should have 0 with and 0 height.
+				assert.areEqual( '0px', altContainer.getStyle( 'height' ) );
+				assert.areEqual( '0px', altContainer.getStyle( 'width' ) );


I know that you just added width and height styles but it would be nice to test full style position:fixed;top:0;left:-1000px;width:0;height:0 in such test.

jacekbogdanski

Looks fine, some minor issues which I will fix in the upcoming commits.

jacekbogdanski · 2018-09-03T09:02:04Z

tests/core/selection/fake.js

+				};
+
+				for ( item in expected ) {
+					assert.areEqual( expected[ item ], styles[ item ] );


It may be simplified using CKEDITOR.tools.objectCompare.

jacekbogdanski · 2018-09-03T09:02:34Z

tests/core/selection/manual/hiddenselcontaiener.md

@@ -0,0 +1,11 @@
+@bender-tags: 4.10.2, bug, 898


Test file typo.

jacekbogdanski · 2018-09-03T09:06:32Z

tests/core/selection/fake.js

+
+			altContainer = editor.editable().findOne( '[data-cke-hidden-sel]' );
+
+			// On IE and Edge < 14 element should have `display:none`.


Not essentially true, this code path is for IE only. In fact, this comment is useless thus it explain code, not business reasons.

jacekbogdanski

LGTM

Comandeer

Additionally manual test could have better name (e.g. hiddenselvisibility).

Comandeer · 2018-09-25T14:16:15Z

CHANGES.md

@@ -18,6 +18,7 @@ Fixed Issues:
 API Changes:

 * [#1451](/~https://github.com/ckeditor/ckeditor-dev/issues/1451): Fixed: Context menu is incorrectly positioned when opened with `Shift-F10`.
+* [#898](/~https://github.com/ckeditor/ckeditor-dev/issues/898): Fixed: [Image2](https://ckeditor.com/cke4/addon/image2) long alt text protrudes into editor when image is selected.


It's not API change, entry should be in "Fixed Issues" section.

Additionally the name of the plugin is "Enhanced Image", not "Image2".

Comandeer · 2018-09-25T14:16:53Z

tests/core/selection/fake.js

+	},
+
+	// (#898)
+	'Test image long alt visibility': function() {


Actually test does not check alt visibility, but styles for hidden selection container.

Comandeer · 2018-09-25T14:18:32Z

tests/core/selection/fake.js

+
+			editor.getSelection().fake( editor.document.getById( 'bar' ), '<i>foo</i>' );
+
+			altContainer = editor.editable().findOne( '[data-cke-hidden-sel]' );


altContainer variable name is misleading, as there isn't any [alt] in it during the test.

Comandeer · 2018-09-25T14:19:14Z

tests/core/selection/fake.js

+			if ( CKEDITOR.env.ie && CKEDITOR.env.version < 14 ) {
+				assert.areEqual( 'none', altContainer.getStyle( 'display' ) );
+			} else {
+				styles = CKEDITOR.tools.parseCssText( altContainer.getAttributes().style );


It's safer to check computed styles of the container, not the ones in [style] attribute as they're still possible to overwrite.

Comandeer · 2018-09-25T14:20:57Z

tests/core/selection/manual/hiddenselcontainer.md

+
+There is a visible text around the image.
+
+# Test steps for browsers supporting JAWS text reader:


We only support Firefox + JAWS combo.

Comandeer

LGTM!

mlewand added the review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer. label Jul 18, 2018

jacekbogdanski self-requested a review July 18, 2018 10:00

jacekbogdanski self-assigned this Jul 18, 2018

jacekbogdanski requested changes Jul 18, 2018

View reviewed changes

jacekbogdanski assigned engineering-this and unassigned jacekbogdanski Jul 18, 2018

engineering-this force-pushed the t/898 branch from f991517 to 5892550 Compare July 24, 2018 10:16

engineering-this requested a review from jacekbogdanski July 24, 2018 11:00

jacekbogdanski requested changes Jul 27, 2018

View reviewed changes

engineering-this force-pushed the t/898 branch from 917169d to 028f922 Compare August 17, 2018 14:04

engineering-this requested a review from jacekbogdanski August 17, 2018 14:05

CKEditorBot unassigned engineering-this Aug 17, 2018

jacekbogdanski self-assigned this Aug 28, 2018

jacekbogdanski requested changes Aug 28, 2018

View reviewed changes

CKEditorBot unassigned jacekbogdanski Aug 28, 2018

engineering-this force-pushed the t/898 branch 2 times, most recently from 8ae1821 to f0f3a3d Compare August 31, 2018 15:34

engineering-this self-assigned this Aug 31, 2018

engineering-this requested a review from jacekbogdanski August 31, 2018 15:37

CKEditorBot unassigned engineering-this Aug 31, 2018

engineering-this changed the base branch from master to next August 31, 2018 15:39

jacekbogdanski self-assigned this Sep 3, 2018

jacekbogdanski requested changes Sep 3, 2018

View reviewed changes

CKEditorBot unassigned jacekbogdanski Sep 3, 2018

jacekbogdanski self-assigned this Sep 3, 2018

jacekbogdanski force-pushed the t/898 branch from b65feb6 to f0adfb2 Compare September 3, 2018 09:15

jacekbogdanski approved these changes Sep 3, 2018

View reviewed changes

CKEditorBot unassigned jacekbogdanski Sep 3, 2018

Comandeer self-requested a review September 3, 2018 16:51

mlewand closed this Sep 12, 2018

engineering-this and others added 9 commits September 21, 2018 09:55

git push Moved TC, asserting all styles

e5d4773

typo

35e142f

Updated version tag

292fdce

Fixed manual test file typo.

df6a216

Corrected unit test name.

8410c2e

Refactored tests and fixed styling.

b0cfe8a

Fixed manual test grammar.

6445fa5

Added overflow hidden to hidden selection container.

bb88ee4

Tests: test overflow style.

cdd7c9f

engineering-this force-pushed the t/898 branch from e191db8 to cdd7c9f Compare September 21, 2018 08:02

Tests: added test steps for JAWS.

100a642

engineering-this requested a review from Comandeer September 21, 2018 09:34

CKEditorBot unassigned engineering-this Sep 21, 2018

Comandeer self-assigned this Sep 25, 2018

Comandeer requested changes Sep 25, 2018

View reviewed changes

CKEditorBot unassigned Comandeer Sep 25, 2018

engineering-this self-assigned this Sep 27, 2018

engineering-this added 5 commits September 27, 2018 14:29

Changelog: Moved changelog entry and corrected plugin name.

b6524ad

Tests: renamed TC.

8d3927c

Tests: reworked assertion.

feb9509

Tests: updated test step.

12b0c86

Tests: renamed variable.

2265b65

engineering-this requested a review from Comandeer September 27, 2018 12:49

CKEditorBot unassigned engineering-this Sep 27, 2018

Comandeer self-assigned this Sep 27, 2018

Fix typo in changelog entry.

d4ad381

Comandeer approved these changes Sep 27, 2018

View reviewed changes

CKEditorBot unassigned Comandeer Sep 27, 2018

Comandeer merged commit 9bd2abc into master Sep 27, 2018

CKEditorBot deleted the t/898 branch September 27, 2018 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Long image alt text protrudes into editor when image is selected #1435

Long image alt text protrudes into editor when image is selected #1435

engineering-this commented Jan 10, 2018 •

edited

Loading

jacekbogdanski left a comment

jacekbogdanski Jul 18, 2018

jacekbogdanski Jul 18, 2018

jacekbogdanski Jul 18, 2018

jacekbogdanski left a comment

jacekbogdanski Jul 27, 2018

engineering-this Aug 17, 2018

jacekbogdanski Aug 28, 2018

jacekbogdanski left a comment

jacekbogdanski Aug 28, 2018

jacekbogdanski left a comment

jacekbogdanski Sep 3, 2018

jacekbogdanski Sep 3, 2018

jacekbogdanski Sep 3, 2018

jacekbogdanski left a comment

Comandeer left a comment

Comandeer Sep 25, 2018

Comandeer Sep 25, 2018

Comandeer Sep 25, 2018

Comandeer Sep 25, 2018

Comandeer Sep 25, 2018

Comandeer left a comment


		altContainer = editor.editable().findOne( '[data-cke-hidden-sel]' );

		// On IE and Edge < 14 element should have `display:none`.


		editor.getSelection().fake( editor.document.getById( 'bar' ), '<i>foo</i>' );

		altContainer = editor.editable().findOne( '[data-cke-hidden-sel]' );


		There is a visible text around the image.

		# Test steps for browsers supporting JAWS text reader:

Long image alt text protrudes into editor when image is selected #1435

Long image alt text protrudes into editor when image is selected #1435

Conversation

engineering-this commented Jan 10, 2018 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

jacekbogdanski 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

jacekbogdanski 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

jacekbogdanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacekbogdanski 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

jacekbogdanski left a comment

Choose a reason for hiding this comment

Comandeer 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

Comandeer left a comment

Choose a reason for hiding this comment

engineering-this commented Jan 10, 2018 •

edited

Loading