[RNMobile] Fix undo/redo history when inserting a link configured to open in a new tab #50460
Conversation
derekblank
added
Mobile App - i.e. Android or iOS
|
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
derekblank
changed the title
|
Flaky tests detected in c4bb30003698367981d4bcc837f9a796f9d7f523. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5055066858
|
derekblank
changed the title
Co-authored-by: Tanner Stokes <tanner.stokes@automattic.com>
There was a problem hiding this comment.
Super interesting! Thank you for researching and addressing this.
The prototype iOS build does not pass the test plan. However, my guess is this occurs because the prototype build does not include the fixed code. The wordpress-mobile/gutenberg-mobile#5747 PR does not include a new bundle via npm run bundle. The Android prototype build succeeds, though. 👍🏻
Would you be willing to follow up with tests covering this bug? In general, I think it would be good to always include a test with each bug we fix. For this specific bug, adding a test to the Editor History file makes sense to me.
Co-authored-by: David Calhoun <github@davidcalhoun.me>
Thanks for pointing this out. There were a handful of unrelated CI issues on the Gutenberg Mobile PR, and I ended up manually updating the Gutenberg ref (possibly incorrectly), which may have caused it the branches to be out of sync. I'll update the iOS build. 👍
This is a good idea, and I'll start working on it to include before we merge this PR. |
You may already understand this, but your response gave me pause. The updated Gutenberg ref (git submodule commit SHA) is updated correctly. The missing piece is committing an updated app bundle produced via |
No, I was actually not aware of that, but that makes much more sense now. I was aware of the I was looking for documentation on this, and noted that it is mentioned in the WPiOS Integration doc. I had only read about it in the release context over in the release-toolkit-gutenberg-mobile docs. I test PRs in the host apps using the local gutenberg-mobile checkout, so it's possible that I once read the latter half of the WPiOS Integration doc and put it out of my mind. Do you think it would be beneficial to link or mention the host app Integration documentation from the Gutenberg Mobile README? |
Yeah, sounds like a good addition to the existing "Run" section. I think it makes sense for the README to default to the Demo editor, but a note could be added linking to documentation on how to integrate into the host WordPress iOS and Android apps. |
…open in a new tab
…ordPress/gutenberg into rnmobile/fix/undo-redo-history
The targeted elements do not require asynchronous queries. Using synchronous queries is more concise and easier to comprehend.
The targeted bug occurs when: 1. Changing a link to "Open in a new tab." 2. Performing additional changes, e.g. typing. 3. Undoing both actions. 4. Redoing both actions. Thus, this changes the initial HTML, adds additional undo/redo invocations, and updates inline snapshots to match the expected outcomes.
|
@dcalhoun I've added a WIP for a Editor History E2E test. I'm curious of your thoughts on this approach before proceeding further. Although just a scaffold at the moment, the test structure is similar to the integration test in its arrangements and actions but with more realistic editor behavior, and could potentially replace the integration test in purpose. Specifically:
|
There was a problem hiding this comment.
The Gutenberg Mobile E2E tests felt more snapshot-focused, while the react-native-editor E2E tests felt more assertion focused. I'm trying to understand if there is a more nuanced difference than that between the two test locations.
The tests in the former location are either Automattic-centric (e.g. Jetpack) or experimental (e.g. visual snapshots). Additionally, you may note that not every test in latter location are run within the gutenberg CI; this was done due to how slow and flaky the suite is. Ideally, we would run all relevant tests within the gutenberg CI. Efforts to replace e2e tests with integration tests relate to this subject.
Also, not every e2e test is automatically run for every PR within gutenberg-mobile. You will note the "canary" name in certain e2e test files. These are run within the gutenberg-mobile CI for every PR. All other e2e tests are run when a person manually initiates the "Optional UI Tests" CI task in CircleCI.
We are careful to add e2e tests that run by default in gutenberg-mobile and even more so within gutenberg. Placing this in gutenberg makes sense to me, but we should likely not run it in the CI.
Would it make more sense to include as a test case in an existing E2E file, like gutenberg-editor-paragraph.test.js? Since it is intended to test editor history, I think it makes sense as its own file.
I think this is largely a judgement call. It makes sense from a subject matter perspective that a single file focuses upon the feature of editor history. However, there is a test environment setup cost we pay for each test file. I.e. because e2e tests are so slow, it is beneficial to group several together rather than creating a new environment for each individual test.
If you have a moment to review, I'd be interested in any tips or caveats on these scaffolded areas -- particularly if you are able to point to any helpers that may already exist that would accomplish the described action
I left inline comments with any suggestions I have. However, keep in mind there are a lot of helpers and I am not intimately familiar with them all. I may have missed opportunities. My technique is to "fold" all code in the editor-page and utils files to get a high-level view of the available helpers.
Folded test helper code
Additionally, my recommendation would be to postpone creating new test helpers until we identify common patterns across several tests or a need to simplify a very complex or brittle interaction. As discussed, there are a lot of helpers; frequently adding more will continue to increase the incomprehensibility of them, reducing the usefulness of the helpers.
Lastly — and I should have shared this in my original review — if we believe completing new test coverage will take a good amount of time, it may be worth merging this PR without tests and opening a separate PR adding tests. That might allow us to deliver a bug fix to users sooner.
packages/react-native-editor/__device-tests__/pages/editor-page.js
Outdated
Show resolved
Hide resolved
packages/react-native-editor/__device-tests__/gutenberg-editor-history.test.js
Outdated
Show resolved
Hide resolved
packages/react-native-editor/__device-tests__/gutenberg-editor-history.test.js
Outdated
Show resolved
Hide resolved
derekblank
force-pushed
the
rnmobile/fix/undo-redo-history
branch
from
c4bb300
to
d0098e6
Compare
|
@dcalhoun Thank you for reviewing, and for your explanations regarding the differences between the Gutenberg and Gutenberg Mobile CI tests, as well as the explanations for CI considerations. That all makes sense. 👍
I agree here too. After also discussing with @twstokes, I think we should pause on the E2E test due to complexities of writing additional helpers, and potentially include these at a later time. The integration test is actually correctly checking for the shifting |
What?
Fixes a bug affecting undo/redo functionality on mobile. When a link is configured to open in a new tab and text is typed after this link, it is not possible to undo and then use the Redo button to redo the text that was added after the link.
Original issue:
Related issues:
Gutenberg Mobile PR:
Why?
There are several related issues that describe the broken behavior with undo/redo when using the "Open in new tab" option on mobile. One symptom of this issue is that an extra state event occurs on mobile when a link is set to open in a new tab. This is demonstrated visually in wordpress-mobile/gutenberg-mobile#2091 (comment) with further information. Configuring a link to open in a new tab shifts the
relattribute within the HTML, which causes the edit reducer to disrupt the redo history.Example HTML when a link is created without opening in a new tab:
Example HTML when a link is created and configured to open in a new tab:
How?
This PR adds
relto the object of attributes passed to the link formatting, along withurlandtarget:When
relis passed with the attributes object to registerFormatType, therelattribute is no longer an unassigned attribute, and does not shift places when the HTML attributes are re-serialized. Therefore, the extra state event is not fired on mobile. On the web, the extra state event is not fired in either circumstance, and the re-serialization of the attributes has no effect on behavior.Testing Instructions
relattribute within the link's HTML. (It should be the final attribute.)Demo builds for testing:
Screenshots
Undo-Redo.mov