Activate cursorBack when clicking the hidden thought by onurpolattimur · Pull Request #1128 · cybersemics/em

onurpolattimur · 2021-05-07T18:53:52Z

This issue occurs because we are clicking the editable area of hidden thoughts. I added border for .editable class to show you the boundaries of editable elements.

There is already a function written for handle this situation isElementHiddenByAutoFocus . But the problem was in tap handler, it was not entering the if block because editingOrOnCursor is true when we click the area of d

em/src/components/Editable.tsx

Lines 612 to 630 in e303756

    
             const isHiddenByAutofocus = isElementHiddenByAutoFocus(e.target as HTMLElement) 
        
             const editingOrOnCursor = state.editing || equalPath(path, state.cursor) 
        
             if (disabled || ( 
        
               !globals.touching && 
        
               // not sure if this can happen, but I observed some glitchy behavior with the cursor moving when a drag and drop is completed so check dragInProgress to be safe 
        
               !state.dragInProgress && 
        
               !editingOrOnCursor 
        
             )) { 
        
               e.preventDefault() 
        
               if (isHiddenByAutofocus) { 
        
                 dispatch(cursorBack()) 
        
               } 
        
               else { 
        
                 // prevent focus to allow navigation with mobile keyboard down 
        
                 setCursorOnThought() 
        
               } 
        
             } 
        
           }

I was thinking solve this issue with css visible: hidden property but this will cause other issues. For example, if we add .distance-from-cursor-3 > .child { visibility: hidden }then we can not see the thoughts in .distance-from-cursor-3 We were going to need to add other css properties for visible thughts in .distance-from-cursor-3

raineorshine

I appreciate the succinct description of the cause of the problem, existing infrastructure, and choice of solution. I tested and it fixes the issue, but it unfortunately breaks a test case when tapping on an uncle.

This only occurs when isTouch is true. Tested on mobile safari and emulated on desktop by hardcoding isTouch = true.

Steps to Reproduce

- a
  - b
    - c
  - d

