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

Smooth Native Multi Block Selection #16835

Merged
merged 4 commits into from
Nov 29, 2019
Merged

Smooth Native Multi Block Selection #16835

merged 4 commits into from
Nov 29, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jul 31, 2019

Description

  • Fixes Smooth Native Multi Block Selection #16835.
  • Fixes Blocks use custom selection color #15270. Uses native selection colour.
  • Uses native selection behaviour to allow selection across blocks.
    • This makes selection feel very smooth.
    • It eliminates a bunch of logic to track in which block the mouse is positioned.
  • A step closer towards partial block selection and actions. Currently when the mouse press is released, we expand the selection to the whole block. In the future, we can remove this code, set a more detailed selection state, and update action like merging to look at this more detailed state.
  • Increased e2e test coverage. We didn't have any tests for multi selection by mouse. 😶

I had to disable contenteditable for multi-selection, because browsers won't allow selection across editable hosts. This actually works quite well.

How has this been tested?

Select multiple block by holding and releasing a mouse press, just like you would for normal selection.

Screenshots

smooth

Types of changes

Bug fix.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@ellatrix ellatrix added [Status] In Progress Tracking issues with work in progress [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Feature] Block Multi Selection The ability to select and manipulate multiple blocks labels Jul 31, 2019
@youknowriad
Copy link
Contributor

This would make this #16811 impossible to achieve right?

@youknowriad
Copy link
Contributor

I guess in theory, you can have multiple ranges?

@ellatrix
Copy link
Member Author

@youknowriad Not necessarily, since for non consecutive selection, you wouldn't use mouse dragging. But it is true that we cannot set a native selection in that case, or at least not for all of the selection pieces. We'd have to use our own colours... That may look inconsistent though... Cc @jasmussen

@ellatrix
Copy link
Member Author

In the case of non consecutive selection, we could maybe only rely on a border/shadow/outline to visualise the selection.

@ellatrix
Copy link
Member Author

I could imaging non consecutive block selection could looks something like this:

Select-All-Photos-app

I think it's ok that both of these behave a bit differently. Non consecutive selection may have UI for selecting blocks that consecutive selection wouldn't have.

@jasmussen
Copy link
Contributor

Here's what I see:

new multi select

Immediate reaction: this is really nice. It solves the issues from #15640 very elegantly, and it even feels very native. It even works nicely with the more exotic blocks:

Screenshot 2019-08-05 at 10 15 15

I definitely think we should move forward with this.

This would make this #16811 impossible to achieve right?

Outside of whatever technical magic I'm sure Ella can wield, this can be solved with design.

Here's how a selected text block looks in Figma:

Screenshot 2019-08-05 at 10 16 44

Here's how a selected text block with the text inside also selected looks:

Screenshot 2019-08-05 at 10 16 48

Figma is obviously not a 1:1 with our editor. But we can take some inspiration. In this case, I think it could be solved by keeping the text selection indicator as we have here, but also adding an additional indicator of block selection. It could just be a simple border around a selected block, like Figma, and could look something like this (very quick and dirty mockup, not final suggestion):

Screenshot-2019-08-05-at-10 11 27

Question: in addition to showing the selection range as this PR has already accomplished, is it possible to also show a highlight border around the selected blocks? If it is, then this would solve the non-consecutive block selection challenge as well, even if we can't show multiple selection ranges.

@ellatrix
Copy link
Member Author

ellatrix commented Aug 6, 2019

Question: in addition to showing the selection range as this PR has already accomplished, is it possible to also show a highlight border around the selected blocks? If it is, then this would solve the non-consecutive block selection challenge as well, even if we can't show multiple selection ranges.

Yes, sure, we can style the selected block however we want. I would recommend not using a background, but only a border, shadow, or outline, ideally in the currentColor of the main text, or the inverse color of the background. I think for now this can be similar to the border we have for a single selected block. I'm not sure how contrast issues are solved in that case.

@jasmussen
Copy link
Contributor

I would recommend not using a background, but only a border, shadow, or outline

