Editor: Post contains local changes even when the user doesn't modify the post

malinajirka commented 4 years ago

Expected behavior

When I open a post in the gutenberg editor and I close it I expect the post won't contain any local changes.

Actual behavior

When I open a post in the gutenebrg editor and I close it the post contains local changes.

Steps to reproduce the behavior

My Site
Blog Posts
Open a published post (eg. this long post) - it happens on some posts only after you click into the content. However, it happens on this post even when I just open it and close it)
Press back
Notice the post contains "Local changes" label

Tested on [device], Android [version], WPAndroid [version]

Emulator API 29

cc @mchowning you recently played with saving/exiting editor cc @planarvoid you also have context. Would you have time to look into it during groundskeeping?

hypest commented 4 years ago

Looks related to the ticket opened a few days ago in the gb-mobile repo: https://github.com/wordpress-mobile/gutenberg-mobile/issues/2714

malinajirka commented 4 years ago

Thanks for the info, I've missed that. They look similar, however, I haven't even need to scroll + this issue isn't about autosave but about save on exit. Having said all that there is a pretty high chance these two tickets are related.

planarvoid commented 4 years ago

Since you're also @mchowning doing groundskeeping this week, is it ok if I'll leave this to you?

mchowning commented 4 years ago

@mkevins has noted a similar-sounding issue here:

Another observation [...] is that simply opening and existing the post (without making any changes) and then closing it results in the autosave mechanism kicking in.

mchowning commented 4 years ago

I have spent some time investigating this, and it appears to be due to certain blocks having outdated or otherwise unexpected (but valid) formatting in the post content. I (obviously) still need to do some more investigation into this, and I do plan to follow-up on this.

mchowning commented 3 years ago

I suspect this may be related to https://github.com/wordpress-mobile/gutenberg-mobile/issues/894.

malinajirka commented 3 years ago

A note which might be useful - I noticed the EditorFragment.getTitleOrContentChanged() is invoked when you open a post. It's also interesting that it is invoked on the same post every time you open it (even if you open it after it already has the "local changes" label).

mchowning commented 3 years ago

Removed my assignment because I'm not going to have time to get back to this in the near future.

designsimply commented 3 years ago

Raising priority because there is potential data loss if Gutenberg Mobile doesn't return the most up to date content when the EditPostActivity asks for it (noted at https://github.com/wordpress-mobile/WordPress-Android/issues/13003#issuecomment-750106983).

Apologies, I added that last comment to the wrong issue. @malinajirka can you help me with prioritization on this one? Should it stay medium?

AmandaRiu commented 3 years ago

tested on a Pixel 4 running Android 11 and this is still an issue. When I loaded a really long post I had to actually click into a block to replicate the issue, but made no changes and yet when I return to the post list it shows I have local changes.

antonis commented 3 years ago

I managed to reproduce the reported behaviour in smaller post. Starting from a post with content from the first revision of this gist you can observe the content loop between two states (check the revisions).

When you open the post and Update from within the editor the local changes go away
When you open the post (that has no local changes) and just press back the local changes appear again

https://user-images.githubusercontent.com/304044/114757321-5eefdb00-9d64-11eb-84e8-6919428b41bc.mp4

develric commented 3 years ago

Looked into this issue a bit and just wanted to confirm that starting from the steps reported by Antonis here, I was able to get the same two states loop. Also in my case to trigger the loop, the on opening EditorFragment.getTitleOrContentChanged() call was enough. by the effect it seems like some piece of logic exists that manage in different ways things like the semicolon and blank spaces in between tags.

