Skip to content

fix(core): clean up event-handling in ViewCommon #10534

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
May 2, 2024
Merged

fix(core): clean up event-handling in ViewCommon #10534

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
923133f
chore(core): clean up addEventListener() and removeEventListener() in…
shirakaba May 1, 2024
b489207
chore(core): clean up _gestureObservers
shirakaba May 1, 2024
6311592
fix(core): remove redundant event name splitting
shirakaba May 1, 2024
b006365
Merge branch 'main' into gesture-events-cleanup
NathanWalker May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 45 additions & 53 deletions packages/core/ui/core/view/view-common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ export abstract class ViewCommon extends ViewBase implements ViewDefinition {
_setMinWidthNative: (value: CoreTypes.LengthType) => void;
_setMinHeightNative: (value: CoreTypes.LengthType) => void;

public _gestureObservers = {};
public readonly _gestureObservers = {} as Record<GestureTypes, Array<GesturesObserver>>;

_androidContentDescriptionUpdated?: boolean;

Expand Down Expand Up @@ -177,7 +177,7 @@ export abstract class ViewCommon extends ViewBase implements ViewDefinition {

onLoaded() {
if (!this.isLoaded) {
const hasTap = this.hasListeners('tap') || this.hasListeners('tapChange') || this.getGestureObservers(GestureTypes.tap);
const hasTap = this.hasListeners('tap') || this.hasListeners('tapChange') || !!this.getGestureObservers(GestureTypes.tap);
const enableTapAnimations = TouchManager.enableGlobalTapAnimations && hasTap;
if (!this.ignoreTouchAnimation && (this.touchAnimation || enableTapAnimations)) {
TouchManager.addAnimations(this);
Expand Down Expand Up @@ -286,62 +286,52 @@ export abstract class ViewCommon extends ViewBase implements ViewDefinition {
this._gestureObservers[type].push(gestureObserve(this, type, callback, thisArg));
}

public getGestureObservers(type: GestureTypes): Array<GesturesObserver> {
public getGestureObservers(type: GestureTypes): Array<GesturesObserver> | undefined {
return this._gestureObservers[type];
}

public addEventListener(arg: string | GestureTypes, callback: (data: EventData) => void, thisArg?: any) {
if (typeof arg === 'string') {
arg = getEventOrGestureName(arg);
if (typeof arg === 'number') {
this._observe(arg, callback as unknown as (data: GestureEventData) => void, thisArg);
return;
}

const gesture = gestureFromString(arg);
if (gesture && !this._isEvent(arg)) {
this._observe(gesture, callback as unknown as (data: GestureEventData) => void, thisArg);
} else {
const events = arg.split(',');
if (events.length > 0) {
for (let i = 0; i < events.length; i++) {
const evt = events[i].trim();
const gst = gestureFromString(evt);
if (gst && !this._isEvent(arg)) {
this._observe(gst, callback as unknown as (data: GestureEventData) => void, thisArg);
} else {
super.addEventListener(evt, callback, thisArg);
}
}
} else {
super.addEventListener(arg, callback, thisArg);
}
}
} else if (typeof arg === 'number') {
this._observe(<GestureTypes>arg, callback as unknown as (data: GestureEventData) => void, thisArg);
// Normalize "ontap" -> "tap"
const normalizedName = getEventOrGestureName(arg);

// Coerce "tap" -> GestureTypes.tap
// Coerce "loaded" -> undefined
const gesture: GestureTypes | undefined = gestureFromString(normalizedName);
Copy link
Contributor Author

@shirakaba shirakaba May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Desperate for TypeScript strict mode 😔


// If it's a gesture (and this Observable declares e.g. `static tapEvent`)
if (gesture && !this._isEvent(normalizedName)) {
Comment on lines +306 to +307
Copy link
Contributor Author

@shirakaba shirakaba May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A very curious case is when it's a gesture (e.g. "tap") and yet the View hasn't declared static tapEvent. We'll fall through to the next case, which just treats it as a regular event named "tap" rather than setting up a tap gesture observer. This has long/always been the case, so I decided not to change it.

I don't have the appetite to change it during this PR that's just focused on cleanup, but any thoughts about this anyway? Why is it necessary in the first place to declare static tapEvent on a ViewBase in order for it to be treated as a gesture event? Is that still an appropriate idea, or would it make sense to remove those across all classes by removing this condition?

Copy link
Contributor

Choose a reason for hiding this comment

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

Those static properties are for vanilla to be able to distinguish properties from events because XML format does not help in that matter.

We could get rid of those static properties across all classes possibly by doing a breaking change on vanilla and expect event declarations to have something like on prefix in XML.

<!-- 'on' helps distinguish events from properties -->
<Label text="Hello world" on:tap="dosomething"/>

this._observe(gesture, callback as unknown as (data: GestureEventData) => void, thisArg);
return;
}

super.addEventListener(normalizedName, callback, thisArg);
}

public removeEventListener(arg: string | GestureTypes, callback?: (data: EventData) => void, thisArg?: any) {
if (typeof arg === 'string') {
const gesture = gestureFromString(arg);
if (gesture && !this._isEvent(arg)) {
this._disconnectGestureObservers(gesture);
} else {
const events = arg.split(',');
if (events.length > 0) {
for (let i = 0; i < events.length; i++) {
const evt = events[i].trim();
const gst = gestureFromString(evt);
if (gst && !this._isEvent(arg)) {
this._disconnectGestureObservers(gst);
} else {
super.removeEventListener(evt, callback, thisArg);
}
}
} else {
super.removeEventListener(arg, callback, thisArg);
}
}
} else if (typeof arg === 'number') {
this._disconnectGestureObservers(<GestureTypes>arg);
if (typeof arg === 'number') {
this._disconnectGestureObservers(arg);
return;
}

// Normalize "ontap" -> "tap"
const normalizedName = getEventOrGestureName(arg);

// Coerce "tap" -> GestureTypes.tap
// Coerce "loaded" -> undefined
const gesture: GestureTypes | undefined = gestureFromString(normalizedName);

// If it's a gesture (and this Observable declares e.g. `static tapEvent`)
if (gesture && !this._isEvent(normalizedName)) {
this._disconnectGestureObservers(gesture);
return;
}

super.removeEventListener(normalizedName, callback, thisArg);
}

public onBackPressed(): boolean {
Expand Down Expand Up @@ -379,7 +369,7 @@ export abstract class ViewCommon extends ViewBase implements ViewDefinition {
const firstArgument = args[0];
const view = firstArgument instanceof ViewCommon ? firstArgument : <ViewCommon>Builder.createViewFromEntry({
moduleName: firstArgument,
});
});

return { view, options };
}
Expand Down Expand Up @@ -501,10 +491,12 @@ export abstract class ViewCommon extends ViewBase implements ViewDefinition {

private _disconnectGestureObservers(type: GestureTypes): void {
const observers = this.getGestureObservers(type);
if (observers) {
for (let i = 0; i < observers.length; i++) {
observers[i].disconnect();
}
if (!observers) {
return;
}

for (const observer of observers) {
observer.disconnect();
}
}

Expand Down
Loading