Yes indeed. Shadows are removed in Windows High Contrast mode, so if we have to go with those, we need something else for that mode also -- a transparent outline is a neat hack that works because the transparency becomes opaque in that mode.

ideally in the currentColor of the main text, or the inverse color of the background.

Oh if only CSS2's Highlight color was canonical and worked solidly...

I think that we should choose a color here, in the blue spectrum for the default theme and potentially themed for the others. Reason being that a shade of blue is the default selection color for most operating systems, and likely to be subconsciously connected to "selections" for that reason.

I think for now this can be similar to the border we have for a single selected block. I'm not sure how contrast issues are solved in that case.

I suppose "currentColor" would solve that because presumably the themer who chose a black background would also choose light text. But it also probably wouldn't be bulletproof because a random block might have a different color that would then be inherited, and it would not look great if the color of the selection border changed across blocks.

You mention ...

or the inverse color of the background

We could look into https://www.w3schools.com/cssref/pr_mix-blend-mode.asp for that, potentially?

@ellatrix

This comment has been minimized.

@ellatrix ellatrix self-assigned this Aug 7, 2019
@ellatrix ellatrix force-pushed the try/native-multi-select branch 2 times, most recently from 02af322 to def6860 Compare September 20, 2019 12:04
@ellatrix ellatrix changed the title Native Multi Block Selection Smooth Native Multi Block Selection Sep 20, 2019
@ellatrix
Copy link
Member Author

@jasmussen Would love if you could test this again. I made some changes which should make selection now really smooth.

@jasmussen
Copy link
Contributor

Quick GIF:

gif

This feels really responsive to me. While there are some little things could could use a little TLC, like the select style and the lingering hover styles, overall this feels like a superbly solid direction. I think this can totally be made to work, impressive!

I noticed that when you select downwards across an image, it's "highlighted", but not when you select upwards:

image

— until you release the mouse of course and then it's selected.

One of the greatest benefits of going with the native selection style, is that you get the "grayed selection" when the window loses focus. This works, but it doesn't work when the link dialog shows:

gray inactive

I could swear that worked in the past attempt that simply removed the colors. Do you know what this might be?

And finally — this feels like it can be the first step towards the ability to select between the middles of two paragraphs to merge them. Do you think we can enable that in a future PR? :)

@ellatrix
Copy link
Member Author

Yeah, I don't know what's wrong with the hover styles. Need to have a look into it.
The multi selection style, I might need your help with. :) I don't really find a good borders solution.

The image issue would be a native thing. Which browser are you using? It might be that you need to move a little further to select it. Selection is completely in the hands of the browser here, there's not much I can do.

And finally — this feels like it can be the first step towards the ability to select between the middles of two paragraphs to merge them. Do you think we can enable that in a future PR? :)

Yes, totally. This is also meant as a step towards partial block selection.

@jasmussen
Copy link
Contributor

I'll jump in and help as soon as I can. I'm working on some jet lag at the moment. In sure Monday I'll be alright.

All sounds good. Curious about the image thing, selecting from after the image and upwards, I'll test a little more. I'm using chrome.

Did you have any thoughts on the inactive select style for links?

@ellatrix
Copy link
Member Author

@jasmussen Fixed the hover styles. I have a hard time understanding what you mean from the GIF. Could you show are explain a little more? What's different from master?

@ellatrix

This comment has been minimized.

@ellatrix
Copy link
Member Author

Fixed the Safari issue. Should make things smoother in general too in other browsers.

@jasmussen
Copy link
Contributor

I have a hard time understanding what you mean from the GIF. Could you show are explain a little more? What's different from master?

Nothing's different from master, so to clarify there's nothing you have to fix here at all.

But I was hoping that moving from colored selections to stock native selections would fix a lingering issue I have with our link box:

  • When you use CSS to color ::selection, the "unfocused selection color" disappears
  • When you use native selection colors, you can still see what's selected i an unfocused window — it turns gray as in the GIF.

My hope was that the unfocused selection would be present when the link dialog is open, as opposed to just just floating in space, and I could swear I had seen this being the case. But I just spun up #15640 to verify, and no — that link box still floats in space, sadly.