(d is indented one level compared to the original test case in #1029.)

Set the cursor on c
Tap one line height below c, just to the right of where d would be.

Current Behavior

Cursor moves to a.

Expected Behavior

Cursor and caret should move to d.

onurpolattimur · 2021-05-08T15:25:36Z

Hi @raineorshine, I reproduced your issue and I saw that onClick event of the Content div triggered. First, I suspected event bubbling, but it was not. In onTap of Editable we have already prevented the default events.

The problem was that if we click the thought 'd', the 'b' thought will be collapsed and the place where we touch up is on Content element and this will trigger the onClick event of Content.

I made some changes to Content component. Can you review it, please?

onurpolattimur · 2021-05-08T17:34:02Z

src/components/Content.tsx

-  const clickOnEmptySpace = () => {
+  const clickOnEmptySpace = (e: any) => {
+    // Stop event bubbling. We need to handle this event if we click really on content element not other elements.
+    if (e.target.id !== 'content') {


I couldn't get id from React.MouseEvent so I used any for here.

raineorshine

f546841 results in incorrect behavior when clicking on empty space.

Can you see if this is reproducible on desktop with isTouch = true? That will tell us if we can write an e2e case for this as well.

Steps to Reproduce

- a
  - b
    - c
  - d

Set the cursor on c
Tap on empty space below d

Current Behavior

If edit mode is enabled, edit mode is disabled and the cursor doesn't move.
If edit mode is disabled, the cursor moves to a.

Expected Behavior

Cursor and caret should move to ~~d~~ b.

raineorshine · 2021-05-10T20:51:56Z

Hi @raineorshine, I reproduced your issue and I saw that onClick event of the Content div triggered. First, I suspected event bubbling, but it was not. In onTap of Editable we have already prevented the default events.

I apologize if I am misunderstanding, but preventDefault doesn't have any effect on event bubbling. e.preventDefault() prevents the default browser behavior (such as navigating to the href of an <a>) and e.stopPropagation() prevents the same event from firing on ancestors (event bubbling). Here's a Code Sandbox that demonstrates this more clearly: https://codesandbox.io/s/divine-morning-0cw49?file=/src/App.js.

The problem was that if we click the thought 'd', the 'b' thought will be collapsed and the place where we touch up is on Content element and this will trigger the onClick event of Content.

I'm not sure I understand yet. Can you explain this more? What happens before the Content onClick and why does it get that far?

onurpolattimur · 2021-05-10T22:19:36Z

Yes, but e.stopPropogation() doesn't work for touch events. But we can prevent event bubbling using e.preventDefault() for touch events. Here is an example for you, you can test in https://qf8e8.csb.app/ with forcing mobile view by developer tools.
Source codes: https://codesandbox.io/s/withered-water-qf8e8?file=/src/App.js

But our problem was not about event bubbling. I will try to explain more clearly.
First of all, onClick events are triggered after we release the mouse button or release the touch.

- a
  - b
    - c
  - d

Think about the case above, when the cursor is on c and we touch the thought 'd', context 'b' will be collapsed, and We won't have released the touch yet, the current touch position will be on Content div, and after we release the touch, the onClick event of Content div will be triggered.

I will also look into the case you wrote and will try to make some explanation.

raineorshine · 2021-05-10T22:31:35Z

Yes, but e.stopPropogation() doesn't work for touch events. But we can prevent event bubbling using e.preventDefault() for touch events. Here is an example for you, you can test in qf8e8.csb.app with forcing mobile view by developer tools.
Source codes: codesandbox.io/s/withered-water-qf8e8?file=/src/App.js

Okay, thanks! I didn't know the touch and click events were competing like that.

But our problem was not about event bubbling. I will try to explain more clearly.
First of all, onClick events are triggered after we release the mouse button or release the touch.
- a
  - b
    - c
  - d
Think about the case above, when the cursor is on c and we touch the thought 'd', context 'b' will be collapsed, and We won't have released the touch yet, the current touch position will be on Content div, and after we release the touch, the onClick event of Content div will be triggered.

Got it, that makes sense now!

onurpolattimur · 2021-05-11T22:52:24Z

I made an in-depth review. I think the only option is that using the onMouseDown event instead of onClick. I explained the reason why we should not use onClick above.

But when we use onMouseDown, another issue will occur. Because click/touch operation is not completed yet selection will be on Editable, so it will not pass the condition below.

So I added a guard, to make sure onClick is triggered only if the user starts the action by pressing the Content element.

em/src/components/Content.tsx

Lines 108 to 110 in a08761a

    
           const selection = window.getSelection() 
        
           const focusNode = selection && selection.focusNode 
        
           if (focusNode && focusNode.nodeType === Node.TEXT_NODE) return

onurpolattimur added 2 commits May 7, 2021 21:47

activate cursorBack when clicking the hidden thought

7e65df6

add e2e test

230696b

onurpolattimur force-pushed the fix-1029 branch from 2ca9cfe to 230696b Compare May 7, 2021 19:18

raineorshine requested changes May 7, 2021

View reviewed changes

use touchEnd and mouseDown instead of onclick for Content div

f546841

onurpolattimur requested a review from raineorshine May 8, 2021 15:27

onurpolattimur commented May 8, 2021

View reviewed changes

raineorshine requested changes May 10, 2021

View reviewed changes

add guard for onClick event of Content element

56cdae1

Content: isPressed clean up.

b56e94b

raineorshine approved these changes May 11, 2021

View reviewed changes

raineorshine merged commit 4a59734 into dev May 12, 2021

raineorshine deleted the fix-1029 branch May 12, 2021 00:30

raineorshine mentioned this pull request May 12, 2021

Cursor moves to thought hidden by autofocus #1029

Closed

onurpolattimur mentioned this pull request May 12, 2021

stop propagation for MouseEvent of Editable #1135

Merged

raineorshine mentioned this pull request May 15, 2021

Mobile: cursorUp incorrectly activated on gesture #1147

Closed

This was referenced Jun 15, 2021

Add ios e2e test for 'Tab hidden uncle' case #1280

Merged

Add ios e2e test for 'Tap empty content' case #1281

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Activate cursorBack when clicking the hidden thought#1128

Activate cursorBack when clicking the hidden thought#1128
raineorshine merged 5 commits intodevfrom
fix-1029

onurpolattimur commented May 7, 2021 •

edited

Loading

Uh oh!

raineorshine left a comment •

edited

Loading

Uh oh!

onurpolattimur commented May 8, 2021 •

edited

Loading

Uh oh!

onurpolattimur May 8, 2021

Uh oh!

raineorshine left a comment •

edited

Loading

Uh oh!

raineorshine commented May 10, 2021

Uh oh!

onurpolattimur commented May 10, 2021

Uh oh!

raineorshine commented May 10, 2021

Uh oh!

onurpolattimur commented May 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	const isHiddenByAutofocus = isElementHiddenByAutoFocus(e.target as HTMLElement)
	const editingOrOnCursor = state.editing \|\| equalPath(path, state.cursor)

	if (disabled \|\| (
	!globals.touching &&
	// not sure if this can happen, but I observed some glitchy behavior with the cursor moving when a drag and drop is completed so check dragInProgress to be safe
	!state.dragInProgress &&
	!editingOrOnCursor
	)) {
	e.preventDefault()
	if (isHiddenByAutofocus) {
	dispatch(cursorBack())
	}
	else {
	// prevent focus to allow navigation with mobile keyboard down
	setCursorOnThought()
	}
	}
	}

Conversation

onurpolattimur commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raineorshine left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Steps to Reproduce

Current Behavior

Expected Behavior

Uh oh!

onurpolattimur commented May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onurpolattimur May 8, 2021

Choose a reason for hiding this comment

Uh oh!

raineorshine left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Steps to Reproduce

Current Behavior

Expected Behavior

Uh oh!

raineorshine commented May 10, 2021

Uh oh!

onurpolattimur commented May 10, 2021

Uh oh!

raineorshine commented May 10, 2021

Uh oh!

onurpolattimur commented May 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

onurpolattimur commented May 7, 2021 •

edited

Loading

raineorshine left a comment •

edited

Loading

onurpolattimur commented May 8, 2021 •

edited

Loading

raineorshine left a comment •

edited

Loading