Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List Block fixes for iOS. #863

Closed
wants to merge 9 commits into from
Closed

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Apr 12, 2019

Fixes #886

This PR implements changes on Aztec-iOS that fix an issue on the block list.
There is a related gutenberg side PR needed to fix an issue pressing enter on List blocks.
Issue:
ios-list-end-issue

After the fix:
list_block

To test:

  • Create a list block with the following content:
<!-- wp:list -->
<ul><li>line 1<ul><li>line 21</li><li>line 22<ul><li>line 31</li><li>line 32</li></ul></li></ul></li><li>line 3</li><li>line 4</li><li>line 5</li></ul>
<!-- /wp:list -->
  • Add a new list item.
  • Check that the new list item is properly placed.

@hypest
Copy link
Contributor

hypest commented Apr 15, 2019

👋 , when running yarn ios I get this error:

$ cd ./ios && carthage bootstrap --platform iOS --cache-builds
*** Checking out AztecEditor-iOS at "f11e3d7176927b931d59931da0e98ff373f73a5d"
*** xcodebuild output can be found in /var/folders/py/vdkc920j69b255sld66fbxt40000gp/T/carthage-xcodebuild.9bUzuR.log
*** Invalid cache found for AztecEditor-iOS, rebuilding with all downstream dependencies
*** Building scheme "Gridicons" in Gridicons.xcworkspace
Could not find any available simulators for iOS
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Any tips for how to get past that?

@diegoreymendez
Copy link
Contributor

@hypest - Nothing specific, but one starting point would be to make sure you update Carthage - I remember @koke mentioning that fixed the issue on his side.

@koke
Copy link
Member

koke commented Apr 16, 2019

Not sure if it upgraded but it was a reinstall (brew uninstall/install carthage)

@hypest
Copy link
Contributor

hypest commented Apr 16, 2019

I should have added that the build works OK for me on develop, not sure if it adds any important information. So, it's only on this PR that the build error appears.

@hypest
Copy link
Contributor

hypest commented Apr 16, 2019

Update: I can confirm that re-installing (brew uninstall carthage followed by `brew install carthage) fixed the build issue for me. In the process, Carthage went from v0.31 to v0.33.

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the fix and can verify that it is indeed fixing the issue! 🎉

Haven't done any code level passes on the actual fix though and I will that to @diegoreymendez and other iOS experts.

By the way, there's an issue with creating new list items when there are nested lists that happens under some conditions but I think it's not introduced by this PR. It appears both on Android and iOS alike. Here's the ticket: #874

@etoledom
Copy link
Contributor Author

The Aztec-iOS side PR was merged.
@hypest this is ready for a final review.
Thank you!

@etoledom etoledom requested a review from hypest April 17, 2019 20:09
Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etoledom - I managed to crash this PR (once) following these steps:

  1. Create the specified block.
  2. Switch back to visual.
  3. Place caret at the end of "line 32".
  4. Press enter once.
  5. Type something, and press SHIFT + TAB to de-indent the current (new) bullet.
  6. Press ENTER

I can no longer crash it but there is undoubtedly some strange behavior.

listProblems

@diegoreymendez
Copy link
Contributor

I'm not sure what the cause is but I'd imagine this type of block may require changes in how we intercept ENTER events. I added the above review as a comment as you may decide this is something to tackle incrementally.

@etoledom
Copy link
Contributor Author

etoledom commented Apr 17, 2019

Thank you @diegoreymendez !

I saw a crash on enter on another multi paragraph instance of RichText here (first iOS issue described): WordPress/gutenberg#14883 (review)

I don't know if they are related but it might be.
If it's not something new from this PR, it's probably better to merge and continue on a new PR.

I'll check that out!

@etoledom
Copy link
Contributor Author

@diegoreymendez @hypest - Indentation and outdentation via Tab / Shift + Tab is definitely broken on Aztec-iOS.

Example indent:

From this initial HTML:

<ul>
    <li>Hello</li>
    <li>world</li>
</ul>

Pressing Tab in the last list item we should get this:

<ul>
    <li>Hello
        <ul>
            <li>world</li>
        </ul>
    </li>
</ul>

But we get this:

<ul>
    <li>Hello</li>
    <li>
        <ul>
            <li>world</li>
        </ul>
    </li>
</ul>

Example undent:

Starting from this HTML:

<ul>
    <li>Hello
        <ul>
            <li>world</li>
        </ul>
    </li>
</ul>

Pressing Shift + Tab in the indented list item we should get the original:

<ul>
    <li>Hello</li>
    <li>world</li>
</ul>

But we get:

<ul>
  <li>Hello </li>
world
</ul>

From there is clear that any inconsistency passed to Aztec could cause an out of bounds exception. Even though I haven't been able to crash it yet. But we know that it can happen from @diegoreymendez 's experience.

I think that the correct solution for gutenberg would be to intercept this key bindings and use the Gutenberg's indentListItems and outdentListItems functionality.

Maybe we could just deactivate the bindings on Aztec Wrapper and work on indentation in a second iteration?

I'll still try to work on a solution on Aztec-iOS level. I have created a PR with breaking Unit Tests here:
wordpress-mobile/AztecEditor-iOS#1174

@hypest
Copy link
Contributor

hypest commented Apr 18, 2019

Maybe we could just deactivate the bindings on Aztec Wrapper and work on indentation in a second iteration?

Sounds good to me. We've already removed the toolbar buttons for nesting the list since it's not well supported yet so, makes sense to also disable the keyboard path to them.

@hypest
Copy link
Contributor

hypest commented Apr 18, 2019

I think that the correct solution for gutenberg would be to intercept this key bindings and use the Gutenberg's indentListItems and outdentListItems functionality.

Agreed, getting closer to handling this as the web via the format-lib is what I'd personally prefer too.

@etoledom
Copy link
Contributor Author

Issue: #888
PR: #889

@diegoreymendez
Copy link
Contributor

diegoreymendez commented Apr 18, 2019

Just to clarify, my comments above were just to give visibility to the problem.

I'm 100% fine with any direction chosen by you and @hypest to move this forward (ie: it's not my intention to comment on that).

@etoledom
Copy link
Contributor Author

Thank you @diegoreymendez ! It was great to rise the issue 😁

@hypest if we are fine with this fix. We just need the ✅ to merge.

@etoledom
Copy link
Contributor Author

etoledom commented Apr 19, 2019

The Aztec reference has been already updated to a later point via this PR: #889

So merging the current PR is not longer needed.

@etoledom etoledom closed this Apr 19, 2019
@etoledom etoledom deleted the issue/list-block-fixes-for-ios branch April 25, 2019 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List block split: item and caret at wrong lines when nested lists
4 participants