This repository has been archived by the owner on Jun 26, 2020. It is now read-only.
Added tabindex="0" to editable element to make it focusable in read-only mode
#540
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…in read-only mode.
Reinmar
reviewed
Feb 10, 2020
| console.error( err.stack ); | ||
| } ); | ||
|
|
||
| document.querySelector( '#btn-readonly' ).addEventListener( 'click', () => { |
There was a problem hiding this comment.
Looks like a problem with indentation here?
oleq
suggested changes
Feb 10, 2020
There was a problem hiding this comment.
I extended the test with more editors and more content.
Some remarks:
- I added links to the content. Unfortunately, now with this
tabindex="0"there's no way to reach a link in a read-only editor using tab. It also means you can't tab past it so generally speaking the entire flow of tab navigation is broken (it stops at the link). I thought it's a "Chrome thing" but FF acts in the same way so I guess this may be a problem on our side.
- Now when tabbing across the form, when entering the read–only editor, the inline and ballon toolbars show up (balloon, when there was previously a selection). They are full of disabled items, but this should not be a big deal. Maybe that's even better, as @Comandeer pointed out, we should not change the navigable structure (familiar to the users) by making the editor read–only.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Suggested merge commit message (convention)
Other: Added
tabindex="0"to editable element to make it focusable in read-only mode. Closes ckeditor/ckeditor5#6200.Additional information
I'd like this enhancement to be merged in iteration 29.
Three notes:
Tests. This will require thorough tests, some before merging and surely a lot during the testing phase. I didn't add too many tests in this PR, though. For unit tests, there's not a lot to test. When it comes to manual tests, we will have to keep this change in mind during the testing phase, so I am not sure if there's a need to make multiple/complex manual tests. I've added just one manual test that allows checking how the focus on Tab changes and how the editor behaves in read-only mode.
There's one important note about the editor's behavior in the read-only mode after this change. Balloon contextual toolbars are now visible. For example, when you click an image in the read-only mode, the toolbar for the image will appear. It will be all-disabled, but it will appear. I guess this is something that we might want to discuss.
I've added
tabindex="0"not only for the read-only mode but also for the normal mode. In theory, this should not have an impact on the editor's behavior in the normal mode. But if you are afraid, I can change that and bindtabindex="0"to the read-only mode state.If you want, I can try to come up with other things in the manual test (or expand the description) but again, since this is a change can affect multiple features in the read-only mode, I think that we should simply keep this change in mind during the testing phase.