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

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

Open
malinajirka opened this issue Oct 19, 2020 · 19 comments

Comments

@malinajirka
Copy link
Contributor

malinajirka commented Oct 19, 2020

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

  1. My Site
  2. Blog Posts
  3. 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)
  4. Press back
  5. 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
Copy link
Contributor

hypest commented Oct 19, 2020

Looks related to the ticket opened a few days ago in the gb-mobile repo: wordpress-mobile/gutenberg-mobile#2714

@hypest hypest added this to Triage in Mobile Gutenberg via automation Oct 19, 2020
@malinajirka
Copy link
Contributor Author

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
Copy link
Contributor

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

@mchowning mchowning self-assigned this Oct 20, 2020
@hypest hypest moved this from Triage to Scheduled in Mobile Gutenberg Oct 22, 2020
@hypest hypest moved this from Scheduled to In progress in Mobile Gutenberg Oct 22, 2020
@mchowning
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

I suspect this may be related to wordpress-mobile/gutenberg-mobile#894.

@malinajirka
Copy link
Contributor Author

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
Copy link
Contributor

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

@designsimply
Copy link
Contributor

designsimply commented Jan 5, 2021

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 #13003 (comment)).

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
Copy link
Contributor

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 antonis self-assigned this Apr 12, 2021
@antonis
Copy link
Member

antonis commented Apr 14, 2021

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
localchanges.mp4

@antonis antonis removed their assignment Apr 19, 2021
@develric
Copy link
Contributor

develric commented May 17, 2021

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.

image

image

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
Copy link
Member

dcalhoun commented Jul 7, 2021

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 #13169 (comment).

Reproducible Post HTML
<!-- wp:spacer {"height":90} -->
<div style="height:90px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:preformatted -->
<pre class="wp-block-preformatted"><br>coding is fun</pre>
<!-- /wp:preformatted -->

<!-- wp:preformatted -->
<pre class="wp-block-preformatted"><br>coding is fun</pre>
<!-- /wp:preformatted -->

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.

wpandroid-block-parsing

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
Copy link
Contributor

mkevins commented Jul 8, 2021

Hi @dcalhoun 👋 😄 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 🤔

@mchowning
Copy link
Contributor

mchowning commented Jul 8, 2021

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>

<!-- wp:cover {"url":"https://cldup.com/cXyG__fTLN.jpg","id":890,"dimRatio":20,"overlayColor":"luminous-vivid-orange","focalPoint":{"x":"0.63","y":"0.83"},"minHeight":219} -->
from the web gets gets a bunch of escape characters added:
<!-- wp:cover {"url":"https:\/\/cldup.com\/cXyG__fTLN.jpg","id":890,"dimRatio":20,"overlayColor":"luminous-vivid-orange","focalPoint":{"x":"0.63","y":"0.83"},"minHeight":219} -->

<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"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","className":"has-text-color has-very-light-gray-color","fontSize":"large"} -->
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"><!-- wp:paragraph {"align":"center","placeholder":"Write title\u2026","className":"has-text-color has-very-light-gray-color","fontSize":"large"} -->

@dcalhoun
Copy link
Member

dcalhoun commented Jul 9, 2021

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
Copy link

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
Copy link
Contributor

zwarm commented Feb 14, 2022

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
Copy link
Contributor

mkevins commented Mar 2, 2022

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 2d0a46c). 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Mobile Gutenberg
  
In progress
Development

No branches or pull requests