mavo
mavo copied to clipboard
mavoweb
Clicking on editable property inside <a> triggers the link
This is a regression, pretty sure it worked before.
Testcase: https://codepen.io/leaverou/pen/GRvKQPo?editors=1100
@DmitrySharabin do you have time to bisect?
It looks like this commit is the first bad one: 8300a210a3ab9aba9e4c346ffe486956f6722e79.
It turned out the last stable version where this feature worked well was 0.1.6. 🤪
@DmitrySharabin Thanks for bisecting! Are you sure that's the right commit? It seems super weird that this introduced that bug. 🤔
That was exactly my thoughts. That’s why I re-checked it 3 times. And every attempt gave the same result. I used v0.1.6 as a good commit and the current HEAD position as the bad one. I tried to bisect between v0.1.6 and v0.2.0 — the same result. :(
I guess that must be the one then. Another case of the Code Butterfly Effect! Thanks again for looking into it @DmitrySharabin.
I gave another try to bisect the issue (since Lea discovered git bisect skip yesterday and it helped us with another issue)... but got the same result. 😔
It looks like the Code Butterfly Effect is inevitable sometimes.
I gave this issue a closer look, and here are some of my observations that might help.
To begin with, in line 479 we prevent the default behavior only when this.editor doesn't contain the event target. However, don't we need the exact opposite?
https://github.com/mavoweb/mavo/blob/1371988d077bd651184b3cb87a88403e97e3383d/src/primitive.js#L479
I removed the not operator and got the desired behavior. But, surprisingly, it works only when we switch from read mode to edit mode. If our app supports only edit mode, it doesn't work.
It happens because wasEditing becomes equal to true in that case, however, we add a listener to the click.mavo:edit event only if wasEditing is false (I don't know why, unfortunately).
https://github.com/mavoweb/mavo/blob/1371988d077bd651184b3cb87a88403e97e3383d/src/primitive.js#L469-L484
So I tried to move lines 475 through 483 outside the if (!wasEditing) { ... } block, and it fixed all issues. I also checked the tests — they still pass.
https://github.com/mavoweb/mavo/blob/1371988d077bd651184b3cb87a88403e97e3383d/src/primitive.js#L475-L483
@LeaVerou I'm ready to send a PR, but I'm not sure whether I'm right or not. Also, you might be working on something, and my PR might mess something up.
Hey, thanks for looking into it! Please don't hesitate to send a PR, it's far easier to understand code changes that way. A PR in itself cannot mess anything up, it's merging the PR that might ;)
Please ignore the PR for now. I’ve not completely thought through the details. I’ll ping you when I’m ready to present my results. :)
Please ignore the PR for now. I’ve not completely thought through the details. I’ll ping you when I’m ready to present my results. :)
Btw you can mark PRs as draft if you don't want them to be merged until you do some more work on them 😉
Actually, even this doesn't look like it's fixed, see testcase: https://codepen.io/leaverou/pen/JjrNQyo?editors=1100 So I'm going to reopen this. I wonder if we should revert the changes, since they didn't actually fix the bug, and any changes can cause regressions. What do you think @DmitrySharabin?
Yes, we definitely should reopen it. However, this bug isn't fixed because we've already reverted the changes here: https://github.com/mavoweb/mavo/commit/4254d1afa87610fe1fad1ab1e7383651290775e2 (because they introduced some regressions).
So we are now at the point we were when this issue was opened.
This is fixed in 2971d9d6739ee73f0be542163553b50fc66e3550. 🥳
We should probably close the issue.