[iOS] Redo history lost after adding link to text

[etoledom]

Describe the bug
Redo is lost after redoing a link on rich-text.

To Reproduce
Steps to reproduce the behavior:

1. Start an empty post.
2. Add a paragraph block and write something.
3. Add formats (Bold, Italic, etc...).
4. Add a link to one of the words checking Open in new tab option.
5. Add more changes (formats or new words).
6. Undo changes until the link is undone.
7. Try to Redo all the changes again.
8. 💥 The Redo history will be lost.

Expected behavior
Redo should work redoing all changes that were undone previously.

Video

https://cloudup.com/cOPGvdNJnli

Smartphone (please complete the following information):

• Device: iPhone XS
• OS: iOS only

Additional context
Add any other context about the problem here.

[guarani]

This is also present on Android, although the behavior is slightly different I think it's the same issue. Also, I noticed that Step 5 from the steps to reproduce can be omitted while still producing the bug.

Debugging

The bug stems from an extra undo entry being added to undo history in the Redux store right after adding links that set the "Open in new tab option". As an example, suppose a link is added to the letter "two" in the string "One two three". Here's the sequence of events:

1. User types "Zxc", selects the "x" portion of the text, taps toolbar link button, bottom sheet is displayed, user fills in link URL value (e.g. "f", doesn't need to be a valid link), switches on "Open in new tab option" and dismisses the bottom sheet. Behind the scenes, an undo entry is correctly added to the Redux store containing the following:

   "content": "Z<a href=\"f\" target=\"_blank\" rel=\"noreferrer noopener\">x</a>c",
   

2. When the user subsequently taps anywhere on the line to continue entering text, an extra undo object is added to the Redux store. This object contains the following:

   "content": "Z<a rel=\"noreferrer noopener\" href=\"f\" target=\"_blank\">x</a>c",
   

   Notice the rel attribute has moved from last place to first place in the tag.

Here's a diff of the entry that's added correctly to the undo history (on the left) and the entry (on the right) that's then added incorrectly to the undo history when the user sets the cursor after adding the link. You'll see the only significant difference is the position of the rel attribute inside the <a> tag.

What makes me quite sure this is what breaks the redo button functionality as described in this issue, is that if I use the debugger to prevent this erroneous entry from being added, this error disappears. To prevent the entry from being added, I put a breakpoint inside the undo reducer and popped the extra entry off the nextState array as soon as it's added.

There is a piece of code that's calling the undo reducer with an action.edits array containing the shifted rel attribute. I'm having trouble locating this code because the undo reducer is used in a couple of places and the action that's triggering this (tapping to set cursor position), is not very explicit.

[hypest]

This is also present on Android, although the behavior is slightly different I think it's the same issue

Hey @guarani, what's the different in Android's case? And, is it possibly related to #2495?

[hypest]

This ticket is not worked on at the moment so, I'll clear the assignee field to denote so.

[fluiddot]

I reproduced this issue in the latest version of the editor 1.88.0 so it's still present. The undo/redo functionality is part of the writing flow so I think this should be a high priority, hence I'm going to set the High priority label.

[derekblank]

Some initial findings:

@guarani's comment has some very accurate information. I paired with @fluiddot on this for a bit, and we both confirmed that the rel attribute shifts places in the link HTML after text has been typed, as noted in the comment and screenshot. There appears to be a state event that is invoked when the rel attribute is shifted, which causes the edit reducer to disrupt the redo history. There are several related issues that describe the broken behavior with undo/redo when using the "Open in new tab" option, so this issue may have a broader scope than is currently known.

Interestingly, when debugging the same behavior on the web, I noted that the rel attribute also shifts within the link HTML after typing, so this behavior is not specific to mobile. When the option to Open in New Tab is selected, the target="_blank" and rel="noreferrer noopener" attributes are added at the end of the link's HTML. Then, when typing text after the link, the rel="noreferrer noopener" jumps to the beginning of the link's HTML, while target="_blank" remains at the end:

Link.Attributes.Test.mov

My next steps are:

1. Determine if there is a purpose for the rel attribute to shift places like in the example.
2. If there is no purpose, determine the cause of where the rel attribute shifts in the code (perhaps it is in the shared createLinkFormat util?)
3. If the createLinkFormat util is the cause of the rel shift and the rel shift has no technical purpose, modify the createLinkFormat code so the rel does not shift.

The scope of the issue seems quite broad. For example, the undo/redo issues have been known within the manual test cases caveats for three years. It may be worth organizing all of the broader undo/redo issues together under a single issue if they are related, which I am happy to start. Open to feedback on how we might better organize this as a broader issue. 🙇

For reference, here are some of the other undo/redo issues that seem related (some are marked as complete as they are duplicates of each other):

• Undo does not work with links that open in a new tab #2495
• [iOS] Redo history lost after adding link to text #2091
• Links: Undo doesn’t work for links that open in a new tab. #1160
• Links: “Open in a new tab” setting doesn’t look like it saves. #1161
• Undo/redo - Apply text format to a link breaks the undo functionality #3136
• Quote Block: Undo and Redo consistently don’t work on first line #1158
• Undo/redo: Undoing a "enter for new block" action seems to add several actions to the Undo history #307
• Undo/redo doesn't seem to work when typing some text #360

[derekblank]

Also, a follow-up on this question:

What's the different in Android's case? And, is it possibly related to #2495?

It does seem very much related to #2495. However, the difference in my testing on Android is that the redo history is preserved (when it is not on iOS), but on Android it is much more difficult to perform an undo action on a link that has been configured to Open in a New Tab. This aligns with the feedback already captured in #2495. So, while they seem very related, at the moment I do not think they are duplicate issues.

[fluiddot]

Interestingly, when debugging the same behavior on the web, I noted that the rel attribute also shifts within the link HTML after typing, so this behavior is not specific to mobile. When the option to Open in New Tab is selected, the target="_blank" and rel="noreferrer noopener" attributes are added at the end of the link's HTML. Then, when typing text after the link, the rel="noreferrer noopener" jumps to the beginning of the link's HTML, while target="_blank" remains at the end:

It's interesting that this behavior can also be reproduced in the web version. I tried to reproduce this issue and undo/redo actions work as expected, adding a link to one of the words and checking Open in new tab option doesn't break the undo/redo history. I'm wondering what's the difference in this logic between platforms that only affects the native version. 🤔

[derekblank]

It's interesting that this behavior can also be reproduced in the web version. [...] I'm wondering what's the difference in this logic between platforms that only affects the native version. 🤔

I agree. It's also interesting that there are slight differences between iOS and the similar behavior on Android reported in #2495. Given that the web implementation works as expected, this seems to point to Aztec's handling of HTML attributes as the source of these differences between web, iOS, and Android.

[geriux]

Hey @derekblank 👋 It looks like this issue was solved in WordPress/gutenberg#50460 if so, can we close this issue? Thanks!

[derekblank]

@geriux Yep, closing. 👍