Conversation
|
Additionally, moving up and down a few times seems to end the flicker, so I'm not sure what exactly is causing it to stop... |
|
Hm, I even with this patch, I still get the flicker when starting in a quote, between the text and citation. |
|
Checking out the commit before multi-select was added: If I start selection in between the quoted text and citation, I can also see that |
ellatrix
force-pushed
the
fix/multi-select-flicker
branch
from
fa5e7be to
e563579
Compare
|
Actually it just looks like there is an |
ellatrix
force-pushed
the
fix/multi-select-flicker
branch
2 times, most recently
from
3674a4a to
34fe5a4
Compare
ellatrix
force-pushed
the
fix/multi-select-flicker
branch
from
34fe5a4 to
5330671
Compare
|
Hm, we can't just take this away. Additionally, I've moved the callback in the wrong place when merging multi-select. |
ellatrix
force-pushed
the
fix/multi-select-flicker
branch
from
5330671 to
dfbfbc8
Compare
|
Flickering seems much less intermittent in this branch, that's great. The selection seems a little bit sticky though, hard to explain, but it feels like sometimes you select a few blocks, then deselect, scroll, and then sometimes the blocks are selected again. Also managed to get a white screen of death and this error in the console:
|
|
This works great for me, I can't reproduce the error above or the sticky selection myself. |
blocks/editable/index.js
Outdated
There was a problem hiding this comment.
Could we merge into a single if?
There was a problem hiding this comment.
Sure, I should use different techniques to indent/wrap. :D
|
@jasmussen Could you post the error without minified JS? Regarding the "sticky" thing, I know what you mean, but it is unrelated to this PR. What you mean is that the selection does not adjust on scroll, but only on mouse move, right? This is because browsers do not fire mouse move events on scroll as the pointer is staying in the same position. I'll attempt to fix this in a separate PR, but it's not trivial. |
Do you mean that you still see flicker? |
ellatrix
force-pushed
the
fix/multi-select-flicker
branch
from
dfbfbc8 to
8b5e765
Compare
|
Just to clarify, this branch should fix:
|
|
For the last point, try e.g. tabbing into a YouTube block. It will not select on master, but should here. |
|
I couldn't reproduce the error, but got a different one. But I think I narrowed down the selection issue I was having. I think that if you start selecting, then drag outside of the viewport, release the mouse, and then move it inside again, you're still selecting blocks. Which gets a little extra weird when you start selecting anew. I recorded a video to showcase, but it's soundless so it's a bit hard to see when I click to deselect. But there should be a circle indicator around the arrow when I click: |
|
Okay, that looks like a combination of the issue I described above, and not having mouse move event handlers outside the blocks. The selection issue when moving outside the window is also valid. Could we please attempt to fix these in a separate issues? |
Absolutely! It's a vast improvement as is! |
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
If we're clear from the JS error, Let's 🚢 this
@jasmussen Could you share what the other error was? |
|
I can't share more than that screenshot I took in #1024 (comment). It could be just some ghost data from me switching branches and reloading fast. |
3 participants
To reproduce the issue, try to start selecting a block containing an
Editableregion, but not from inside that region, then move up or down to multi-select. The state will bounce between having no multi-selection (as it toggles the selected block) and having multi-selection (as you move the mouse). This results in the flickering selection colour.I'm still not sure what is causing the multiple set focus calls in the scenario above, while not when starting from inside the Editable...