Set correct focus on tab in dialog when is clicked #3499

msamsel · 2019-09-25T13:02:09Z

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?

* [#3474](https://github.com/ckeditor/ckeditor4/issues/3474): Fixed: Incorrect focus order after tab in dialog navigation was clicked.
* [#3689](https://github.com/ckeditor/ckeditor4/issues/3689): Fixed: Cannot change dialog tabs focus with keyboard arrows after focusing with any tab with mouse click.

What changes did you make?

After clicking into the tab in navigation setup a proper focus and state.
There is also some clean up to prevent jshint errors.

Closes: #3474, closes: #3689

f1ames · 2019-11-21T12:23:48Z

Rebased onto latest master.

f1ames

Generally, the fix looks good - it slightly changes focusing behaviour, but from what I see it makes it more consistent. Some polishing in dialogTools still needs to be done.

plugins/dialog/plugin.js

tests/_benderjs/ckeditor/static/bot.js

f1ames · 2019-11-22T15:31:39Z

tests/_benderjs/ckeditor/static/bot.js

+					}, 0 );
+				} );
+
+				CKEDITOR.tools.setTimeout( function() {


This timeout will be still executed even after resolve is done. You should save store object which is returned by CKEDITOR.tools.setTimeout and cancel timeout execution before resolve.

Promises should be resilient to such cases, as a single promise cannot be resolved or rejected twice. But as we are using polyfills and cleaning up unnecessary functions is a good practice I add the cleaning up mechanism.

Yes, you can't resolve/reject promise more than one time or rejected already resolved promise. Still, it's good practise to cleanup code in such cases as above.

tests/plugins/dialog/_helpers/tools.js

tests/plugins/dialog/focus.js

tests/plugins/dialog/_helpers/tools.js

tests/plugins/dialog/focus.js

msamsel · 2019-11-28T15:31:01Z

I didn't rebase the branch to not lose the comments history. But I check it locally and it rebases without any issue.

f1ames

Submitting first review part (review still pending).

tests/_benderjs/ckeditor/static/bot.js

f1ames

I only did some minor refactoring in dialog tools. It looks quite impressive and the way how easily we can now test dialogs is really nice, good job 👍

Just some minor polishing in tests left (see #3499 (review)). Also I wonder, do you see in benefit in creating more integrational tests (so testing focus on some real plugins dialogs instead of test dialogs)?

msamsel · 2019-12-03T15:28:09Z

Also I wonder, do you see in benefit in creating more integrational tests (so testing focus on some real plugins dialogs instead of test dialogs)?

It's a great idea. I tried to do it within this PR, however, I noticed it would require to attach my custom focusChange event to all default dialogs. That's why it won't be super easy, however, it seems to be doable in a reasonable time. I extract this as a separate task, with a description what should be done and link it to this task.

msamsel · 2019-12-03T15:55:04Z

Helper improvements and extra unit tests are extracted to #3706.

… be confusing and replace it with comment in code.

…ly initialized and focus remain on cancel button. Provide config to the test editor to have always the same order of buttons regardles of OS.

Co-Authored-By: Krzysztof Krztoń <[email protected]>

…to it.

…les to use more types of fileds in dialogs.

Co-Authored-By: Krzysztof Krztoń <[email protected]>

f1ames · 2019-12-04T08:45:06Z

Rebased onto latest master.

f1ames

LGTM, good job 👍

msamsel force-pushed the t/3474 branch from ee41664 to 17594bd Compare September 27, 2019 08:19

msamsel changed the base branch from major to master September 27, 2019 15:01

msamsel force-pushed the t/3474 branch 2 times, most recently from 020b94c to cef63ba Compare October 1, 2019 10:51

msamsel marked this pull request as ready for review October 1, 2019 10:56

f1ames self-requested a review October 2, 2019 14:19

f1ames self-assigned this Oct 2, 2019

f1ames force-pushed the t/3474 branch from cef63ba to 1f370a6 Compare November 21, 2019 12:23

f1ames suggested changes Nov 25, 2019

View reviewed changes

CKEditorBot unassigned f1ames Nov 25, 2019

msamsel self-assigned this Nov 27, 2019

msamsel removed their assignment Nov 28, 2019

msamsel requested a review from f1ames November 28, 2019 15:31

f1ames self-assigned this Nov 29, 2019

f1ames reviewed Dec 2, 2019

View reviewed changes

f1ames suggested changes Dec 3, 2019

View reviewed changes

CKEditorBot unassigned f1ames Dec 3, 2019

msamsel mentioned this pull request Dec 3, 2019

Create helper and unit test for testing focus in dialogs #3706

Open

msamsel requested a review from f1ames December 4, 2019 08:15

f1ames self-assigned this Dec 4, 2019

Mateusz Samsel added 6 commits December 4, 2019 09:44

Set focus on tab when it is clicked.

d031589

Unify behvior of pressing F10 and clicking with mous into tab.

6234951

Fix jshint warnings.

ea88897

Add simple unit test with callbacks.

56016bb

Add promise based dialog tests and simple reworked unit test.

c484a11

Merge focus next and previous helpers.

d2fab9e

Mateusz Samsel and others added 23 commits December 4, 2019 09:44

Add 'use strict' to dialog plugin.

af9da54

Add bunch of multipage unit test together with helpers.

3b6d203

Add unit tests for dialogs with hidden tabs.

ff9240b

Extract helpers to separate file.

b725da1

Add description and improve test cases names.

9cd4c75

Improve description in helpers. Remove 'hasRejects' flag, as it might…

65bc012

… be confusing and replace it with comment in code.

Aff fix for test under IE11 - assert was fired when dialog wasn't ful…

02cd4e1

…ly initialized and focus remain on cancel button. Provide config to the test editor to have always the same order of buttons regardles of OS.

Add manual test to test focus behaviour.

f6d62aa

Update tests/plugins/dialog/_helpers/tools.js

cee9ada

Co-Authored-By: Krzysztof Krztoń <[email protected]>

Add cleaning up mechanism for timeoutes in asyncDialog.

53890da

Rename event to be coherent with editor's events.

adc3f0f

Rename function which changes focus and provide extended description …

d47a857

…to it.

Add more validation and checks in focusChange function. Improve examp…

2b59b57

…les to use more types of fileds in dialogs.

Move ie11 fix to async dialog.

52f0c00

Improve docs inside tests.

becf5fc

Remove timoutes when promise is resolved.

d68b37e

Rename more occurance of config to options.

46ac62e

Move dialog initialization to dialog tools.

e92e842

Add small comment improvements.

f7367e7

Tests tools refactoring.

f880777

Docs rewording. [skip ci]

da38a9e

Apply suggestions from code review

c9ece82

Co-Authored-By: Krzysztof Krztoń <[email protected]>

Remove unnecessary comment.

3664a11

f1ames force-pushed the t/3474 branch from 458b348 to 3664a11 Compare December 4, 2019 08:44

Changelog entry. [skip ci]

9eee5d4

f1ames approved these changes Dec 4, 2019

View reviewed changes

CKEditorBot unassigned f1ames Dec 4, 2019

f1ames merged commit 1ff8ee2 into master Dec 4, 2019

CKEditorBot deleted the t/3474 branch December 4, 2019 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set correct focus on tab in dialog when is clicked #3499

Set correct focus on tab in dialog when is clicked #3499

msamsel commented Sep 25, 2019 •

edited by f1ames

Loading

f1ames commented Nov 21, 2019

f1ames left a comment

f1ames Nov 22, 2019

msamsel Nov 28, 2019

f1ames Dec 3, 2019

msamsel commented Nov 28, 2019

f1ames left a comment

f1ames left a comment

msamsel commented Dec 3, 2019

msamsel commented Dec 3, 2019

f1ames commented Dec 4, 2019

f1ames left a comment

Set correct focus on tab in dialog when is clicked #3499

Set correct focus on tab in dialog when is clicked #3499

Conversation

msamsel commented Sep 25, 2019 • edited by f1ames Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What is the proposed changelog entry for this pull request?

What changes did you make?

f1ames commented Nov 21, 2019

f1ames left a comment

Choose a reason for hiding this comment

f1ames Nov 22, 2019

Choose a reason for hiding this comment

msamsel Nov 28, 2019

Choose a reason for hiding this comment

f1ames Dec 3, 2019

Choose a reason for hiding this comment

msamsel commented Nov 28, 2019

f1ames left a comment

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

msamsel commented Dec 3, 2019

msamsel commented Dec 3, 2019

f1ames commented Dec 4, 2019

f1ames left a comment

Choose a reason for hiding this comment

msamsel commented Sep 25, 2019 •

edited by f1ames

Loading