♻️ Change flag opening/closing logic #80

fyvfyv · 2022-03-07T10:36:11Z

There is the issue with using CSS for opening/closing logic for flags. It's broken on touch devices (which don't trigger the :hover styling) (Issue #77). To fix it, we moved all the logic to JS:

Changed z-index to «move» a cursor behind the text. It resolves the issue, when a cursor prevent clicking on the text (even if a user clicks exactly on cursor)
Added listeners for mousemove and touchstart with a callback, which shows/closes flags depends on a point position

To achieve it there are added a couple of callbacks to update cursors position and store it in the quill-cursors module. It should be enough to cover all cases for touch and non-touch devices. Also, there is a small throttling for mousemove events to improve performance and decrease amount of callbacks executions

alecgibson

[moved comment inline]

alecgibson · 2022-03-10T14:08:16Z

src/quill-cursors/quill-cursors.ts

@@ -110,6 +116,26 @@ export default class QuillCursors {
  private _registerDomListeners(): void {
    const editor = this.quill.container.getElementsByClassName('ql-editor')[0];
    editor.addEventListener('scroll', () => this.update());
+
+    editor.addEventListener('mousemove', throttle(this._toggleNearCursors.bind(this), 100));


This feels pretty heavy-handed to me. Using a throttle like this is a bit of a code smell.

Did you try mouseenter and mouseleave? I think from experience they can be a bit flaky, so no big deal if they didn't work.

However, I'd rather we then move to use mouseover, and specifically attach to the elements we care about our mouse being inside. That would at least target our events a bit better, and would avoid needing to have the concept of being "near" the cursor.

I've tried it, but it doesn't work with z-index: -1.
But we need to have z-index: -1, otherwise a caret overflows the text and we can't really click on the text (also, keep in mind, that it should work with touch devices).

alecgibson · 2022-03-10T14:09:09Z

assets/quill-cursors.scss

-    padding: 0 $spacing-sm;
-    z-index: 1;
-
+    z-index: -1;


I think this might theoretically be breaking. If you have a background on your Quill instance, the carets will no longer be visible.

If we need this, then fine, but we should major version and add this to the changelog.

alecgibson · 2022-03-22T11:12:55Z

@fyvfyv thanks for your patience! I've done a bit of tinkering, and I've potentially got an avenue we can explore that doesn't require constantly listening for mousemove on the entire Quill root. There's a rough proof-of-concept here, where we toggle a class in JS on hover, but also allow clicking through to the underlying link.

The basic principles are:

Initially listen for mouseover on the cursor only, which vastly restricts the number of events we're handling
When we get a mouseover, then we apply pointer-events: none (and any hover state we want) — this then lets us click the underlying element.
We now start a small loop of one-off mouseover handlers on the document itself: when the mouse moves, we check if it's still intersecting the cursor. If it is, then we add another one-off handler. If it's not, then we remove the hover state and reset pointer-events, which lets us pick up a hover again.

This potentially handles a lot of events if a client makes extremely small movements within the cursor (thus staying in the manual mouseout check), but given that the cursor is so small, I think this is very unlikely.

What do you think?

fyvfyv · 2022-03-24T07:47:16Z

@alecgibson Yeah, it might work. I'm trying to implement it in this repo and let's check

alecgibson

It's possible right now to get the flag "stuck" open if you move your cursor away quickly. I don't know if this might be solved by following my suggested proof-of-concept method a bit closer (ie a recursive once loop, with appropriate setTimeout() calls)?

assets/quill-cursors.scss

src/quill-cursors/touch-device.ts

src/quill-cursors/cursor.ts

assets/quill-cursors.scss

alecgibson · 2022-04-12T16:35:05Z

src/quill-cursors/cursor.ts

@@ -106,6 +132,28 @@ export default class Cursor {
    selections.forEach((selection: ClientRect) => this._addSelection(selection, container));
  }

+  private _setHoverState(): void {
+    this._caretEl.classList.add(Cursor.CONTAINER_NO_POINTER_CLASS);
+    document.addEventListener('mouseover', this._toggleOpenedCursor);


We're still getting the flag "stuck" as per my previous comment. However, I think we can fix it if we change this to listen for mousemove instead:

Suggested change

document.addEventListener('mouseover', this._toggleOpenedCursor);

document.addEventListener('mousemove', this._toggleOpenedCursor);

alecgibson · 2022-04-12T16:36:22Z

src/quill-cursors/cursor.ts

@@ -29,11 +33,15 @@ export default class Cursor {
  private _hideDelay: string;
  private _hideSpeedMs: number;
  private _positionFlag: (flag: HTMLElement, caretRectangle: ClientRect, container: ClientRect) => void;
+  private _showFlagTimeout: NodeJS.Timeout;


alecgibson · 2022-04-12T16:40:53Z

src/quill-cursors/cursor.ts

@@ -106,6 +132,28 @@ export default class Cursor {
    selections.forEach((selection: ClientRect) => this._addSelection(selection, container));
  }

+  private _setHoverState(): void {
+    this._caretEl.classList.add(Cursor.CONTAINER_NO_POINTER_CLASS);


I don't think you need this line

alecgibson · 2022-04-12T16:43:21Z

src/quill-cursors/cursor.ts

+    document.removeEventListener('mouseover', this._toggleOpenedCursor);
+    if (!shouldShow) {
+      this._showFlagTimeout = null;
+      return;
+    }
+
+    this._showFlagTimeout = setTimeout(this._setHoverState, 100);


Not sure if I'm missing something here?

Suggested change

document.removeEventListener('mouseover', this._toggleOpenedCursor);

if (!shouldShow) {

this._showFlagTimeout = null;

return;

}

this._showFlagTimeout = setTimeout(this._setHoverState, 100);

if (!shouldShow) document.removeEventListener('mousemove', this._toggleOpenedCursor);

alecgibson · 2022-04-12T16:45:42Z

src/quill-cursors/cursor.ts

+    const {x, y, width, height} = this._caretEl.getBoundingClientRect();
+    this.coordinates = {x, y, width, height};


I think our maths would be more readable if we changed to:

Suggested change

const {x, y, width, height} = this._caretEl.getBoundingClientRect();

this.coordinates = {x, y, width, height};

const {left, right, top, bottom} = this._caretEl.getBoundingClientRect();

this.coordinates = {left, right, top, bottom};

alecgibson · 2022-04-12T16:48:53Z

src/quill-cursors/cursor.ts

+    const isYNear = pointY - y - height <= 0 && pointY - y >= 0;
+    const shouldShow = isXNear && isYNear;
+
+    this.toggleFlag(shouldShow);


toggleFlag() (deliberately) ignores the hideDelay option, but we should honour the hideDelay when moving our mouse away.

alecgibson · 2022-04-12T16:51:13Z

src/quill-cursors/cursor.ts

  public toggleFlag(shouldShow?: boolean): void {
-    const isShown = this._flagEl.classList.toggle(Cursor.SHOW_FLAG_CLASS, shouldShow);
-    if (isShown) return;


Why did we remove this early return?

alecgibson · 2022-04-12T16:57:34Z

src/quill-cursors/cursor.ts

+  }
+
+  private _updateCoordinates(): void {
+    const {x, y, width, height} = this._caretEl.getBoundingClientRect();


I think getBoundingClientRect() is an expensive function — rather than proactively update and track our coordinates, I think we should just ask for them when needed, so:

Remove the coordinates property

Change _updateCoordinates(): void -> _getCoordinates(): ICoordinates

Replace references to this.coordinates with this._getCoordinates()

src/quill-cursors/cursor.ts

src/quill-cursors/quill-cursors.ts

alecgibson

Couple of small tweaks, and I think we're there!

alecgibson · 2022-07-19T10:18:04Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "quill-cursors",
-  "version": "3.1.2",
+  "version": "3.2.0",


Let's be conservative and bump a major version, since we're changing (including removing) some CSS classes, which people might be overriding, etc.

Also please update the CHANGELOG with the breaking changes.

alecgibson · 2022-07-19T10:20:43Z

src/quill-cursors/quill-cursors.ts

+  }
+
+  private _handleCursorTouch(e: MouseEvent): void {
+    const touchFlagDelay = 2000;


Shouldn't we use this._hideDelay here?

Yeah, changed it to this.options.hideDelayMs

There is the issue with using CSS for opening/closing logic for flags. It's broken on touch devices (which don't trigger the `:hover` styling) (Issue #77). To fix it, we moved all the logic to JS: - Changed `z-index` to «move» a cursor behind the text. It resolves the issue, when a cursor prevent clicking on the text (even if a user clicks exactly on cursor) - Added listeners for `mousemove` and `touchstart` with a callback, which shows/closes flags depends on a point position To achieve it there are added a couple of callbacks to update cursors position and store it in the quill-cursors module. It should be enough to cover all cases for touch and non-touch devices. Also, there is a small throttling for mousemove events to improve performance and decrease amount of callbacks executions

alecgibson

🎉

fyvfyv requested a review from alecgibson March 7, 2022 10:36

alecgibson reviewed Mar 10, 2022

View reviewed changes

fyvfyv force-pushed the touch-other-users-cursors branch 2 times, most recently from 6d7e1b4 to 7aaa8d9 Compare April 4, 2022 17:04

fyvfyv requested a review from alecgibson April 4, 2022 17:56

alecgibson reviewed Apr 5, 2022

View reviewed changes

fyvfyv force-pushed the touch-other-users-cursors branch from 7aaa8d9 to bd73218 Compare April 12, 2022 09:06

fyvfyv requested a review from alecgibson April 12, 2022 09:10

fyvfyv force-pushed the touch-other-users-cursors branch 2 times, most recently from 29fae41 to 0badba7 Compare April 12, 2022 09:15

alecgibson reviewed Apr 12, 2022

View reviewed changes

fyvfyv force-pushed the touch-other-users-cursors branch from 0badba7 to c48be2b Compare June 22, 2022 14:12

fyvfyv force-pushed the touch-other-users-cursors branch from c48be2b to ff7c5ce Compare July 12, 2022 09:41

fyvfyv requested a review from alecgibson July 12, 2022 09:42

alecgibson reviewed Jul 12, 2022

View reviewed changes

src/quill-cursors/cursor.ts Outdated Show resolved Hide resolved

src/quill-cursors/quill-cursors.ts Outdated Show resolved Hide resolved

fyvfyv force-pushed the touch-other-users-cursors branch from ff7c5ce to 051adf4 Compare July 18, 2022 15:16

fyvfyv requested a review from alecgibson July 18, 2022 15:25

alecgibson reviewed Jul 19, 2022

View reviewed changes

fyvfyv force-pushed the touch-other-users-cursors branch from 051adf4 to d4a57f9 Compare July 26, 2022 07:24

fyvfyv requested a review from alecgibson July 26, 2022 07:28

alecgibson approved these changes Jul 26, 2022

View reviewed changes

alecgibson merged commit 3474541 into main Jul 26, 2022

alecgibson deleted the touch-other-users-cursors branch July 26, 2022 09:08

alecgibson mentioned this pull request Jul 26, 2022

Can't touch near other user's cursors #77

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ Change flag opening/closing logic #80

♻️ Change flag opening/closing logic #80

fyvfyv commented Mar 7, 2022

alecgibson left a comment •

edited

alecgibson Mar 10, 2022

fyvfyv Mar 10, 2022

alecgibson Mar 10, 2022

alecgibson commented Mar 22, 2022

fyvfyv commented Mar 24, 2022

alecgibson left a comment

alecgibson Apr 12, 2022

alecgibson Apr 12, 2022

alecgibson Apr 12, 2022

alecgibson Apr 12, 2022

alecgibson Apr 12, 2022

alecgibson Apr 12, 2022

alecgibson Apr 12, 2022

alecgibson Apr 12, 2022

alecgibson left a comment

alecgibson Jul 19, 2022

alecgibson Jul 19, 2022

fyvfyv Jul 26, 2022

alecgibson left a comment

	document.addEventListener('mouseover', this._toggleOpenedCursor);
	document.addEventListener('mousemove', this._toggleOpenedCursor);

		const {x, y, width, height} = this._caretEl.getBoundingClientRect();
		this.coordinates = {x, y, width, height};

♻️ Change flag opening/closing logic #80

♻️ Change flag opening/closing logic #80

Conversation

fyvfyv commented Mar 7, 2022

alecgibson left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecgibson commented Mar 22, 2022

fyvfyv commented Mar 24, 2022

alecgibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecgibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecgibson left a comment

Choose a reason for hiding this comment

alecgibson left a comment •

edited