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

Should readonly editor be focusable? #6200

Open
scofalik opened this issue Feb 6, 2020 · 8 comments · May be fixed by ckeditor/ckeditor5-ui#540
Open

Should readonly editor be focusable? #6200

scofalik opened this issue Feb 6, 2020 · 8 comments · May be fixed by ckeditor/ckeditor5-ui#540
Labels
domain:accessibility This issue reports an accessibility problem. domain:ui/ux This issue reports a problem related to UI or UX. support:2 An issue reported by a commercially licensed client. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@scofalik
Copy link
Contributor

scofalik commented Feb 6, 2020

📝 Provide a description of the improvement

I recently stumbled upon one thing connected to editable focus in CKE5.

So, when the editor is in read only mode, it receives contenteditable=false which makes it not-focusable by the browser, as it doesn't have tabindex set. This messes up with some features that use editor's FocusTracker to operate (I mean focusing annotations for comments and track changes, in readonly mode).

This is not a critical issue for now, but I'd like us to think whether the editor should be or should not be focusable when it is in readonly mode. For me readonly and focusing are different properties. I mean, why should editor be not focusable if it is readonly? For example, for default browser fields, readonly=true fields are still focusable.

The easy fix around that is adding tabindex="-1" to the editable element when it is in readonly mode. This will not mess up tabbing (tabbing will behave the same as it did in both readonly and normal mode). But it will enable clicking into markers to highlight annotations. However, this is the easy fix solution. Maybe there's more to that issue.

@oleq prepared a fiddle you can play with: https://jsfiddle.net/oleq/j6vdsx0q/5/
@scofalik scofalik added the type:improvement This issue reports a possible enhancement of an existing feature. label Feb 6, 2020
@oleq
Copy link
Member

oleq commented Feb 6, 2020

Generally speaking, I’m OK with the solution (tabindex="-1" when read–only). There are some things that worry me and must be checked first:

  1. Could it affect a11y? I checked tabbing across the form (jsFiddle) and it looks OK (tab ignores the editable) but there could be some aspect I didn't check that matters.
  2. I see that the WidgetToolbarRepository listens to the global focus tracker and updates its state (maybe some other features too?). I hope this will not cause some issues like toolbar popping up despite the editor being read–only.

@oleq oleq added domain:ui/ux This issue reports a problem related to UI or UX. domain:accessibility This issue reports an accessibility problem. labels Feb 6, 2020
@scofalik
Copy link
Contributor Author

scofalik commented Feb 6, 2020

As @Comandeer noted on Slack discussion, tabindex="0" is a better solution and AFAICS, it could be even set no matter if the editable is in readonly or not.

@oleq
Copy link
Member

oleq commented Feb 6, 2020 •
edited
Loading

tabindex="0"

It means the editable will be included in the tab navigation even if read-only. We've never done this before (ATM we're compliant with v4 in that matter). Is this the right a11y pattern @Comandeer? Are there some recommendations telling us this is the right thing to do?

For the record: I'm not saying no. Just making sure this is the right approach.

@Comandeer
Copy link
Member

Are there some recommendations telling us this is the right thing to do?

As a read-only element, it should be marked up with [aria-readonly] attribute. Its definition states:

Readonly elements are relevant to the user, and application authors SHOULD NOT restrict navigation to the element or its focusable descendants.

If the editor itself is correctly marked (as e.g. [role=textbox]), then making it focusable in read-only mode should be working fine.

@mlewand
Copy link
Contributor

mlewand commented Feb 6, 2020

From end-user point of view even readonly field should still be focusable. It's even a nice UX addition if you can still manipulate selection inside (so you can just tab into it, do a ctrl/cmd+a and copy something from it).

@Reinmar Reinmar modified the milestones: iteration 29, nice-to-have Feb 6, 2020
@Reinmar
Copy link
Member

Reinmar commented Feb 6, 2020

For me, it's 👍 conceptually. But I'm sensing some issues, so we'd need some time for it.

@scofalik scofalik linked a pull request Feb 9, 2020 that will close this issue
Open
@mlewand mlewand mentioned this issue Mar 18, 2020
Closed
@pomek pomek removed this from the nice-to-have milestone Feb 21, 2022
@lslowikowska lslowikowska added the support:2 An issue reported by a commercially licensed client. label Mar 24, 2022
@scofalik scofalik mentioned this issue Nov 2, 2022
Closed
Closed
@scofalik
Copy link
Contributor Author

It looks like inline annotations cannot be displayed if the editor is in read-only mode. It may be connected with above - AFAIR, the contextual balloon is not shown if the editor is not focused.

Additionally this is also a problem for non-classic builds because you lose the access to the toolbar at all. So, for example, you cannot use export to PDF etc.

@usqr
Copy link

usqr commented Jan 13, 2023 •
edited
Loading

It looks like inline annotations cannot be displayed if the editor is in read-only mode. It may be connected with above - AFAIR, the contextual balloon is not shown if the editor is not focused.

Problem here is inconsistency, as it only affects inline UI - narrow/wide side bar are displayed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
domain:accessibility This issue reports an accessibility problem. domain:ui/ux This issue reports a problem related to UI or UX. support:2 An issue reported by a commercially licensed client. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Added tabindex="0" to editable element to make it focusable in read-only mode ckeditor/ckeditor5-ui
8 participants
@Reinmar @Comandeer @oleq @scofalik @pomek @mlewand @usqr @lslowikowska