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.

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

Blurred editor selection should be distinguishable #6517

Closed
mlewand opened this issue Mar 30, 2020 · 13 comments · Fixed by ckeditor/ckeditor5-theme-lark#277
Closed

Blurred editor selection should be distinguishable #6517

mlewand opened this issue Mar 30, 2020 · 13 comments · Fixed by ckeditor/ckeditor5-theme-lark#277
Assignees
Labels
domain:ui/ux This issue reports a problem related to UI or UX. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@mlewand
Copy link
Contributor

mlewand commented Mar 30, 2020

📝 Provide detailed reproduction steps (if any)

  1. Open any CKEditor 5 sample.
  2. Select all the content.
  3. Move the focus away from the editor, e.g. press shift + tab to move the selection to preceding "classic editor" link.
  4. Examine editor selection color.

✔️ Expected result

A common practice is to turn it gray or hide completely if the input widget is not focused.

❌ Actual result

Currently the editor focus is always blue.

📃 Other details

  • Browser: Any
  • OS: Win10 (probably any)

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

@mlewand mlewand added type:bug This issue reports a buggy (incorrect) behavior. domain:ui/ux This issue reports a problem related to UI or UX. labels Mar 30, 2020
@Reinmar
Copy link
Member

Reinmar commented Mar 30, 2020

That's native behavior. We do nothing custom here. I'm not sure we should be touching this.

OTOH, it's something that we could perhaps do differently than the OS.

@Reinmar
Copy link
Member

Reinmar commented Mar 30, 2020

cc @oleq

@Reinmar Reinmar added this to the unknown milestone Mar 30, 2020
@mlewand mlewand added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). and removed type:bug This issue reports a buggy (incorrect) behavior. labels Mar 30, 2020
@mlewand
Copy link
Contributor Author

mlewand commented Mar 30, 2020

I'm actually surprised to see that this is an issue coming from browser, and in fact this is what we get from contenteditable. For comparison here's the behavior for textarea in Chrome, which still is not ideal:

Works better for this particular case but then again user is not aware what has been selected.

The way I see it the most optimal way is to keep showing the selection but change its color, to indicate that it doesn't have the focus.

@oleq
Copy link
Member

oleq commented Apr 1, 2020

Yes, we can do something about it. I created a PoC in https://github.com/ckeditor/ckeditor5-theme-lark/compare/i/6517?expand=1 that works in Chrome and Firefox.

2020-04-01 12 19 57

Except for the selection background in nested editables which is broken due to the ::selection selector I used:

image

I couldn't find any way to filter nested editables out. Technically we could override this ::selection in widget styles but then there's probably no way to revert it to UA value.

Wanna take a look @panr?

@panr
Copy link
Contributor

panr commented Apr 2, 2020

Sure, will check it tomorrow morning 👌

@panr
Copy link
Contributor

panr commented Apr 3, 2020

@oleq what about using .ck-blurred class?

.ck-blurred ::selection {
    background: var(--ck-color-editable-blur-selection);
}

IMHO, it's even better. You keep "active" selection when you're navigating through the toolbar, but you get "inactive" selection when you blur the editor for real. And of course it's binded to the existing logic (switching between .ck-focused and .ck-blurred).

@oleq
Copy link
Member

oleq commented Apr 3, 2020

@panr It won't work because if you focus the toolbar via Alt+F10, the root editable still has .ck-focused. And this is when the selection should be greyed-out.

image

@panr
Copy link
Contributor

panr commented Apr 3, 2020

As I mentioned above:

IMHO, it's even better. You keep "active" selection when you're navigating through the toolbar, but you get "inactive" selection when you blur the editor for real. And of course it's binded to the existing logic (switching between .ck-focused and .ck-blurred).

:-)

And for some reason, you decided to keep the .ck-focused class when focus changes to the toolbar 😉

@oleq
Copy link
Member

oleq commented Apr 3, 2020

Can you create a PoC so we can see it live?

@panr
Copy link
Contributor

panr commented Apr 3, 2020

Sure, the simplest and the quickest POC (without any mocked environment) — https://github.com/ckeditor/ckeditor5-theme-lark/compare/i/6517-ck-blurred?expand=1 (don't be bothered by the diff in formheader.css ;-P )

@oleq
Copy link
Member

oleq commented Apr 6, 2020

So it only affects the selection when the focus went out of the editor (not when the toolbar became focused)... I think it makes sense and we could go with it. WDYT @mlewand?

@mlewand
Copy link
Contributor Author

mlewand commented Apr 6, 2020

@oleq @panr Having it active while in the toolbar sounds totally fine to me (I think it makes for less distractions that way, so we should favor this UX-wise) 👍

@panr
Copy link
Contributor

panr commented Apr 6, 2020

@mlewand @oleq I'll create a proper PR later today 👌

@panr panr self-assigned this Apr 6, 2020
oleq added a commit to ckeditor/ckeditor5-theme-lark that referenced this issue Apr 9, 2020
Other: The selection inside the `InlineEditableUIView` should be grey when the editor is blurred. Closes ckeditor/ckeditor5#6517.
@oleq oleq modified the milestones: unknown, iteration 31 Apr 9, 2020
mlewand pushed a commit that referenced this issue May 1, 2020
Other: The selection inside the `InlineEditableUIView` should be grey when the editor is blurred. Closes #6517.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants