Fix logic for retrieving lastKnownElement

[jdpnielsen]

Prerequisites

• I have added steps to test this contribution in the description below

This fixes #16057

Description

This resolves the linked issue. I believe the issue was introduced by the addition of an extra if-statement in this commit: 9730a18#diff-75783fbe92a64ae9a209d96c0ce71e8ddb7b04214f9f4f7ba1f73b78283f9a3aR98 from this PR: #10274

I'm assuming the linked pull request was attempting to prevent calling .querySelector() on an undefined element. I have solved this issue by using a ternary instead of a nested if-statement, which was preventing the fallback to $rootScope.lastKnownFocusableElements from kicking in.

[github-actions]

Hi there @jdpnielsen, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[mikecp]

Hello @jdpnielsen ,

Thank you for spotting this issue and providing the fix for it 👍
And congratulations on your first contribution to the Umbraco CMS 🎉
A member of the core collaborators team will have a look at is soon.

Cheers!

[jdpnielsen]

Thanks @mikecp!

Is there a good way to patch an existing Umbraco solution with this fix?

[nul800sebastiaan]

Thanks @jdpnielsen - the existing code was already hard to read but your fix made it even harder to decode 😅 Usually, I wouldn't recommend refactoring things until asked / recommended, it makes it very hard for the team reviewing to see what has actually changed.

Please confirm, the one and only change I can see here is actually:

Both infiniteEditors.length === 1 are now infiniteEditors[0].length === 1. In plain language: don't check if there is a single infinite editor, instead make sure that there is at least 1 infinite editor. Am I reading that correctly?

In that case, the other fix could have been infinitEditors.length > 0, correct?

I do appreciate the readability of the new code by the way, don't get me wrong, I just need to make sure I understand the scope of the change.

[nul800sebastiaan]

As to your other question:

Is there a good way to patch an existing Umbraco solution with this fix?

There really is no good way, this is all built and bundled and each NuGet restore will overwrite any changes you make. The only way you could do it is to make a custom build of this and host the output NuGet packages on (for example) MyGet.

We're hoping to merge this fix soon and ship it in a release in the next few weeks (we don't have a solid release date yet).

[jdpnielsen]

Thanks @jdpnielsen - the existing code was already hard to read but your fix made it even harder to decode 😅 Usually, I wouldn't recommend refactoring things until asked / recommended, it makes it very hard for the team reviewing to see what has actually changed.

Agreed, this file is a giant mess! I was attempting to keep my changes as minimal as possible while restoring the intended functionality to how it was before the linked change.

Please confirm, the one and only change I can see here is actually:

Both infiniteEditors.length === 1 are now infiniteEditors[0].length === 1. In plain language: don't check if there is a single infinite editor, instead make sure that there is at least 1 infinite editor. Am I reading that correctly?

In that case, the other fix could have been infinitEditors.length > 0, correct?

That was a mistake - i have now changed it to check infiniteEditors.length === 1 as before.

[nul800sebastiaan]

That was a mistake - i have now changed it to check infiniteEditors.length === 1 as before.

Thanks for the update @jdpnielsen! I was looking at the wrong thing, I see what you've done now by pulling the editorInfo out of that weird nested if, so that we actually do get to the else that sets lastKnownElement, nice!

Tests well too, thanks for the help here! We're going to get a 13.3.1 out there in the next few weeks and this fix will be included in that. 👍