Opened 4 years ago

Last modified 2 years ago

#13168 review_failed Bug

Impossible to navigate to editable from toolbar by keyboard and vice versa.

Reported by: Artur Delura Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.7.1
Component: Accessibility Version:
Keywords: Cc:

Description (last modified by Artur Delura)

  1. Open editor page perhaps http://sdk.ckeditor.com/samples/accessibility.html.
  2. Focus on editable.
  3. Press Alt + F10 to navigate to toolbar.
  4. Press Alt + F11 to navigate to elements path.

Result: You can't navigate to elements path.

Expected Result: You can navigate to elements path.

It should be possible to navigate to elements path right froom toolbar and vice versa.

Change History (25)

comment:1 Changed 4 years ago by Artur Delura

Description: modified (diff)

comment:2 Changed 4 years ago by Piotrek Koszuliński

Component: GeneralAccessibility
Status: newconfirmed

comment:3 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7

comment:4 Changed 3 years ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

comment:5 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7CKEditor 4.5.8

comment:6 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:7 Changed 3 years ago by Tade0

Status: assignedreview

This didn't work, because the keystroke handler was attached only to the editable body.

I attached it to the element's path bar and the toolbar.

Changes pushed to branch:t/13168.

comment:8 Changed 3 years ago by Piotrek Koszuliński

So what happens now when you press e.g. CTRL+B while having a focus in the toolbar/element path?

comment:9 Changed 3 years ago by Tade0

The same as if you'd simply press the button "Bold".

comment:10 Changed 3 years ago by Marek Lewandowski

Status: reviewreview_failed

This behavior is reasonable. The code looks fine, but I'm missing manual test for it.

It make sense to put a test for it into tests/core/manual/keystrokehandler as a general purpose manual test (without tc tag).

comment:11 Changed 3 years ago by Piotrek Koszuliński

The same as if you'd simply press the button "Bold".

What I meant was that it may happen that some features won't want to work with a focus in the toolbar, because it's not a usual situation. So +1 for a manual test.

comment:12 Changed 3 years ago by Tade0

Status: review_failedreview

Added manual test, rebased and fixed a minor bug that appeared when testing.

Changes pushed to branch:t/13168.

comment:13 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.6.0
Status: reviewreview_failed
  • We're still having some integration issues - I've added two more failing manual test cases. I'm sure there are couple more.
  • There are way to many plugins added in 1.md manual test.
  • 1.md is to little specific as for TC.
  • Still we need general-use manual test.

We need to take steps more cautiously, otherwise this change might backfire rather badly.

Design

  • Keys from outside editable (toolbar, elements path) must not fire editor#key event. This event is commonly used for in-editable operations.
  • Only keystrokes added with editor#setKeystroke with a special parameter should be handled in toolbar, elements path.
    • Currently editor#setKeystroke takse 2 parameters, we could an extra parameter based on following constants:
      • CKEDITOR.KEYSTROKE_TOOLBAR
      • CKEDITOR.KEYSTROKE_ELEMENTS_PATH
      • CKEDITOR.KEYSTROKE_EDITOR
      • CKEDITOR.KEYSTROKE_ALL

So that you could make a call like:

editor.setKeystroke( CKEDITOR.CTRL + 76 /*L*/, 'link', CKEDITOR.KEYSTROKE_ALL );
editor.setKeystroke( CKEDITOR.ALT + 122 /*F11*/, 'elementsPathFocus', CKEDITOR.KEYSTROKE_ALL );
editor.setKeystroke( CKEDITOR.CTRL + CKEDITOR.SHIFT + 76 /*L*/, 'myFancyCommand', CKEDITOR.KEYSTROKE_ELEMENTS_PATH | CKEDITOR.KEYSTROKE_TOOLBAR );

Because of increased risk, let's move it to major release.

comment:14 Changed 3 years ago by Tade0

Status: review_failedreview

Added context to keystrokes. The default context is KEYSTROKE_EDITOR, since this is how it worked before these changes.

Rebased with major, pushed to branch:t/13168.

comment:15 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.0CKEditor 4.6.1

comment:16 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1CKEditor 4.6.2

Moving to 4.6.2 minor release, as 4.6.1 is mostly about polishing 4.6.0.

comment:17 Changed 2 years ago by kkrzton

As target milestone changed from major to master, I rebased the branch on current master and pushed to branch:t/13168. The original branch was moved to branch:t/13168b for possible future references.

On the rebased branch I also removed 9c9776 commit which fixed jslint errors in core/editable.js. While fixing this file to remove jslint errors seems tempting I will omit it as an addition to any fix as it makes the diff unreadable and the whole fix much harder to understand... I know it should be done, I also encounter this issue few times, but I think the good idea will be to create separate ticket for this (which can then cherry-pick fixing commit from the original issue branch).

comment:18 Changed 2 years ago by kkrzton

Status: reviewreview_failed

There are few unit tests failing. You can see travis build: https://travis-ci.org/cksource/ckeditor-dev/builds/188515783. I run tests manually to recheck and the result was the same. Failing TCs:

tests/core/config/inline
tests/core/config/inline2
tests/core/editor/keystrokehandler
tests/core/selection/fake
tests/plugins/undo/change
tests/plugins/undo/keyboard
tests/plugins/widget/nestededitables
tests/plugins/widget/widgetsintegration

5669 passed / 23 failed in 10m 29s 581ms - 100%

To make sure it is not the rebase fault, I also checked the original branch (based on previous major). The tests results were the same.

I am not sure what is the exact reason of failing tests, maybe context is missing in some places or some events are canceled instead of propagating, but it have to be investigated in more detail.

comment:19 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.2CKEditor 4.7.0

comment:20 Changed 2 years ago by Tade0

Status: review_failedreview

Reason for the tests failing was that at least one method was not updated in accordance to the changes made in CKEDITOR.editor.setKeystroke. Some tests also suffered from the same problem.

Changes pushed to branch:t/13168.

comment:21 Changed 2 years ago by Tomasz Jakut

Status: reviewreview_failed

Case 1:

  1. Open http://tests.ckeditor.dev:1030/tests/tickets/13168/2.
  2. Click widget.
  3. Press Alt+F10.
  4. Press Right Arrow.
  5. Move mouse.

Expected result: border on image is still there.

Actual result: border on image is gone.

Case 2:

Test http://tests.ckeditor.dev:1030/tests/tickets/13168/3 is failing: only spaces are inserted.

I've rebased ticket to the newest major and updated tags in tests. Pushed to branch:t/13168.

comment:22 Changed 2 years ago by Tade0

It took a while but I finally noticed the obvious:

To determine if a command should be executed the appropriate event is fired and the result of that operation is taken into consideration.

comment:23 Changed 2 years ago by Tade0

Status: review_failedreview

Change the keystroke handler behaviour so that in the editor context keystrokes are blacklisted, while in other context they are whitelisted.

Updated one manual test so that the changes are reflected.

Changes pushed to branch:t/13168.

comment:24 Changed 2 years ago by kkrzton

Status: reviewreview_failed

Added some docs improvements. Overall the solution looks pretty solid, however unit tests for new context parameter are missing, it will be good to add those.

comment:25 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.7.0CKEditor 4.7.1
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy