ckeditor5 icon indicating copy to clipboard operation
ckeditor5 copied to clipboard

Make editor's application labels unique

Open Comandeer opened this issue 2 years ago • 8 comments

📝 Provide detailed reproduction steps (if any)

  1. Open https://ckeditor.com/docs/ckeditor5/latest/features/lists/lists.html
  2. Open VoiceOver.
  3. Open rotor and switch to landmarks list.

✔️ Expected result

Every editor has a unique name on the list displayed by the rotor.

❌ Actual result

Every editor has the same name, "Rich Text Editor".

❓ Possible solution

Add the editor name after the static part of the label: "Rich Text Editor, <editor name>".

📃 Other details

  • Browser: all (tested on Chrome)
  • OS: all
  • First affected CKEditor version: all
  • Installed CKEditor plugins: N/A

The analogous issue is present in other screen readers.


If you'd like to see this fixed sooner, add a 👍 reaction to this post.

Comandeer avatar Jun 01 '22 12:06 Comandeer

While working on this issue we can look how we solve this problem in CKE4.

We could add a config property that would allow for providing a human readable title / label. CKE4 has it for instance here: https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_config.html#cfg-applicationTitle

If no human-readable title is provided we should do the same thing we did in CKE4, so create a label like "editor1", "editor2", "editorn...".

mlewand avatar Jun 27 '22 12:06 mlewand

Notes from the recent meeting:

  • Let's use editor.config.ui.name for configuration.
  • For the time being, no properties such as Editor#name.
    • If a feature wants to know the name of the editor, it should use editor.config.get( 'ui.name' ) instead.
    • To provide the incremental EditorN label for editors without editor.config.ui.name config I think we should use editor.config.define() instead of generator proposed in the PR. We're doing this already.
  • If there's just a single root (e.g. classic editor), we ditch its name in the label (Editor editing area instead of Editor editing area: main) because it only creates noise.
  • Multi root editors
    • editor.config.ui.name should be used for the entire root constellation
    • As for naming individual roots, let's leave it as it is (Rich Text Editor. Editing area: footerleft) for now.
  • Editor context should not pass editor.config.ui.name to the editor (we need to exclude it to avoid misleading duplication)
  • Comments editor
    • All editors should be using the same name because the (a11y) context will be set on the parent of the thread in the future.
  • Let's ignore CKEditorError integration because accessible name is not an instance name.
  • Let's ignore CKEditorInspector integration because accessible name is not an instance name

oleq avatar Sep 08 '22 14:09 oleq

To provide the incremental EditorN label for editors without editor.config.ui.name config I think we should use editor.config.define() instead of generator proposed in the PR. We're doing this already.

I just realized this won't work. See how the classic editor is initialized:

https://github.com/ckeditor/ckeditor5/blob/b0a183e5ddf131625d6c9c26fc575269e55890a1/packages/ckeditor5-editor-classic/src/classiceditor.js#L75-L90

  • I think EditorUI (in this case ClassicEditorUI) should use editor.config.define( 'ui.name' ) because the Editor class has nothing to do with the UI. 
  • But... to allow EditorUIView (in this case ClassicEditorUIView) propagate the value of the config in DOM, it needs to be known before EditorUI gets created. 
  • I don't remember why we decided to create a view first and then pass it to the UI. I don't see any elegant solution to work around this one.

oleq avatar Sep 08 '22 14:09 oleq

Maybe I don't see something but... why not set it as one of options when creating ClassicEditorUIView instance?

const view = new ClassicEditorUIView( this.locale, this.editing.view, { 
     shouldToolbarGroupWhenFull,
     label: this.config.get( 'ui.name' )
} );

Or just pass whole config.

Or, cannot this be a dynamic value (I mean either bound to template or just observable)? It could be set afterwards. Why not?

scofalik avatar Sep 08 '22 14:09 scofalik

label: this.config.get( 'ui.name' )

Because there must be a place that takes care of unnamed editors and increments "EditorN". Preferably by setting editor.config.define( 'ui.name', Editor${ ++i } ). And the right place for this thing is definitely EditorUI. And that does not exist yet at this point 😛

oleq avatar Sep 09 '22 07:09 oleq

An update: After a F2F with PK we figured that we're gonna change the API for our editors' UIs ClassicEditorUI, InlineEditorUI etc. and implement ClassicEditorUI#setView( view: EditorUIView )

This will be a minor change because it's unlikely that any of these UIs is used as a base class by integrators and, at the same time, EditorUI#constructor( editor: Editor ) accepts editor instance only, so we're not changing the API of the base class. 

Also, we figured we should probably use editor.config.ui.editorName but let's figure it out during code review when we can see everything coming together.

To sum things up:

  • Let's use editor.config.ui.name for configuration.
  • For the time being, no properties such as Editor#name.
    • If a feature wants to know the name of the editor, it should use editor.config.get( 'ui.name' ) instead.
    • To provide the incremental Rich Text Editor N label for editors without editor.config.ui.name config I think we should use editor.config.define() instead of generator proposed in the PR. We're doing this already.
  • If there's just a single root (e.g. classic editor), we ditch its name in the label (Editor editing area instead of Editor editing area: main) because it only creates noise.
  • Multi root editors
    • editor.config.ui.name should be used for the entire root constellation
    • As for naming individual roots, let's leave it as it is (Rich Text Editor. Editing area: footerleft) for now.
  • Editor context should not pass editor.config.ui.name to the editor (we need to exclude it to avoid misleading duplication)
  • Comments editor
    • All editors should be using the same name because the (a11y) context will be set on the parent of the thread in the future.
  • Let's ignore CKEditorError integration because accessible name is not an instance name.
  • Let's ignore CKEditorInspector integration because accessible name is not an instance name

oleq avatar Sep 09 '22 15:09 oleq

Does this have any impact on multi root editor implementations that people have? I mean changing how the view is set?

scofalik avatar Sep 09 '22 15:09 scofalik

Does this have any impact on multi root editor implementations that people have? I mean changing how the view is set?

It should not because we're not changing the api of the base EditorUI class.

oleq avatar Sep 12 '22 07:09 oleq

Related - https://github.com/ckeditor/ckeditor5/issues/9731

FilipTokarski avatar Sep 05 '23 11:09 FilipTokarski