-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
Caption feature #1175
Caption feature #1175
Conversation
704aec8
to
fbbd35d
Compare
b314b68
to
ec49660
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests
There are some failing tests (same for rebased branch and t/932-2992_old
ba5d595).
All browsers
- http://tests.ckeditor.test:1030/tests/plugins/easyimage/uploadwidget (
test classic with easyimage (integration test)
for each editor type)
Edge
- http://tests.ckeditor.test:1030/tests/plugins/imagebase/features/caption#tests%2Fplugins%2Fimagebase%2Ffeatures%2Fcaption%20test%20toggling%20caption%20(one%20widget%20with%20empty%20caption%20that%20gets%20filled)%20(classic)
- http://tests.ckeditor.test:1030/tests/plugins/imagebase/features/caption#tests%2Fplugins%2Fimagebase%2Ffeatures%2Fcaption%20test%20toggling%20caption%20(one%20widget%20with%20non-empty%20caption%20that%20gets%20emptied)%20(classic)
IE11
Also 4 tests are failing on IE11 in the http://tests.ckeditor.test:1030/tests/plugins/imagebase/features/caption.
General Feedback
First of all: I'm pleased to see how well the existing widget feature concept worked for this case.
Back to the solution: well the concept is good, in a way that in general it works for the end user as expected.
But I'm not happy with approach of 'selectionChange'
listener based solution. This event fires very, very often, on top of that you're iterating over each widget instance where you use getFocusedWidget()
.
Instead I would prefer to make as little footprint of this feature as possible.
The first issue you've been dealing with is that when focusing nested editable, the widget itself does not say it's focused. That's why you're iterating over every widget, and using widgetRepository.getByElement()
.
Instead I think much easier solution would be to mark the active (unrolled) caption area with an attribute, say data-cke-caption-active
, and put it either on caption or a widget wrapper. As you move to another caption and call addPlaceholder()
(or alike) this function could do a check in editor.editable()
for a widget with data-cke-caption-active
and hide the caption in that widget. It should also check if, by any chance, it's the same widget, in which case... the placeholder is already visible, and there's no reason to hide it (it would cause a redundant reflow).
With the above you would do a single widgetRepo.getByElement()
check every selectionChange
which seems to be a better trade.
More Performance-friendly Solution
There is more efficient solution that came to my mind at first, but after giving it a longer thought I find it to be overly complex in implementation.
The idea is to use focusin
to detect nested editable focus and selectionChange
to detect main widget focus (checking widgets.focused
).
But here we end up with two listeners, with a noticeably different logic (captions would be matched based on attribute, widgets based on implementing widget feature).
I think that above is enough to say that I no longer find it feasible solution.
Other
If getData()
is called while the figure caption placeholder is shown, it will include placeholder text in returned value. I have added an unit test for that in ed24161.
In case you'd need some clarifications regarding this feedback - please let me know.
plugins/imagebase/plugin.js
Outdated
var featuresDefinitions = { | ||
caption: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to have quite a few helper functions in a minute over here. Now we gained createCaption
, isEmptyOrHasPlaceholder
, addPlaceholder
. I'd like to have it contained within a separate closure, simply sth like getCaptionFeature
.
plugins/imagebase/plugin.js
Outdated
* @member CKEDITOR.plugins.imagebase.featuresDefinitions.caption | ||
* @param {CKEDITOR.dom.element} sender Element, which triggered `selectionChange` event | ||
*/ | ||
_toggleCaption: function( sender ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_toggleCaption
is not a good function in this case, it works pretty much like a refresh function. "Toggle" implies that the state is going to be changed which is not the case.
plugins/imagebase/plugin.js
Outdated
i; | ||
|
||
for ( i in widgets ) { | ||
if ( widgets[ i ].features && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
widgets[ i ].features && CKEDITOR.tools.array.indexOf( widgets[ i ].features, 'caption' )
should get extracted to a function like widgetHasFeature()
as it's also used here:
function isLinkable( widget ) {
return widget && widget.features && CKEDITOR.tools.array.indexOf( widget.features, 'link' ) !== -1;
}
plugins/imagebase/plugin.js
Outdated
function createCaption( widget ) { | ||
var element = widget.element, | ||
caption = element.getDocument().createElement( 'figcaption' ), | ||
definition = widget.editor.widgets.registered[ widget.name ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Widget instance has a definition property directly in it.
plugins/imagebase/plugin.js
Outdated
@@ -44,7 +66,84 @@ | |||
return CKEDITOR.plugins.link.parseLinkAttributes( widget.editor, widget.parts.link ); | |||
} | |||
|
|||
function createCaption( widget ) { | |||
var element = widget.element, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should integrate it with ACF. I think that in case 'figcaption' is not allowed we should simply not create caption
part. In turn caption widget feature should not break anything (while ofc not adding any functionality).
plugins/imagebase/plugin.js
Outdated
|
||
function isEmptyOrHasPlaceholder( widget ) { | ||
|
||
return !widget.editables.caption.getData() || widget.parts.caption.hasAttribute( 'data-cke-placeholder' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dom.element
has a data
method that will simplify it for you.
plugins/imagebase/plugin.js
Outdated
editable = this.editables.caption; | ||
|
||
if ( isFocused ) { | ||
caption.removeAttribute( 'data-cke-hidden' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This attribute should be removed after the placeholder was changed. This would mean less reflows which is always a good thing 👍
I'm still investigating failing tests in IE and Edge. Something fishy's going there as manual test for the same case is working just fine (probably artificial selection change is not working). I also don't know how to deal with |
I've pushed Tests in IE and Edge are still failing. it seems that these browsers don't fire |
I've pushed improvement for tests both on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see one more issue:
Provide detailed reproduction steps (if any)
- Open Easy Image sample.
- Select whole "Saturn V carrying Apollo 11" caption and remove it.
- Put the selection outside of the widget.
Expected result
Caption should be hidden, as it is with newly inserted Easy Image widgets.
Actual result
Caption remains visible.
Other details
- Browser: Any
- OS: Any
Please provide unit tests for that case.
I've pushed fix to both of branches. |
I have a feeling that this is an edge case, but I'll put it here for my future reference, so that it can be extracted to a separate issue. Provide detailed reproduction steps (if any)
Expected resultWidget wrapper gets highlighted with yellow outline. Actual resultWidget wrapper is highlighted, but weird reflows are taking place. CKEditor Easy Image Sample - Google Chrome 1_11_2018 5_48_23 PM.zip Other details
HIDPI scaling: 250% I wasn't able to repro it on other browsers, but I think to very specific layouting in this case (the cause for it might be that widget drag handler is affecting DOM - scroll bar appears, the width changes and reflow occurs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally CSS approach is the best idea here, so please merge this branch to t/932-2992
and continue with it.
I have found 2 issues (+1 kinda upstreamish, I'm talking aboud drag and drop one).
There are also some problems in unit tests, which is likely the cause for unit tests to fail in IE11.
plugins/imagebase/plugin.js
Outdated
widget.editables.caption.setData( '' ); | ||
} | ||
|
||
function toggleVisibility( caption, isVisible ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toggleVisibility
is not a good name, it implies that it will toggle visibility based on current state - however you require providing the visibility explicitly. So it should be setVisibility
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that you don't like word "toggle". Every time you demand to change it to something else :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, I like toggle methods, but only when it actually toggles something 😉
plugins/imagebase/plugin.js
Outdated
} | ||
|
||
function toggleVisibility( caption, isVisible ) { | ||
caption.data( 'cke-active', isVisible ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your data attributes should feature caption in its name, otherwise you don't know what the attribute is related to.
So I suggest:
data-cke-caption-active
data-cke-caption-hidden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also renamed data-cke-placeholder
to data-cke-caption-placeholder
for consistency.
@@ -0,0 +1,11 @@ | |||
@bender-tags: 4.8.0, feature, 932 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, correct the target to 4.9.0 version.
@@ -0,0 +1,34 @@ | |||
<div id="classic"> | |||
<p>Widget with caption:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a widget without a caption.
plugins/easyimage/plugin.js
Outdated
@@ -198,6 +198,8 @@ | |||
widgetDefinition = CKEDITOR.plugins.imagebase.addFeature( editor, 'link', widgetDefinition ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have noticed there's also a problem with drag and drop. But it should be fixed by TP#3351
. Here are the steps:
- Focus a Easy Image widget with no caption text (so that it has a placeholder text).
- Drag and drop the widget into another position.
Expected
Caption box is visible with an empty caption placeholder text.
Actual
Caption is hidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After I rebased it onto latest t/932-2961
branch - that brought Balloon Panel, and it has an issue:
Balloon positioning
-
Focus Easy Image widget editor.
-
Put a collapsed selection inside of the caption.
-
Move focus back to widget (by clicking anywhere on the image).
Expected
Balloon panel shows, again in the same position.
Actual
Balloon panel points to the bottom edge of an image, overlapping the caption.
I have fixed this issue in #1448.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's other issue related to blurring:
- Focus a EI widget with no caption, so that placeholder text appears.
- Put the focus outside of the editor.
Expected
Caption is removed/hidden.
Actual
Caption + placeholder remains visible.
Added manual test for that in tests\plugins\imagebase\features\manual\captionblur.md test.
}; | ||
} | ||
|
||
function createMultipleToggleTest( options ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add at least minimal comment to what the function does/what is its purpose. Currently one has to read its source code to understand this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, review this assertion. It's a cause for 'test toggling captions (two widgets with empty captions)'
to fail on IE11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mlewand which assertion exactly are you thinking of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was referring to createMultipleToggleTest
helper, sorry for the confusion.
} | ||
|
||
bot.setData( getFixture( options.fixture ), function() { | ||
var widgets = getWidgets( editor, options.widgetsCount ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be a better idea to use widely available API? just go with editor.widgets.instances.slice()
instead of adding new redundant code.
} | ||
|
||
function assertOnFocus( widgets, expected ) { | ||
forEach( widgets, function( widget, i ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In reality this code will never execute assertions for more than 1 widget. In YUI wait
throws an exception, so the flow is never resumed afterwards.
@@ -0,0 +1,34 @@ | |||
<div id="classic"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add inline editors. You have added floatingspace
plugin in .md
file but you did not use it with any editor.
@Mgsy can I ask you for help with testing this feature? |
Steps to reproduce
Current resultPlaceholder doesn't disappear. Expected resultPlaceholder disappears. Other informationOS: Windows 10, MacOS X |
Steps to reproduce
Current resultImage has been pasted, but editor has crashed. Expected resultImage is pasted without errors. Error
Other informationOS: Windows 10, MacOS X |
Steps to reproduce
Current resultCaption disappears and selection moves to the previous block. Expected resultSelection stays inside the caption and Other informationOS: Windows 10, MacOS X |
Steps to reproduce
Current resultCaption disappears. Expected resultCaption is visible and selection stays inside the empty caption. Other informationOS: Windows 10, MacOS X |
Steps to reproduce
Current resultSecond image's empty caption is visible. Expected resultSecond image's empty caption hides. Other informationOS: Windows 10, MacOS X |
Expected resultWidget is focused, caption placeholder is visible. Actual resultWidget is focused but there's no caption placeholder visible. Other details
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have found mainly a polishing tasks, we need to make it production ready.
Also please add proper namespace for catpion feature - just mark it as a private member. I have proposed syntax for that in #1483, see docs for getUploadFeature
in plugins/imagebase/plugin.js
file.
* during each step. Assertions are made for every widget in the group. | ||
* | ||
* @param {Object} options | ||
* @param {String} options.fixture Fixture's name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an id I think you meant it, but someone might get it wrong when it's referred as name.
* during each step. | ||
* | ||
* @param {Object} options | ||
* @param {String} options.fixture Fixture's name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It' an id I think you meant it, but someone might get it wrong when it's referred as name.
* @param {Boolean} options.initial Caption visibility at the start of the test. | ||
* @param {Boolean} options.focus Caption visibility after calling focus function. | ||
* @param {Boolean} options.blur Caption visibility after calling blur function. | ||
* @param {Boolean} options.onInit Callback to run at the beginning of the test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a function. Also this is an optional parameter.
* @param {Function} options.onFocus Callback to run after focus function. | ||
* @param {Function} options.onBlur Callback to run after blur function. | ||
* @param {Function} options.customFocus Function to be called instead of default focus function. | ||
* @param {Function} options.customBlur Function to be called instead of default blur function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onFocus onBlur customFocus and customBlur are all optional.
plugins/imagebase/plugin.js
Outdated
function listener( evt ) { | ||
var path = evt.name === 'blur' ? editor.elementPath() : evt.data.path, | ||
sender = path ? path.lastElement : null, | ||
widgets = editor.widgets, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
widgets
is used only in previous. I think it could be inlined there. It will get lengthy (115 chars) but it will still be under 120 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any 115 chars line, there is only 135 chars one, but it's not the previous one 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, wasn't accurate. What I meant is that widgets
variable is used only in line previous = widgets.getByElement( editor.editable(…)
then the rest is applicable.
plugins/imagebase/lang/en.js
Outdated
@@ -3,5 +3,5 @@ Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. | |||
For licensing, see LICENSE.md or http://ckeditor.com/license | |||
*/ | |||
CKEDITOR.plugins.setLang( 'imagebase', 'en', { | |||
captionPlaceholder: 'Caption' | |||
captionPlaceholder: 'Enter image caption' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no meta lang entry for this label.
plugins/imagebase/plugin.js
Outdated
}, | ||
|
||
/** | ||
* Method used inside {@link CKEDITOR.editor#event-selectionChange} event to decide if caption for given widget |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have tied this method strongly with the selectionChange
and now it uses also blur
. It's possible there will be different events soon. Please make the docs event-agnostic.
|
||
--- | ||
|
||
Repeat above steps for all other editors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, mention that Edge users could be affected by #1458 - and experience focus quirks when focusing between the editors.
@@ -0,0 +1,92 @@ | |||
<h2>Classic editor</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have inline editors, please import CSS files, so that the widgets in inline editors are styled properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there aren't any styles that could be additionally loaded – all are already loaded. It looks differently than EI because it's a custom, test image widget:
plugin.addImageWidget( editor, 'testWidget', plugin.addFeature( editor, 'caption', {} ) );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it's a figure - I wanted figures to be styled consistently in all editors. It just requires importing contents.css
- see 289bd70.
@@ -0,0 +1,455 @@ | |||
/* bender-tags: editor,widget */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ignore this test in unspported environments.
I've fixed all raised issues, I've also rebased to the newest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I just added subtle corrections to manual tests.
There are still some bugs related to captions, but they're not big enough to stop this feature. Instead bugs will be extracted to a followup issues.
What is the purpose of this pull request?
New feature for new feature
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
What changes did you make?
I've added caption feature, which is responsible for toggling visibility of empty caption and for showing placeholder text.