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

Jump to bottom

Integrate caption with basicstyles plugin #1559

Closed
wants to merge 14 commits into from
Closed

Integrate caption with basicstyles plugin #1559

wants to merge 14 commits into from

Conversation

Comandeer
Copy link
Member

@Comandeer Comandeer commented Jan 31, 2018 •
edited by mlewand
Loading

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 broaden check for caption (now it's check if an element is a caption or is inside a caption). I've also introduced mechanism to force treating widget as a focused one (via the second parameter to the _refreshCaption private method).

Closes #1646.

@Comandeer Comandeer force-pushed the t/932-2961 branch 2 times, most recently from 4a60cc1 to 748f4ed Compare February 6, 2018 10:22
@Comandeer Comandeer mentioned this pull request Feb 12, 2018
Open
@mlewand
Copy link
Contributor

mlewand commented Feb 12, 2018

Note this PR has to be rebased onto major, as EI feature has been already merged.

@f1ames f1ames self-requested a review February 16, 2018 11:11
@f1ames f1ames changed the base branch from t/932-2961 to major February 16, 2018 11:24
@f1ames
Copy link
Contributor

f1ames commented Feb 16, 2018

Rebased on major.

f1ames
f1ames suggested changes Feb 16, 2018
View reviewed changes
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

There is one test failing on Safari:
screen shot 2018-02-16 at 15 45 14

and three on IE11:
screen shot 2018-02-16 at 15 54 03

Also there are some TP issue references in the tests, maybe we should change them to github issue references (if it exists, if not it may be a good idea to create one).

plugins/imagebase/plugin.js Outdated
@@ -554,11 +554,18 @@
} );
}

// In Firefox and Edge applying styles inside caption results
// in firing this listener without focused widget.
// However it could be obtained from element, on which the event took place.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe issue reference could be useful here?

tests/plugins/imagebase/features/manual/captionbasicstyles.md Outdated
@@ -0,0 +1,24 @@
@bender-tags: 4.9.0, feature, 932, tp3384
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you update version tag to 4.9.1?

tests/plugins/imagebase/features/manual/captionbasicstyles.md Outdated
@@ -0,0 +1,24 @@
@bender-tags: 4.9.0, feature, 932, tp3384
@bender-ui: collapsed
@bender-ckeditor-plugins: sourcearea, wysiwygarea, toolbar, imagebase, link, htmlwriter, elementspath, basicstyles
Copy link
Contributor

Choose a reason for hiding this comment

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

Are link, htmlwriter, elementspath plugins needed here?

tests/plugins/imagebase/features/manual/captionbasicstyles.md Outdated

* Bold is applied.
* There is no visible placeholder.
* Firefox/Edge: Selection remains inside the caption.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it expected result for all browsers?

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant "check it especially in Firefox and Edge", but, yeah, does not make much sense.

plugins/imagebase/plugin.js Outdated
@@ -586,16 +593,26 @@
* @private
* @member CKEDITOR.plugins.imagebase.featuresDefinitions.caption
* @param {CKEDITOR.dom.element} sender The element that this function should be called on.
* @param {Boolean} [isFocused] Indicates if current widget should be treated as a focused one.
Copy link
Contributor

Choose a reason for hiding this comment

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

As it will land on different version than one in which imagebase plugin was introduced we should state here since which version this param is available.

@Comandeer Comandeer force-pushed the t/932-3384 branch 2 times, most recently from a023416 to 02a7834 Compare February 16, 2018 16:41
f1ames
f1ames suggested changes Feb 20, 2018
View reviewed changes
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

The manual test fails on Edge and IEs (except IE8 where test fails from different reason - see comments):

feb-20-2018 13-58-32

tests/plugins/imagebase/features/caption.js Outdated
@@ -435,6 +435,33 @@
onFocus: function( widget ) {
assert.isNotMatching( /Enter image caption/, widget.editor.getData() );
}
} ),

// #1646
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be // (#1646) - https://docs.ckeditor.com/ckeditor4/latest/guide/dev_tests.html#tagging-tests

tests/plugins/imagebase/features/manual/captionbasicstyles.html
</div>

<script>
var cfg = {
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 does not work (the divarea editor to be precise) on IE8. AFAIR, imagebase is not supposed to work on IE8 so it should be ignored probably.

@f1ames
Copy link
Contributor

f1ames commented Feb 20, 2018

As it now targets major and is regular bugfix now, please also add changelog entry.

@Comandeer Comandeer mentioned this pull request Mar 12, 2018
Open
@Comandeer
Copy link
Member Author

I've extracted IE & Edge issue into separate ticket, #1778, as it's reproducible from 4.5.0 in other widgets as well. I've also find one more issue, #1776.

@Comandeer
Copy link
Member Author

I've updated due to changes made in #1793.

This was referenced Aug 31, 2018
Closed
Open
@Comandeer Comandeer changed the base branch from major to master November 11, 2019 18:55
@Comandeer
Copy link
Member Author

Rebased onto latest master.

@Comandeer Comandeer added the pr:frozen ❄ The pull request on which work was suspended due to various reasons. label Jan 16, 2020
@Comandeer Comandeer closed this Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@f1ames f1ames f1ames requested changes

Assignees
No one assigned
Labels
pr:frozen ❄ The pull request on which work was suspended due to various reasons.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Comandeer @mlewand @f1ames