I was trying to have some more debug into the GB editor to get a better understanding but had some issue with my local setup to get Metro up without crashing the app (it then disappear so this is why I'm pretty sure it was an issue with my setup only 🤷 , but run out of time in the end).

dcalhoun commented 3 years ago

I investigated this issue some and wanted to capture what I noticed so far. I was able to reproduce the issue with a subset of the content shared in https://github.com/wordpress-mobile/WordPress-Android/issues/13169#issuecomment-819723808.

Reproducible Post HTML

```html


coding is fun


coding is fun

```

I observed similar changes to the block markup as referenced before, namely the jump between <br> and <br /> in the core/preformatted block. It would appear parsing the initialHtml provided to the editor results in different output than the parsed blocks that populate the core/editor store on editor initialization.

I have been unable to identify why parsing the initialHtml resulted in a different result, but it causes Gutenberg to inform the native host app that changes have occurred.

mkevins commented 3 years ago

Hi @dcalhoun :wave: :smile: Great job reproducing with a subset of content.

the jump between <br> and <br />

In your debugging screenshot, is the content going "back and forth" between those representations (i.e. would explain Antonis's observations about triggering local changes in a "loop")?

have been unable to identify why parsing the initialHtml resulted in a different result,

The differences in <br> and <br /> almost suggest an attempt to be valid XML, rather than merely HTML5. I wonder if there is a divergence of intention between the parsing and save logic with regard to which representation is valid :thinking:

mchowning commented 3 years ago

The differences in
and
almost suggest an attempt to be valid XML, rather than merely HTML5. I wonder if there is a divergence of intention between the parsing and save logic with regard to which representation is valid .

I think this is because of Aztec-Android. There are a number of changes like this that it makes to content (i.e., if you pass an html string into the fromHtml method and then immediately call toHtml, you will not always get back the exact same string because Aztec tries to clean up the input.

There has been some talk of trying to create a simplified version of Aztec for use with Gutenberg, and I think that's our best chance to address these issues. I think that without a separate version of Aztec for Gutenberg, it will be very difficult to fix these issues without causing regressions in the classic editor.

Changes I Observed When Web Blocks Were Viewed on WPAndroid

I did some testing of this last year, and I feel like I wrote this down publicly somewhere, but I can't find it now, so here are the changes I saw that would occur:

<figure class="wp-block-image"><img alt=""/></figure> from the web would get a space added, resulting in: <figure class="wp-block-image"><img alt="" /></figure>

<div style="height:100px" aria-hidden="true" class="wp-block-spacer"></div> from the web would get a ; added, resulting in <div style="height:100px;" aria-hidden="true" class="wp-block-spacer"></div>

<h4 class="has-text-align-left has-primary-background-color has-background" style="line-height:2.5">Heading with line-height set</h4> from the web would get a ; added, <h4 class="has-text-align-left has-primary-background-color has-background" style="line-height:2.5;">Heading with line-height set</h4>

 from the web gets gets a bunch of escape characters added: 

<div class="wp-block-cover has-background-dim-20 has-luminous-vivid-orange-background-color has-background-dim" style="background-image:url(https://cldup.com/cXyG__fTLN.jpg);min-height:219px;background-position:63% 83%"><div class="wp-block-cover__inner-container"> from the web would get two changes: single quotes added to the URL, and the … unicode character got changed to \u2026: <div class="wp-block-cover has-background-dim-20 has-luminous-vivid-orange-background-color has-background-dim" style="background-image:url('https://cldup.com/cXyG__fTLN.jpg');min-height:219px;background-position:63% 83%;"><div class="wp-block-cover__inner-container">

dcalhoun commented 3 years ago

the jump between <br> and <br />

In your debugging screenshot, is the content going "back and forth" between those representations (i.e. would explain Antonis's observations about triggering local changes in a "loop")?

@mkevins no, I did not observe multiple "back and forth" switches. I only observed the swap oddity with initialHtml when the editor first initialized.

I think this is because of Aztec-Android. There are a number of changes like this that it makes to content (i.e., if you pass an htmls string into the fromHtml and then immediately call toHtml, you will not always get back the exact same string because Aztec tries to clean up the input.

@mchowning great point. I am unable to reproduce the same issue in WPiOS, so it would make sense to be related to Aztec-Android.

There has been some talk of trying to create a simplified version of Aztec for use with Gutenberg, and I think that's our best chance to address these issues. I think that without a separate version of Aztec for Gutenberg, it will be very difficult to fix these issues without causing regressions in the classic editor.

You may be right that a simplified Aztec is the most straightforward approach. Avoiding regressions in the Classic editor would be difficult.

thabotswana commented 3 years ago

This was also reported in 4347487-zen

I Force Stopped the app, cleared Cache and Storage, then added my site back to the app. Next I checked a page and backed out of the page without making any changes. The app again shows Local Changes. The above is repeatable every time.

User has Google Pixel 2 XL, WPAndroid 18.4-rc-2

zwarm commented 2 years ago

I was able to verify that this is still an issue (during groundskeeping rotation). After login and site selection, I am navigated directly into the editor, then back-out without making any changes. I see the "local changes" in the post list, plus I see a dialog warning upon logout. See pbxNRc-1fJ-p2#comment-3049 for steps to recreate and dialog.

mkevins commented 2 years ago

I have been investigating this issue as part of groundskeeping, and have confirmed that in some cases, the issue only occurs after interacting with a particular block, and in other cases, merely opening existing content is enough to have it marked "dirty" or locally changed. I have also observed that there are some differences with the behavior depending on whether the post is in a draft state or a published state, but I have not yet found the root cause of those differences.

It seems this issue can actually have many underlying causes, so in order to better understand the data flow that may lead to this, I examined the control flow graph from the point of view of the setIsLocallyChanged method in PostModel (as of 2d0a46c15ef9c638f60331f0482b0db5b9537705). There are 6 "entry points", 5 of which are more interesting (in orange) since they set the flag to true:

flowchart LR
    A[setIsLocallyChanged]
    B["PostRestClient:326\n(sets to false)"] --> A
    C["PostUtils:573\n(sets to true)"] --> A
    style C fill:orange,color:black
    D["SavePostToDbUserCase:43\n(sets to true)"] --> A
    style D fill:orange,color:black
    E["UploadService:607\n(sets to true)"] --> A
    style E fill:orange,color:black
    F["UploadService:633\n(sets to true)"] --> A
    style F fill:orange,color:black
    G["UploadService:657\n(sets to true)"] --> A
    style G fill:orange,color:black

To get a sense of breadth and scope, we can work backwards from there (also giving clues about other areas that can affect this, such as media uploads):

flowchart RL
    A[setIsLocallyChanged]
    H[preparePostForPublish] --"!post.isLocalDraft()"--> C["PostUtils:573"] --> A
    I["savePostToDb\n[if postRepository.postHasChanges()]"] --"!post.isLocalDraft()" --> D["SavePostToDbUserCase:43"] --> A
    J[updatePostWithNewFeatureImg] --> E["UploadService:607"] --> A
    K[updatePostWithMediaUrl] --"!post.isLocalDraft"--> F["UploadService:633"] --> A
    L[updatePostWithFailedMedia] --"!post.isLocalDraft"--> G["UploadService:657"] --> A

A deeper traversal expands the graph even further, revealing quite a bit of complexity potentially underlying this issue (I have colored orange the flow discovered while debugging with the example content from this comment):

flowchart RL
    A[setIsLocallyChanged]
    style A fill:orange,color:black
    B["preparePostForPublish\n (in PostUtils)"] --> A
        G[PostUtilsWrapper:44] --> B
        H[UploadService:321] --> B
        I[UploadUtils:453] --> B
    C["savePostToDb\n (in SavePostToDbUserCase)"] ==> A
    style C fill:orange,color:black
        J[PostListMainViewModel:314] --> C
        K[StorePostViewModel:82] --> C
        L[StorePostViewModel:111] ==> C
        style L fill:orange,color:black
        M[SaveInitialPostUseCase:33] --> C
        N[StoryComposerViewModel:164] --> C
        O[StoryMediaSaveUploadBridge] --> C
    D["updatePostWithNewFeatureImg\n (in UploadService)"] --> A
        P["UploadService:474\n(updatePostWithCurrentlyCompletedUploads)"] --> D
    E["updatePostWithMediaUrl\n (in UploadService)"] --> A
        Q["UploadService:476\n(updatePostWithCurrentlyCompletedUploads)"] --> E
    F["updatePostWithFailedMedia\n in (UploadService)"] --> A
        R["UploadService:496\n(updatePostWithCurrentlyFailedUploads)"] --> F

What is perplexing to me, though, is that I did not consistently encounter this (I seemed to encounter it more frequently on a published post, though, than a draft post in my testing). I also wonder why we are saving to the local db when there are no changes to be saved. So, I also began investigating the Gutenberg side of this issue, and found that upon initializing, the autosave function is called at least twice, though the content did not change, which seems slightly suspicious to me. However, I did not observe this function invocation after tapping a block, although such an action often seemed to result in the locally changed flag being set.

I agree with previous conclusions that Aztec improvements will help resolve this issue, and I found that a recent fix has landed for resolving a similar issue regarding parsing and serialization representation differences. Given the complexity of possible sources / causes, I'm hopeful the above diagrams can be useful in resolving this completely. Ultimately, I believe these will reveal the issue to be that of multiple sources of truth (content string diff or an independent boolean flag), and fixing that without regression(s) (i.e. breaking different assumptions) in other areas may require a view of the bigger picture.

wordpress-mobile / WordPress-Android