So nevermind my comment — I'd still love for us to fix the link issue, but it's definitely a separate issue from this PR, and will probably be easier to fix when this is merged.

So continue blazing a trail, I'm super excited about this branch (and can confirm the hover issues are fixed).

@ellatrix ellatrix force-pushed the try/native-multi-select branch from 3dd9ab1 to cc709da Compare November 28, 2019 22:33
@ellatrix
Copy link
Member Author

@youknowriad @jasmussen So far I can think of only one way to have a native selection (colour) over the entire block. It's a bit hacky, but we could insert a transparent image (resized to fit the block) and select that, instead of all the block contents. What is selected doesn't matter anyway, it's purely visual. We overwrite the contents of the clipboard on copy/cut.

@ellatrix
Copy link
Member Author

Screenshot 2019-11-29 at 00 08 26

@youknowriad Here's what it looks like now.

@youknowriad
Copy link
Contributor

Nice trick. I think the fact that we still see the text selection happening in the background defeats the purpose of the trick a little bit. I'm ok moving forward without the trick. Thoughts?

@ellatrix
Copy link
Member Author

@youknowriad Yeah, I think I agree. We will anyway have to remove it again when we implement partial black selection. I’ll fix up this PR in a bit.

@ellatrix
Copy link
Member Author

In the worst case we can always revert the selection colour but keep the smooth selection.

    7a9ce87 (HEAD -> try/native-multi-select, origin/try/native-multi-select) Fix selection in Firefox
    eb668d3 Fix bad rebase
    71bf361 Fix block deletion e2e test
    386810a Add mouse drag test
    41cb592 Rewrite tests
    c22a36b Polish
    9832732 Remove mover min-height and multi select top border
    cac2c20 Fix cross selection delay in Safari
    116e89f Hide hover effect on mouseleave
    a3f1547 Correct toolbar position
    24f817d contenteditable false when there is multi selection
    4eade53 Fix selection setting
    9182fd3 Remove elements in the way of selection
    98dae74 Reselect block correctly
    f1b08ac Smooth selection
    3fb633a Native Multi Block Selection
@ellatrix ellatrix force-pushed the try/native-multi-select branch from fe9aca6 to b46ad9c Compare November 29, 2019 09:53
@ellatrix ellatrix force-pushed the try/native-multi-select branch from def7417 to 9ea4bc4 Compare November 29, 2019 12:24
@ellatrix
Copy link
Member Author

@youknowriad I addressed the feedback. Would be grateful for another pass. :)

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely convinced that this selection modal is better than master from the user's perspective but let's try it.

@ellatrix
Copy link
Member Author

I'm convinced that the selecting feels much smoother and better, but I'm not 100% convinced that the visual block selection is clear. This last part is easy to adjust later though. We can always re-add the old selection colour if necessary, or explore different solutions.

Thank you @youknowriad and @jasmussen for the review. Needless to say, if any issues arise, I'll quickly follow up.

@ellatrix ellatrix merged commit bf6976b into master Nov 29, 2019
@ellatrix ellatrix deleted the try/native-multi-select branch November 29, 2019 13:22
@jasmussen
Copy link
Contributor

I'm out for the weekend but I'm confident in this selection and that we can refine it further, and I plan to look at it Monday.

@ellatrix
Copy link
Member Author

@jasmussen Don't look at your work emails. 😄

@ZebulanStanphill
Copy link
Member

I'm guessing that selecting multiple blocks including nested ones isn't supposed to look like this?
image

jasmussen added a commit that referenced this pull request Dec 2, 2019
jasmussen added a commit that referenced this pull request Dec 4, 2019
jasmussen added a commit that referenced this pull request Dec 4, 2019
jasmussen added a commit that referenced this pull request Dec 4, 2019
* Limit additional selection marker to top level blocks.

Addresses #16835 (comment).

* Provide exception for placeholderes.
@ellatrix
Copy link
Member Author

ellatrix commented Dec 6, 2019

@ZebulanStanphill The extra lines should be gone now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Multi Selection The ability to select and manipulate multiple blocks [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blocks use custom selection color
8 participants