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

The nested widget selection handle is visible while the outer table cells are selected #9491

Closed
niegowski opened this issue Apr 16, 2021 · 6 comments · Fixed by #10556
Closed
Assignees
Labels
intro Good first ticket. package:theme-lark squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@niegowski
Copy link
Contributor

📝 Provide detailed reproduction steps (if any)

  1. https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/features/table.html
  2. make nested table
  3. select multiple cells in the outer table

✔️ Expected result

The selection handle is not visible

❌ Actual result

📃 Other details

  • Browser: …
  • OS: …
  • First affected CKEditor version: …
  • Installed CKEditor plugins: …

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

@niegowski niegowski added type:bug This issue reports a buggy (incorrect) behavior. squad:compat labels Apr 16, 2021
@Mgsy Mgsy added this to the nice-to-have milestone Apr 29, 2021
@mlewand mlewand added the squad:features Issue to be handled by the Features team. label Sep 10, 2021
@mlewand mlewand modified the milestones: nice-to-have, iteration 47 Sep 10, 2021
@mlewand mlewand added the intro Good first ticket. label Sep 10, 2021
@Mgsy Mgsy removed the squad:compat label Sep 13, 2021
@mlewand
Copy link
Contributor

mlewand commented Sep 15, 2021

Issue is not limited to tables but is reproducible for any other widget with selection handle, e.g. HTML embed:

@mlewand mlewand changed the title The nested table selection handle is visible while the outer table cells are selected The nested widget selection handle is visible while the outer table cells are selected Sep 15, 2021
@oleq
Copy link
Member

oleq commented Sep 20, 2021

Before we jump to conclusions and start writing code, let's figure out the right UX first. In my opinion, there are multiple approaches to this problem.

Status of things (master)

  • When the selection includes inline widgets in text, they get the selected outline.
  • When the selection includes block widgets, they get the selected outline (but not their children).
  • When the selection includes a single block widget, it gets the selected outline (but not its children).
  • When the selection spans multiple table cells, the children inside (inline widgets and block widgets), don't get the outline except for the block selection handle glitch reported in this issue.

I noticed there's a certain inconsistency between the "normal selections" (first two screenshots) and the table cell selections (brought by the TableSelection plugin). Normal selections highlight children's widgets (but only 1st level). Table cell selections don't select anything.

The latter is a result of the decision we took last year 237cb30.

  • Why did we go this way BTW?

Possible solutions

Make the table cell selection more like the "regular selection" (less safe)

You can remove the restriction introduced in the commit I mentioned and things will look slightly different:

diff --git a/packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableselection.css b/packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableselection.css
index 5a0901f589..cd8c0bd5e7 100644
--- a/packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableselection.css
+++ b/packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableselection.css
@@ -31,9 +31,5 @@
                   &:focus {
                            background-color: transparent;
                   }
-
-                  & .ck-widget_selected {
-                           outline: unset;
-                  }
          }
 }

You may notice that the table cell selection acts sort of more like the "regular selection". It highlights both inline and block widgets within its reach but only the first level down the tree.

Stay with the UX we have on master but ditch the nested block widget selection handle (safer)

This was the intent of https://github.com/ckeditor/ckeditor5/pull/10556/files. There's no need to manipulate the widget CSS classes, in my opinion. The .ck-widget_selected class is OK to stay whenever the widget is included in any selection because, well... engine–wise, it is selected 😛. The presentational layer can provide the right UX on top of it using pure CSS, though.

We can ditch the selected block widget handle in table cell selection by:

diff --git a/packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableselection.css b/packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableselection.css
index 5a0901f589..6b0b4f8ea2 100644
--- a/packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableselection.css
+++ b/packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableselection.css
@@ -34,6 +34,10 @@

                   & .ck-widget_selected {
                            outline: unset;
+
+                           & .ck-widget__selection-handle {
+                                    display: none;
+                           }
                   }
          }
 }

@oleq
Copy link
Member

oleq commented Sep 20, 2021

During today's UX sync we decided 

Stay with the UX we have on master but ditch the nested block widget selection handle (safer)

is the right direction ATM.

@EwelinaD
Copy link
Contributor

About this safer resolution:

If we set display: none for selection handle in tableselection we will also change its behavior during mouse hover. See the screenshots below

  1. master

2. with safer resolution (display: none)

As you can see there's no selection handle for selected table with hover.

There's also another problem on master with nested tables: there's no border and selection handle for first nested table - I'm not sure if it should be handle within this ticket or not.

@oleq
Copy link
Member

oleq commented Sep 20, 2021

Side-note: Looks like there's a lot of funny things going on we need to straighten out. 

For instance, only the innermost widget gets the yellow hover outline when the mouse is over it because as per #4594 

// Do not mark nested widgets in selected one. See: #57.
if ( isWidget( node ) && !isChild( node, lastMarked ) ) {

and as a result, the innermost widget does not get the .ck-widget_selected class. This was OK in 2018 when nested widgets were not as popular. Now we need to figure this all out.

@oleq
Copy link
Member

oleq commented Sep 20, 2021

A rough idea based on the fact that we don't really care if the widget has the selected CSS class or not when it's inside table selection. The fact it is there is all that really matters, you can't have a table selection and proper widget selection going on in parallel anyway. I wouldn't merge it to master two days before the code freeze, though.

diff --git a/packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableselection.css b/packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableselection.css
index 5a0901f589..fdfa20b009 100644
--- a/packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableselection.css
+++ b/packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableselection.css
@@ -32,8 +32,16 @@
                            background-color: transparent;
                   }
 
-                  & .ck-widget_selected {
+                  /*
+                   * To reduce the amount of noise, all widgets in the table selection have no outline and no selection handle.
+                   * See https://github.com/ckeditor/ckeditor5/issues/9491.
+                   */
+                  & .ck-widget {
                            outline: unset;
+
+                           & > .ck-widget__selection-handle {
+                                    display: none;
+                           }
                   }
          }
 }

@mlewand mlewand modified the milestones: iteration 47, iteration 48 Sep 21, 2021
oleq added a commit that referenced this issue Sep 28, 2021
Fix (table): A nested widget in a multi-cell table selection should not have the selection handle. Closes #9491.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
intro Good first ticket. package:theme-lark squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants