fix(engine): forcing composed: true for focusout events #465

davidturissini · 2018-07-03T00:31:55Z

Details

Fixes issue in firefox where native focusout events are not composed: true

Does this PR introduce a breaking change?

Yes
No

caridy · 2018-07-03T00:35:33Z

This is definitely a breaking change since existing events might be crossing the boundary today, and after this PR is merged, some of them might not cross it anymore.

caridy · 2018-07-03T00:35:45Z

packages/lwc-engine/src/framework/def.ts

+            get(this: FocusEvent) {
+                const { type, isTrusted } = this;
+                const composed = originalComposedGetter.call(this);
+                if (isTrusted && type === 'focusout' && composed === false) {


what about focusin?

davidturissini · 2018-07-03T00:36:50Z

@caridy This branch is based on #462, which has the breaking changes. I had to base this off of that branch because I needed the getter in Event.prototype

caridy · 2018-07-03T00:38:25Z

packages/lwc-engine/src/framework/def.ts

+    // for `focusout` events
+    // This is defined here because we are guaranteed
+    // to have the composed polyfill applied to Event.
+    const originalComposedGetter = Object.getOwnPropertyDescriptor(Event.prototype, 'composed')!.get!;


we should definitely link to firefox issue, so we can remove this once they fix it.

pmdartus · 2018-07-03T03:08:57Z

packages/lwc-engine/src/framework/def.ts

+    // This is defined here because we are guaranteed
+    // to have the composed polyfill applied to Event.
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1472887
+    const originalComposedGetter = Object.getOwnPropertyDescriptor(Event.prototype, 'composed')!.get!;


What about adding the code to the existing polyfill or in another one?
I feel this code should not be part of the engine.

The polyfill and this code are not solving the same problem. This code runs in environments where the polyfill isn’t needed.

Can we create another polyfill then? The same way we have the Safari Proxy fix?

Actually, I didn't realized that this code was here in def.ts. We should definitely move this to the outer folder in polyfill

pmdartus · 2018-07-03T03:11:49Z

packages/lwc-engine/src/framework/def.ts

+            get(this: FocusEvent) {
+                const { type, isTrusted } = this;
+                const composed = originalComposedGetter.call(this);
+                if (isTrusted && (type === 'focusout' || type === 'focusin') && composed === false) {


The focus should be applied to the all the events implementing the FocusEvent interface, not only focusin and focusout.

https://www.w3.org/TR/uievents/#events-focus-types

focus and blur events behave correctly

By not checking the type we can avoid the performance overhead of the branching.

+1 to remove string comparisons!

pmdartus · 2018-07-03T03:16:53Z

...mponents/events/test-focusout-composed-true/focusout-composed-true/focusout-composed-true.js

+    }
+
+    handleButtonClick() {
+        this.template.querySelector('.custom-focus-out').dispatchEvent(new CustomEvent('focusout', { bubbles: true, composed: false }));


Can we also add a test for new FocusEvent('focusout')?

Not supported in IE11, so I don’t think we support this

pmdartus · 2018-07-03T03:17:33Z

.../components/events/test-focusin-composed-true/focusin-composed-true/focusin-composed-true.js

+        this.customEventNotComposed = evt.composed === false;
+    }
+
+    handleButtonClick() {


I am not sure if it's valuable to keep multiple tests since all the FocusEvents are composed.

This is to ensure that users can still define a custom focus event with composed: false

pmdartus · 2018-07-03T17:55:22Z

packages/lwc-engine/src/framework/def.ts

@@ -355,6 +355,7 @@ export function getComponentDef(Ctor: ComponentConstructor): ComponentDef {
 // Initialization Routines
 import "../polyfills/proxy-concat/main";
 import "../polyfills/event-composed/main";
+import "../polyfills/focuse-event-composed/main";


~~focuse~~ -> focus

pmdartus · 2018-07-03T17:56:43Z

packages/lwc-engine/src/polyfills/focus-event-composed/polyfill.ts

@@ -0,0 +1,23 @@
+// This must be run AFTER our event-composed polyfill
+export default function () {
+     // Fix for FF not respecting spec'd composed flat


We should add a README.md explaining the issue with the link to FF bug like the other polyfill.

pmdartus · 2018-07-03T17:58:30Z

packages/lwc-engine/src/polyfills/focus-event-composed/detect.ts

@@ -0,0 +1,3 @@
+export default function () {
+    return true;


Why adding this polyfill by default, this should only be applied to Firefox. We should conditionally execute the polyfill for unsupported browsers.

We have no real way to detect this behavior.

I guess you are right, haven't found a way to test that out. In this case, I would also add a comment explaining why we can't gate the polyfill injection.

yeah, once they fix this in FE, we can remove this. No worries.

salesforce-best-lwc-internal · 2018-07-05T17:13:30Z

Benchmark results

Base commit: 312bed0 | Target commit: 46367d2

lwc-engine-benchmark

table-append-1k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table/append/1k	duration	154.30 (± 5.60 ms)	153.10 (± 5.20 ms)	0.78% 👌

table-clear-1k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table/clear/1k	duration	12.40 (± 0.70 ms)	12.10 (± 0.50 ms)	2.42% 👌

table-create-10k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table/create/10k	duration	893.40 (± 5.70 ms)	869.20 (± 5.70 ms)	2.71% 👍

table-create-1k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table/create/1k	duration	103.30 (± 2.40 ms)	104.10 (± 1.70 ms)	-0.77% 👌

table-update-10th-1k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table/update-10th/1k	duration	84.90 (± 4.70 ms)	87.50 (± 5.20 ms)	-3.06% 👌

tablecmp-append-1k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table-component/append/1k	duration	222.70 (± 3.85 ms)	231.00 (± 4.30 ms)	-3.73% 👎

tablecmp-clear-1k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table-component/clear/1k	duration	35.20 (± 2.20 ms)	36.90 (± 2.00 ms)	-4.83% 👎

tablecmp-create-10k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table-component/create/10k	duration	1597.30 (± 9.20 ms)	1618.40 (± 12.45 ms)	-1.32% 👎

tablecmp-create-1k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table-component/create/1k	duration	175.00 (± 3.10 ms)	175.20 (± 4.80 ms)	-0.11% 👌

tablecmp-update-10th-1k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table-component/update-10th/1k	duration	79.70 (± 3.50 ms)	79.60 (± 4.80 ms)	0.13% 👌

wc-append-1k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table-wc/append/1k	duration	263.70 (± 12.80 ms)	275.00 (± 8.50 ms)	-4.29% 👎

wc-clear-1k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table-wc/clear/1k	duration	37.40 (± 1.30 ms)	36.70 (± 1.20 ms)	1.87% 👌

wc-create-10k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table-wc/create/10k	duration	2066.50 (± 10.00 ms)	2137.50 (± 9.60 ms)	-3.44% 👎

wc-create-1k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table-wc/create/1k	duration	216.50 (± 4.80 ms)	223.40 (± 4.20 ms)	-3.19% 👎

wc-update-10th-1k	metric	base(`312bed0`)	target(`46367d2`)	trend
benchmark-table-wc/update-10th/1k	duration	74.80 (± 5.00 ms)	79.10 (± 3.90 ms)	-5.75% 👎

caridy reviewed Jul 3, 2018

View reviewed changes

caridy approved these changes Jul 3, 2018

View reviewed changes

pmdartus requested changes Jul 3, 2018

View reviewed changes

caridy approved these changes Jul 3, 2018

View reviewed changes

pmdartus reviewed Jul 3, 2018

View reviewed changes

pmdartus approved these changes Jul 3, 2018

View reviewed changes

davidturissini added 4 commits July 3, 2018 16:41

fix(engine): focusout composed flag

bacbe07

fix(engine): focusin event fix for firefox

734803a

fix(engine): moving focus event to polyfill

447af79

fix(engine): readme

c95dde7

davidturissini force-pushed the dturissini/ff-focus-out-composed branch from 3304eed to c95dde7 Compare July 3, 2018 23:44

fix(engine): linting

46367d2

davidturissini merged commit 3c0a6cb into master Jul 5, 2018

davidturissini deleted the dturissini/ff-focus-out-composed branch July 5, 2018 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(engine): forcing composed: true for focusout events #465

fix(engine): forcing composed: true for focusout events #465

davidturissini commented Jul 3, 2018

caridy commented Jul 3, 2018

caridy Jul 3, 2018

davidturissini commented Jul 3, 2018

caridy Jul 3, 2018

pmdartus Jul 3, 2018

davidturissini Jul 3, 2018

pmdartus Jul 3, 2018

caridy Jul 3, 2018

pmdartus Jul 3, 2018

davidturissini Jul 3, 2018

pmdartus Jul 3, 2018

diervo Jul 3, 2018

pmdartus Jul 3, 2018

davidturissini Jul 3, 2018

pmdartus Jul 3, 2018

davidturissini Jul 3, 2018

pmdartus Jul 3, 2018

pmdartus Jul 3, 2018

pmdartus Jul 3, 2018

davidturissini Jul 3, 2018

pmdartus Jul 3, 2018

caridy Jul 3, 2018

salesforce-best-lwc-internal bot commented Jul 5, 2018

fix(engine): forcing composed: true for focusout events #465

fix(engine): forcing composed: true for focusout events #465

Conversation

davidturissini commented Jul 3, 2018

Details

Does this PR introduce a breaking change?

caridy commented Jul 3, 2018

Choose a reason for hiding this comment

davidturissini commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salesforce-best-lwc-internal bot commented Jul 5, 2018

Benchmark results

lwc-engine-benchmark