LPS-117287 Multiple clicks on links mess up SPA navigation #233
Conversation
|
To conserve resources, the PR Tester does not automatically run for every pull. If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed. If your pull was never tested, comment "ci:test" to run the PR Tester for this pull. |
|
hey @markocikos is that only a problem for Journal or could this be happening with other links too? I remember that Senna is supposed to gracefully deal with multiple clicks and cancel the pending navigation and start a new one. If that's affecting other places we might need to fix that in Senna itself. |
|
@brunobasto It is happening with many other links. Senna sound like the right place for the change, I can give it a shot. Is this the correct repo to look in? I never worked on it, so I might need some pointers with integration, PRs, publishing etc. |
That's the one. |
|
One thought before diving too heavily into Senna... (and perhaps a silly question) can we repro this behavior with SPA turned off? |
I heavily looked into this, and I was unable to reproduce it with senna off, for the moment this is the workaround we offer to our clients who had this issues and all confirm this issues stopped once SPA was disabled. |
markocikos
force-pushed
the
LPS-117287-2
branch
from
36268fc
to
7f869b2
Compare
|
@brunobasto @wincent Well, I force-pushed a solution. My confidence is very low that it's the best one 😄 Looking at Senna, I found that, indeed, there is a mechanism that prevents multiple clicks breakage, with pending navigation check. It is a part of This solution is a bit nuclear, we are forcing delegate(document, 'click', 'a', (event) => {
const url = event.delegateTarget.href;
if (app.canNavigate(url)) {
app.navigate(url);
}
});This fixes the double-click bug portal-wide, but is also quite likely to break something. I'll run tests, am curious to see. I would expect that enforcement like this exists somewhere, or we have some alternative mechanism that achieves auto-navigation. I couldn't find it, but intend to dig more. If you know it, please point me to it. |
|
ci:test:relevant |
Hey Marko, I think this can introduce some regressions. Senna has its way of not targeting certain links (or even form submissions) when they have a data attribute I think we might need to dig a little deeper on this one. Looking at git history for both |
|
This might have something to do with it: https://github.com/liferay/senna.js/blob/master/src/app/App.js#L434 Maybe @diegonvs can share some insight (if he remembers this). The issue that introduced this cancellation policy was liferay/senna.js#262 |
I have trouble remembering what I did yesterday... 👴 |
|
|
I don't remember easily which things were needed to change for having the pendingNavigations things working 😭 .
It needs to be investigated deeply ;/ |
|
Watching is being thrown before the |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#6351 |
|
Followup in liferay/senna.js#337 |
This is a bugfix, see description and discussion in https://issues.liferay.com/browse/LPS-117287. This ticket exposes several unrelated bugs, one of which we previously fixed in #171.
To reproduce, double(+)click on an edit link in journal. I added few console logs for gifs, as it is not clear when I click.
Before:
After:
Notes:
<aui:a>links. What would be the ideal scope for this kind of change?