My intention is to order them by impact of the fix on the rest of the system. Things that can be fixed in isolation are the lowest hanging fruit. So roughly in order of how I arranged the checks in the spreadsheet:
Pure Fix (Optimisation or Tail Recursion or Docs)
Change to API Behaviour
API Addition (Backwards Compatible)
API Change (Breaking)
Change to Language Semantics
But yes, you could play around with the order based on a number of other factors too.
Feel like I should add 2 more check box columns to the sheet.
For claimed performance improvements, as these should really be backed up with some benchmarks, in some cases its not really clear if a PR for a claimed perf improvement is really going to be a perf improvement.
For PRs that have raised some controversy in the discussion. Sometimes Evan might have commented to explain why he does not think something is a good idea, for example. Just to help flag up things that might seem straightforward but really need to be given some careful consideration.
The fix seems harmless to me, in that it only allows new behaviour to be possible without breaking the old. In that sense, it does seem like the sort of thing we can and should fix.
I marked it as controversial simply because Evan weighed in with the opinion that its not how he envisages CSS being done in Elm (you would write a function in Elm to generate the CSS you want). When it comes to patching stuff in the core independantly of Evan, these are the kinds of issues that need careful consideration so I am glad you brought it up.
Do you have an opinion on how the fix might affect performance? Fix is to change:
domNodeStyle[key] = styles[key];
I know that mdgriffith/elm-ui has put a lot of thought into making its CSS very fast, and I am wary of upsetting that with this change. There is a UI benchmark somewhere that compares elm-ui, elm-css and a few other ways of styling isn’t there? Would be worth checking against that.
I’m not enough of a CSS expert to know if .setProperty is going to be slower.
It may well be that exact implementation isn’t the best solution, if it makes more sense to have a special setCustomProperty that can only be used for that job to separate the implementation or some other alternate solution, that’s reasonable.
My focus is that the feature needs to be surfaced otherwise Elm is going to end up making any modern webcomponent unusable, and blocking access to other modern features (e.g: Houdini), I’m not advocating for any specific solution to surfacing it.
The spreadsheet has now been completed to cover all open Pull Requests on all core packages and the compiler.
Still happy to add any more issues that anyone wants to bring to my attention?
Other than that, that is as much as I am going to add to it for now. Next step will be to order things by level of impact to reveal what can be fixed without breaking anything versus changes that have deeper impacts that are not backwards compatible, or alter the language semantics. Then start a new thread with a summary of the spreadsheet around that topic.
Added it. I think the issue was already covered under the item “Browser extensions messing with the VDOM”, but no harm in having variations on it.
I marked this one as “API Change (Breaking)” as maybe we might need to change the Browser.application function type, so it accepts some further parameters that tell it where to put the Elm DOM. That isn’t the only way this could be fixed though, there are more backwards compatible ways it could be tackled.
I see. I didn’t actually see the patch since I didn’t go so far as digging into the jinjor/elm-break-dom project to look for it. I wonder why this patch was never submitted as a PR to elm/virtual-dom? Perhaps the author thought there was no point as it would not be merged?
Only comment I could find from Evan on the issue is:
In the past, people (I believe including @rtfeldman) expressed frustration with having that additional named div in there, so I took it out. So I’m surprised by this recommendation. I’m not sure what to do given the history of this.
(the extra div approach screws up printer layouts apparently)
I will change the status of this one to “Runtime Exception” - seems like the patch could be a straightforward bug fix and a really helpful one too.
Found some more from Evan:
Possibly that post got jinjor started on compiling a list of problems and coming up with a fix. We just never completed the process and got the fix out…
(It’s been a while since I posted that issue and I haven’t re-checked since then, so there’s a chance Grammarly doesn’t do that anymore. But I’m sure there are other browser extensions that can insert a piece of UI deeper into the DOM.)
Also, Google Translate replaces text nodes with <font> tags (I believe to keep track of which pieces of text have been translated).