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

Remove touchInputEnabled and pointerEventsEnabled #1576

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jamesmaa
Copy link
Collaborator

@jamesmaa jamesmaa commented Nov 6, 2024

Fixes #466

I just followed instructions of removing the option and related code and very lightly tested functionality

@jamesmaa jamesmaa requested a review from a team as a code owner November 6, 2024 18:50
Copy link

codspeed-hq bot commented Nov 6, 2024

CodSpeed Performance Report

Merging #1576 will not alter performance

Comparing jamesmaa:remove-scan-inputs (ac98aa6) with master (77edfc7)

Summary

✅ 3 untouched benchmarks

Comment on lines 1156 to 1165
} else if (this._arePointerEventsSupported()) {
eventListenerInfos = this._getPointerEventListeners(capture);
} else {
eventListenerInfos = [...this._getMouseEventListeners(capture)];
if (this._scanWithoutMousemove) {
eventListenerInfos.push(...this._getKeyboardEventListeners(capture));
}
if (this._touchInputEnabled) {
eventListenerInfos.push(...this._getTouchEventListeners(capture));
}

Choose a reason for hiding this comment

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

We still need to call these functions to register the event listeners for touch and pen. Only need to remove the if check for this._touchInputEnabled and this._pointerEventsEnabled since we are removing these options and having them true by default.

Copy link
Member

Choose a reason for hiding this comment

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

We might only want one or the other. According to #466, only pointer events should be required. Though I've never seen any browser that doesn't support both equally.

Choose a reason for hiding this comment

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

Maybe need to test if this breaks touch behavior on popup components.

Copy link
Collaborator Author

@jamesmaa jamesmaa Nov 6, 2024

Choose a reason for hiding this comment

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

So is the ask here to restore

        } else if (this._arePointerEventsSupported()) {
            eventListenerInfos = this._getPointerEventListeners(capture);

and keep this line?

Copy link

github-actions bot commented Nov 6, 2024

Playwright test results

failed  1 failed
passed  4 passed

Details

stats  5 tests across 4 suites
duration  4 minutes, 44 seconds
commit  77edfc7

Failed tests

chromium › visual.spec.js › visual

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.

"Support inputs for devices with touch screens" option and related code should be removed
3 participants