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.

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

Fix for: Colordialog is incorrectly positioned when used with another dialog #3572

Merged
merged 19 commits into from
Nov 12, 2019

Conversation

Dumluregn
Copy link
Contributor

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 is the proposed changelog entry for this pull request?

[#3559](https://github.com/ckeditor/ckeditor4/issues/3559): Fix for: [Color Dialog](https://ckeditor.com/cke4/addon/colordialog) is incorrectly positioned when used with another dialog.

What changes did you make?

After the fix cell properties dialog is properly fading when color dialog is opened.

Closes #3559.

@Dumluregn Dumluregn changed the base branch from major to master October 11, 2019 13:49
@jacekbogdanski jacekbogdanski self-assigned this Oct 24, 2019
Copy link
Member

@jacekbogdanski jacekbogdanski left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor stuff.

plugins/dialog/plugin.js Outdated Show resolved Hide resolved
plugins/tabletools/dialogs/tableCell.js Show resolved Hide resolved
plugins/tabletools/plugin.js Show resolved Hide resolved
tests/plugins/dialog/positioning.js Outdated Show resolved Hide resolved
tests/plugins/dialog/positioning.js Outdated Show resolved Hide resolved
tests/plugins/tabletools/manual/colordialogposition.md Outdated Show resolved Hide resolved
tests/plugins/tabletools/manual/colordialogposition.md Outdated Show resolved Hide resolved
tests/plugins/tabletools/manual/colordialogposition.md Outdated Show resolved Hide resolved
</div>

<script>
if ( bender.tools.env.mobile ) {
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to also add manual test for mobiles, as the regression which has been introduced here is caused by changes dedicated to mobiles. I think that if we change test steps with dev console, it should be possible to do it in the same test file, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, should work 👍

Copy link
Member

@jacekbogdanski jacekbogdanski left a comment

Choose a reason for hiding this comment

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

Only small issue with manual test. It looks like when you open it on some slow machine (e.g. you could check it on IE or Browser Stack mobiles) it may show dialogs in incorrect order once button clicked. It only occurs on the first clean page load and first button click, probably due some caching issue / asynchronous nature of commands.

The fix will probably require chaining commands to make sure that the previous finished or even replacing commands with editor.openDialog, as commands may also open dialogs asynchronously.

@Dumluregn
Copy link
Contributor Author

Dumluregn commented Nov 7, 2019

I can fix it but I wonder if it's really an issue - more important than dialogs order is the fact that the one on top is accessible, when it's closed the second one becomes active etc. So maybe instead of messing around with commands we could change Expected to sth like "all dialogs close and activate properly" instead, WDYT?

@jacekbogdanski
Copy link
Member

I have a feeling that order is important here, note that you actually update code around logic responsible for correct dialog order, which may be messed up if z-index is incorrectly updated (on the wrong element). I'm wondering if it won't be simpler to update the test with 3 buttons (all different dialogs) with some crazy high z-index so they are above dialog. Not sure how it will look/being accessible.

@Dumluregn
Copy link
Contributor Author

Dumluregn commented Nov 11, 2019

The bug you noticed here has occured before, so I reported it in #3638 to handle it separately if we decide it's important. For now when you click the same button in test more than once it will fail the test, so I included a note to click each one only once.

@jacekbogdanski jacekbogdanski self-assigned this Nov 12, 2019
Copy link
Member

@jacekbogdanski jacekbogdanski left a comment

Choose a reason for hiding this comment

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

LGTM 👍

tests/plugins/tabletools/manual/colordialogposition.html Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants