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

Fix for: Dragging widget in read-only mode #865

Merged
merged 21 commits into from
Jul 10, 2019
Merged

Fix for: Dragging widget in read-only mode #865

merged 21 commits into from
Jul 10, 2019

Conversation

wojtekw92
Copy link
Contributor

@wojtekw92 wojtekw92 commented Sep 5, 2017 •
edited by Comandeer
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?

Check if editor is in read only mode.

Closes #808.
Closes #3260.

Comandeer
Comandeer requested changes Sep 11, 2017
View reviewed changes
Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

Fix seems to be working correctly, but I'm lacking proper unit tests. Please see also other issues with tests.

plugins/clipboard/plugin.js Outdated
@@ -1388,6 +1388,11 @@
// Cancel native drop.
evt.data.preventDefault();

// #808
Copy link
Member

Choose a reason for hiding this comment

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

Please, elaborate a little bit more.

tests/tickets/gh808/manual/1.md Outdated
@@ -0,0 +1,11 @@
@bender-tags: 4.7.3, bug, 808
Copy link
Member

Choose a reason for hiding this comment

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

This test could be moved to other clipboard tests.

Comandeer
Comandeer requested changes Dec 12, 2017
View reviewed changes
Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

I've changed test's name and improvement formatting of comment in code.

However unit test is failing in IE8.

@mlewand 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
@mlewand
Copy link
Contributor

mlewand commented Feb 14, 2019

@engineering-this please take over this PR

@engineering-this engineering-this self-assigned this Feb 14, 2019
@engineering-this engineering-this changed the title check if editor is in read only mode in drop handler Fix for: Dragging widget in read-only mode Feb 15, 2019
@engineering-this
Copy link
Contributor

Pushed changes:

  • Fixed unit test failing on IE.
  • Hidden drag handler with CSS in readonly mode.
  • Updated manual test to reflect above.
  • Changelog 😉

@Comandeer Comandeer self-assigned this Feb 16, 2019
Comandeer
Comandeer requested changes Feb 16, 2019
View reviewed changes
plugins/widget/plugin.js Outdated
@@ -66,6 +62,16 @@
'.cke_widget_drag_handler_container:hover{' +
'opacity:1' +
'}' +
'.cke_widget_drag_handler_container_hidden{' +
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 introducing such class? It seems to be never applied to the widget's drag handler, causing funny bug when editor is loaded while mouse cursor is over place in which widget will appear:

Image widget with drag-handler attached to the bottom

Additionally I don't see a need to add such class. If display: none is a default style, it can be directly added to other styles of drag handler – no need of introducing another class.

plugins/widget/plugin.js Outdated
// widget's outline far to the left (https://dev.ckeditor.com/ticket/12024).
'display:none;' +
'}' +
'.cke_editable[contenteditable="false"] .cke_widget_drag_handler_container{' +
Copy link
Member

Choose a reason for hiding this comment

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

We can add some comment with ticket reference to this style.

plugins/widget/plugin.js Outdated
} );
this.dragHandlerContainer.removeClass( '.cke_widget_drag_handler_container_hidden' );
Copy link
Member

Choose a reason for hiding this comment

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

After removing this class this line won't be needed, but please note that removeClass takes class name without dot at the beginning.

@Comandeer Comandeer mentioned this pull request Feb 16, 2019
Closed
@engineering-this engineering-this self-assigned this Feb 18, 2019
@engineering-this
Copy link
Contributor

I've moved display:none from class to inline styles, so it can be removed at proper time.

@Comandeer Comandeer assigned Comandeer and unassigned Comandeer Feb 19, 2019
@Comandeer Comandeer self-assigned this Jul 10, 2019
@Comandeer Comandeer changed the base branch from master to major July 10, 2019 09:08
@Comandeer
Copy link
Member

Rebased onto latest major.

Comandeer
Comandeer approved these changes Jul 10, 2019
View reviewed changes
Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

LGTM!

@Comandeer Comandeer merged commit 3dd6e42 into major Jul 10, 2019
@CKEditorBot CKEditorBot deleted the t/808 branch July 10, 2019 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Comandeer Comandeer Comandeer approved these changes

Assignees
No one assigned
Labels
review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drag handler is visible in read-only mode Dragging and dropping content in read-only mode
4 participants
@wojtekw92 @mlewand @engineering-this @Comandeer