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

Pointer Event fixes #1687

Merged
merged 7 commits into from
Mar 12, 2018
Merged

Pointer Event fixes #1687

merged 7 commits into from
Mar 12, 2018

Conversation

samelhusseini
Copy link
Contributor

@samelhusseini samelhusseini commented Mar 2, 2018

Fix pointer event (edge case) issues described in #1655

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

#1655

Proposed Changes

Pointer events:
A couple of major changes with the way we're handling pointer events:
Always use pointer events, and no other mouse event. We were using a mixture of those, since we only had 'mousedown', 'mousemove' and 'mouseup' defined in the map. I added the rest of the mouse events, there were used in a couple of places where we attached events to mouseover, and so I just added mapping for the suite of mouse events. I also added a couple of mappings for touchend and touchcancel (this is really a workaround) since there are a couple of events that we register only for touch (eg: longstop) that we want to register for pointer events.

Another major change is how we register pointer events. If they're available, they should be a replacement to mouse events. and in the same way that we don't add additionally hooks to preventDefault on mouse events (only for touch), we don't need to do that for pointer events. So I've shuffled the event registration code a little so that it registers pointer events with the wrapFunc function and not the touchWrapFunc function.
(In other cases, we register both mouse and touch, if mappings exist).

Comments:
Another fix to comments was to pass opt_noPreventDefault with the registered mouseup event. This was causing the gesture code to capture the event (even though the gesture code cancels on native elements like textarea) it was still capturing the event since the event (as far as it was concerned) was "handled". This was only affecting touch events, such as IOS.

Added an optimization to the bubble logic, only bumping it if it's not currently the top bubble. This also feeds into the comment focus logic where I only re-focus the text area if it has been moved within the bubble canvas.

Context menu:
Regarding the issue with the longStop: I just added a check to ensure the pointer type is not a mouse (since pointer events can be 'mouse', 'touch' or 'pen).

Reason for Changes

Fixes issues described in #1655

Test Coverage

Tested on:
Desktop Chrome on Mac (latest)
Desktop Firefox (latest) and (beta, Firefox 59, supports pointer events) on Mac.
Desktop Safari on Mac.
Desktop Chrome on Surface Book (Windows).
Desktop Firefox on Sufrace Book (Windows).
IE 11 on Surface Book
Edge 16 on Surface Book
IOS (Safari latest).
Chrome Android (although there is a known issue with pointer events on Android, we might want to either exclude that specific version of Android or Android altogether from using pointer events until the next version of Chrome is released).
Firefox Android

commentfix

Additional Information

Note, with my changes all interactions with text areas inside comments are handled completely by the browser. Blockly usually tries to limit this behaviour so that the browser doesn't do some fancy zoom, but in this case, in order to allow things like selecting parts of the text or jumping to a different section in the comment, we need to let the browser take care of that.

core/css.js Outdated
'margin: 0;',
'padding: 2px;',
'resize: none;',
'display: block;',
'overflow: none;',
Copy link
Contributor Author

@samelhusseini samelhusseini Mar 9, 2018

Choose a reason for hiding this comment

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

I think someone else already has a PR for the issue this is trying to fix (Chrome hides the textarea if the content of the text is too long that it needs to scroll), Chrome is basically broken here. The two options I see are:

  • do the overflow: none fix, which fixes this issue on Chrome, but then scrolling doesn't work on any browser, and a user would have to expand the bubble to see the full contents of the comment.
  • dont do the fix, and wait for Chrome to fix the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Only overflow: hidden in #1689. I was hesitant to disable scrolling.

Copy link
Contributor Author

@samelhusseini samelhusseini Mar 9, 2018

Choose a reason for hiding this comment

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

Yeah sorry, no such thing as overflow none. hidden is what I meant. will update

@rachel-fenichel rachel-fenichel merged commit 40bb0d1 into google:develop Mar 12, 2018
@samelhusseini samelhusseini mentioned this pull request Mar 19, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants