From 83b22bf8ef732d4f8d93200a043d5de2439aef82 Mon Sep 17 00:00:00 2001 From: Alice Pote Date: Fri, 26 Aug 2022 13:49:34 -0700 Subject: [PATCH 1/2] fix issue with sometimes-null refs on datetime component This fixes an issue with the Datetime component where in certain circumstances a `ref` on an element rendered by the datetime component would end up erroneously set to `null`, necessitating a yucky `document.querySelector` based workaround. The issue arises because of how Stencil's virtual DOM implementation handles reconciling the children of old and new nodes, and in particular how it deals with situations in which the order of children changes but some of the nodes are conserved. The way this was affecting the datetime component was that in certain circumstances a method which returns a `
` with a `ref` would change position in the children of another node. In particular, this can happen if the `presentation` prop changes, causing the return value of the `renderDatetime` function to change in turn. The salient changes to the return value of _that_ function which would change look like going from: ```ts [ this.renderCalendar(mode), this.renderTime() ] ``` to ```ts [ this.renderTime(), this.renderCalendar(mode) ] ``` `renderCalendar` in turn calls `renderCalendarBody` which returns the `
` with the offending `ref` on it. Anyhow, in reconciling these changed children when re-rendering the component things can go wrong. Stencil's algorithm in this case does a "best effort" at identifying nodes which should be kept between re-renders, but it does not do an exhaustive check _unless nodes have a key attr_. So, more or less, what was happening was that in cases where the `
` with the `ref` on it was going to be included in the next rendered output from the component Stencil would nonetheless remove the 'old' node and create a new identical one for the next render. Then there is also a race condition where the new node would be created before the old one is removed, causing the `ref` to be briefly set to the new value (when the new VNode is created) and then reset to `null` (when the old one was destroyed). The fix is to set the `key` attribute on the same `
` as the `ref`. This allows Stencil to do an exhaustive check when reconciling old and new children in order to not needlessly throw away the old VDom node, and gets rid of the behavior where the `ref` would end up set to `null` erroneously. It's possible (but I think unlikely) that this change will result in slightly less GC pressure, but I did no testing of that. closes ionic-team/stencil#3253 FW-901 --- core/src/components/datetime/datetime.tsx | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/core/src/components/datetime/datetime.tsx b/core/src/components/datetime/datetime.tsx index c63d6eb5153..63833d9b008 100644 --- a/core/src/components/datetime/datetime.tsx +++ b/core/src/components/datetime/datetime.tsx @@ -629,18 +629,8 @@ export class Datetime implements ComponentInterface { return presentation === 'date' || presentation === 'date-time' || presentation === 'time-date'; } - /** - * Stencil sometimes sets calendarBodyRef to null on rerender, even though - * the element is present. Query for it manually as a fallback. - * - * TODO(FW-901) Remove when issue is resolved: https://github.com/ionic-team/stencil/issues/3253 - */ - private getCalendarBodyEl = () => { - return this.calendarBodyRef || this.el.shadowRoot?.querySelector('.calendar-body'); - }; - private initializeKeyboardListeners = () => { - const calendarBodyRef = this.getCalendarBodyEl(); + const calendarBodyRef = this.calendarBodyRef; if (!calendarBodyRef) { return; } @@ -818,7 +808,7 @@ export class Datetime implements ComponentInterface { }; private initializeCalendarListener = () => { - const calendarBodyRef = this.getCalendarBodyEl(); + const calendarBodyRef = this.calendarBodyRef; if (!calendarBodyRef) { return; } @@ -1241,7 +1231,7 @@ export class Datetime implements ComponentInterface { }; private nextMonth = () => { - const calendarBodyRef = this.getCalendarBodyEl(); + const calendarBodyRef = this.calendarBodyRef; if (!calendarBodyRef) { return; } @@ -1261,7 +1251,7 @@ export class Datetime implements ComponentInterface { }; private prevMonth = () => { - const calendarBodyRef = this.getCalendarBodyEl(); + const calendarBodyRef = this.calendarBodyRef; if (!calendarBodyRef) { return; } @@ -1980,7 +1970,7 @@ export class Datetime implements ComponentInterface { } private renderCalendarBody() { return ( -
(this.calendarBodyRef = el)} tabindex="0"> +
this.calendarBodyRef = el} tabindex="0"> {generateMonths(this.workingParts).map(({ month, year }) => { return this.renderMonth(month, year); })} @@ -1989,7 +1979,7 @@ export class Datetime implements ComponentInterface { } private renderCalendar(mode: Mode) { return ( -
+
{this.renderCalendarHeader(mode)} {this.renderCalendarBody()}
From c1c82bf84f881a0b3eef6de5fa88401e6c3443e4 Mon Sep 17 00:00:00 2001 From: Alice Pote Date: Fri, 26 Aug 2022 14:00:08 -0700 Subject: [PATCH 2/2] actually don't change that line --- core/src/components/datetime/datetime.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/components/datetime/datetime.tsx b/core/src/components/datetime/datetime.tsx index 63833d9b008..7cd9b842e5c 100644 --- a/core/src/components/datetime/datetime.tsx +++ b/core/src/components/datetime/datetime.tsx @@ -1970,7 +1970,7 @@ export class Datetime implements ComponentInterface { } private renderCalendarBody() { return ( -
this.calendarBodyRef = el} tabindex="0"> +
(this.calendarBodyRef = el)} tabindex="0"> {generateMonths(this.workingParts).map(({ month, year }) => { return this.renderMonth(month, year); })}