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

Fix multi-select inside new scroll container #2858

Merged
merged 2 commits into from Oct 3, 2017

Conversation

Projects
None yet
2 participants
@iseulde
Member

iseulde commented Oct 2, 2017

Description

This PR fixes multi-select after 43b84a5. There is a new scroll container, and the block list component was relying on the window scroll position. Instead of trying to figure about which node is the scroll box etc., I've changed to logic so that it's independent of the scroll position. This would also avoid future issues if more than one container becomes scrollable (intentionally or not).

How Has This Been Tested?

  1. Open the demo content (so the content exceeds screen height).
  2. Verify that multi-selection works.
  3. Verify that the buffer works (no selection => selection). There should be a bit of space (20px) around the start block that doesn't trigger the multi-selection.
  4. Ensure that the selection updates if you move down enough to trigger a scroll by the browser.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@iseulde iseulde added the [Type] Bug label Oct 2, 2017

@iseulde iseulde added this to the Beta 1.3 milestone Oct 2, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 2, 2017

Codecov Report

Merging #2858 into master will decrease coverage by 0.23%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2858      +/-   ##
==========================================
- Coverage   33.78%   33.55%   -0.24%     
==========================================
  Files         191      191              
  Lines        5692     5791      +99     
  Branches      997     1021      +24     
==========================================
+ Hits         1923     1943      +20     
- Misses       3189     3249      +60     
- Partials      580      599      +19
Impacted Files Coverage Δ
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
blocks/api/serializer.js 98.14% <0%> (+0.23%) ⬆️
blocks/library/paragraph/index.js 34.78% <0%> (+1.44%) ⬆️
blocks/library/cover-image/index.js 35.89% <0%> (+2.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6520235...a86ad9c. Read the comment docs.

codecov bot commented Oct 2, 2017

Codecov Report

Merging #2858 into master will decrease coverage by 0.23%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2858      +/-   ##
==========================================
- Coverage   33.78%   33.55%   -0.24%     
==========================================
  Files         191      191              
  Lines        5692     5791      +99     
  Branches      997     1021      +24     
==========================================
+ Hits         1923     1943      +20     
- Misses       3189     3249      +60     
- Partials      580      599      +19
Impacted Files Coverage Δ
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
blocks/api/serializer.js 98.14% <0%> (+0.23%) ⬆️
blocks/library/paragraph/index.js 34.78% <0%> (+1.44%) ⬆️
blocks/library/cover-image/index.js 35.89% <0%> (+2.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6520235...a86ad9c. Read the comment docs.

@iseulde iseulde requested a review from aduth Oct 2, 2017

@aduth

aduth approved these changes Oct 2, 2017

Appears to work well, and I like the change to rely on bounds of the node instead of scroll position 👍

@@ -83,14 +83,15 @@ class VisualEditorBlockList extends Component {
onPointerMove( { clientY } ) {
const BUFFER = 20;
const { multiSelectedBlocks } = this.props;
const y = clientY + window.pageYOffset;
const boundaries = this.refs[ this.selectionAtStart ].getBoundingClientRect();

This comment has been minimized.

@aduth

aduth Oct 2, 2017

Member

Because onPointerMove will be called very frequently, could there be a performance implication to this call? Would it be possible to catch this in onSelectionStart. At least since the function is throttled, it may not be too much of an issue.

@aduth

aduth Oct 2, 2017

Member

Because onPointerMove will be called very frequently, could there be a performance implication to this call? Would it be possible to catch this in onSelectionStart. At least since the function is throttled, it may not be too much of an issue.

This comment has been minimized.

@iseulde

iseulde Oct 3, 2017

Member

Right, it will stay the same as long as none of the parent containers scroll, but we'd have to recalculate on scroll. I wanted to adjust that, but at the same time found another bug... The scroll event won't fire on window with the scroll container. It doesn't bubble, whereas mousemove does. 😔 Looking further.

@iseulde

iseulde Oct 3, 2017

Member

Right, it will stay the same as long as none of the parent containers scroll, but we'd have to recalculate on scroll. I wanted to adjust that, but at the same time found another bug... The scroll event won't fire on window with the scroll container. It doesn't bubble, whereas mousemove does. 😔 Looking further.

This comment has been minimized.

@iseulde
@iseulde

iseulde Oct 3, 2017

Member

This comment has been minimized.

@iseulde

iseulde Oct 3, 2017

Member

Or we store a ref for the editor layout, then pass it down I guess...

@iseulde

iseulde Oct 3, 2017

Member

Or we store a ref for the editor layout, then pass it down I guess...

This comment has been minimized.

@iseulde

iseulde Oct 3, 2017

Member

Okay, found a fix for that. Coming back to your comment, I'm not sure if it's a good idea to only recalculate the boundaries on scroll, exactly because it's throttled. What if there's a bunch of scroll events and then a bunch of mousemove events, and the one that passes through is a mousemove event? The boundaries won't be recalculated and will maybe be off by a little bit, maybe just enough to feel buggy?

@iseulde

iseulde Oct 3, 2017

Member

Okay, found a fix for that. Coming back to your comment, I'm not sure if it's a good idea to only recalculate the boundaries on scroll, exactly because it's throttled. What if there's a bunch of scroll events and then a bunch of mousemove events, and the one that passes through is a mousemove event? The boundaries won't be recalculated and will maybe be off by a little bit, maybe just enough to feel buggy?

This comment has been minimized.

@iseulde

iseulde Oct 3, 2017

Member

And since we'd allow it to fire every 250 ms on scroll anyway, I'm not concerned to let if fire on mousemove as well, especially since they're both part of the same throttle.

@iseulde

iseulde Oct 3, 2017

Member

And since we'd allow it to fire every 250 ms on scroll anyway, I'm not concerned to let if fire on mousemove as well, especially since they're both part of the same throttle.

@iseulde iseulde merged commit 0fde369 into master Oct 3, 2017

3 checks passed

codecov/project 33.55% (-0.24%) compared to 6520235
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@iseulde iseulde deleted the fix/multi-select-scroll-container branch Oct 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment