Codemirror: platform-custom code in editor

[camillobruni]

There are quite a few places where we have very android and iOS specific code that is not executed elsewhere.
I don't think that's ideal, I'd still propose to integrate my navigator sanitizer (see #336) to get rid of this entirely.

[rniwa]

Since the sanitizer script doesn't seem to be happening in near future, can we put up a PR to remove this style manually?

[julienw]

I'm not worried about platform-specific checks but more about browser-specific checks.
Examples:
https://github.com/search?q=org%3Acodemirror%20browser.chrome&type=code
https://github.com/search?q=org%3Acodemirror%20browser.ie&type=code
https://github.com/search?q=org%3Acodemirror%20browser.gecko&type=code
https://github.com/search?q=org%3Acodemirror%20browser.webkit&type=code

We need to check whether these occurrences are in the path of the tests.

[bgrins]

It's worth looking into the linked examples above to see which of them is getting executed and has a meaningful performance impact, but based on personal experience with contentEditable it might be impossible to remove browser-specific checks without breaking the editor itself (or setting up a future breakage if/when we expand the test to hit a currently-unused code path).

I agree that in general we don't want browser-specific code in the benchmark, but in this case it may be inevitable given the nature of editors (part of the reason these libraries get popular is because they smooth over cross browser behavior differences).

[camillobruni]

I did measure 100 local runs with setting defaults on the browser object and there are some minor changes to the score. Now I can't fully tell at this point whether the functionality is equivalent or if we just run different code.

Safari TP (Release 186 (Safari 17.4, WebKit 19618.1.10.1.1)):

Safari (Version 17.3 (19617.2.4.11.8)):

Firefox Nightly (124.0a1 (2024-01-23) (64-Bit)):

Firefox (121.0.1 (64-bit)):

Chrome Canary (123.0.6262.0):

Chrome Stable (120.0.6099.234):

[bgrins]

There was some back and forth discussion today highlighting that while in general we don't want this kind of workaround, the nature of contentEditable can make them necessary and removing them may actually make cross-browser behavior more inconsistent (and less representative of code in the wild). So the plan is to do some more investigation (especially with the Safari conditions, cc @rniwa), to inform a decision about whether we want to change any code or simply document why this is an exception.

[rniwa]

Is there a PR for this that I can test out?

[camillobruni]

I've duplicated the editor code in my branch: https://github.com/camillobruni/Speedometer/tree/2024-01-24_custom_editor

And it contains the patched browser object: https://github.com/camillobruni/Speedometer/blob/2024-01-24_custom_editor/resources/editors_sanitized/dist/assets/codemirror-ba10726a.js

[rniwa]

I've gotten 5 samples of run for each browser's. The results look largely unchanged although maybe ~0.5% or so better in "after" measurements across the board. Given this, I'm fine with going ahead with this change. Is there an open PR for it?

Chrome 122.0.6261.57
== Before ==
28.9 ± 0.81
29.0 ± 0.62
29.9 ± 0.83
30.0 ± 0.51
29.3 ± 0.65

== After ==

29.7 ± 0.81
30.5 ± 0.98
30.3 ± 1.0
29.9 ± 0.90
30.1 ± 0.91

Firefox 123.0
== Before ==
28.0 ± 0.81
27.9 ± 0.60
28.0 ± 0.70
28.0 ± 0.73
27.8 ± 0.62

== After ==
28.4 ± 0.84
28.8 ± 0.71
28.9 ± 0.79
28.5 ± 0.76
29.1 ± 0.68

Safari Technology Preview 188
== Before ==
29.6 ± 1.1
30.2 ± 1.1
29.6 ± 1.3
30.1 ± 1.1
30.4 ± 1.2

== After ==
31.3 ± 1.4
30.7 ± 1.1
30.8 ± 1.2
30.3 ± 0.96
30.8 ± 1.2

speedometer-editor-change.zip

[bgrins]

Given this, I'm fine with going ahead with this change. Is there an open PR for it?

As I mentioned in #347 (comment), I prefer the behavior as-is, since contentEditable creates the need for browser-specific quirks (part of the reason editor libraries are popular are so these get abstracted away). So we'd really have to validate not just any performance difference but also investigate whether removing those changes the actual functionality. Given that there doesn't seem to be a strong performance difference one way or the other, I'd like to just close this issue (perhaps with a note in the README indicating that we avoid browser-specific behavior in general, but in this particular test we are allowing it).

[rniwa]

Given this, I'm fine with going ahead with this change. Is there an open PR for it?

As I mentioned in #347 (comment), I prefer the behavior as-is, since contentEditable creates the need for browser-specific quirks (part of the reason editor libraries are popular are so these get abstracted away). So we'd really have to validate not just any performance difference but also investigate whether removing those changes the actual functionality. Given that there doesn't seem to be a strong performance difference one way or the other, I'd like to just close this issue (perhaps with a note in the README indicating that we avoid browser-specific behavior in general, but in this particular test we are allowing it).

I'm fine with that outcome as well. Sounds like maybe we're leaning more towards closing this then?

[rniwa]

Per today's meeting, closing this issue in favor of keeping the current code.