Cover block upload completion processor #11541

mkevins · 2020-03-27T09:35:22Z

Fixes: wordpress-mobile/gutenberg-mobile#1739

Description

This PR implements the upload completion processor for the Cover block.

Related PRs:

gutenberg: TBD
gutenberg-mobile: TBD
WordPress-iOS: [RNMobile] Cover with in Cover support WordPress-iOS#13928

To test:

Unit tests:

Run all unit tests in WordPress/src/test/java/org/wordpress/android/ui/posts/mediauploadcompletionprocessors/

Manual tests:

Close/Re-open post with an ongoing image upload - steps
Close post with an ongoing image upload - steps

PR submission checklist:

I have considered adding unit tests where possible.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

peril-wordpress-mobile · 2020-03-27T09:35:27Z

	Warnings
⚠️	PR is not assigned to a milestone.
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

peril-wordpress-mobile · 2020-03-27T09:45:18Z

You can test the changes on this Pull Request by downloading the APK here.

peril-wordpress-mobile · 2020-04-07T08:17:13Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

dmsnell · 2020-04-08T04:26:10Z

...java/org/wordpress/android/ui/posts/mediauploadcompletionprocessors/CoverBlockProcessor.java

+    private static final Pattern PATTERN_COVER_INNER = Pattern.compile(new StringBuilder()
+                    .append("(^.*?<div class=\"wp-block-cover__inner-container\">\\s*)")
+                    .append("(.*)") // inner block contents
+                    .append("(\\s*</div>\\s*</div>\\s*<!-- /wp:cover -->.*)").toString(), Pattern.DOTALL);


oops. I had left a comment here that was wrong. you can take some inspiration from hpq which @aduth wrote. something like jsoup (random internet search) could automate the inference of the block attributes as defined in the block implementation, rather than falling back to attempts at parsing hTML with RegExp

Hi @dmsnell 👋 😃 , thanks for taking a look at this one. I'm using jsoup already, which is really helpful. IIRC, I encountered some minor differences in output in certain scenarios, possible because it parses to an AST and not a parse tree, however, it is probably worth revisiting, as the variations might still be acceptable to pass validation. 👍

I encountered some minor differences in output in certain scenarios, possible because it parses to an AST and not a parse tree

Do you remember what those differences were? Any difference would have to be a bug since we're dealing with HTML/CSS selectors…

also it looks like this entire pattern shouldn't be necessary if we process the inner blocks using a spec-compliant parser

Do you remember what those differences were? Any difference would have to be a bug

Off-hand, I recall that jsoup was adding closing tags, which was problematic during at least one of the iterations toward the current implementation. It's likely this is resolved now, as the implementation no longer processes units of non-self-contained HTML, so probably worth revisiting. To be sure, this was not a bug in jsoup, but rather a limitation in a particular iteration of this implementation, in that it required serialization of the parse tree, while jsoup is designed to serialize abstract trees.

also it looks like this entire pattern shouldn't be necessary if we process the inner blocks using a spec-compliant parser

Indeed, using a spec-compliant parser would obviate the need for this, as we'd be able to handle inner blocks an a much more general way, albeit for all block types.

dmsnell · 2020-04-10T21:46:50Z

...main/java/org/wordpress/android/ui/posts/mediauploadcompletionprocessors/BlockProcessor.java

+     * @return A string containing content with ids and urls replaced
+     */
+    String processInnerBlock(String block) {
+        return block;


Is there a reason to not make the default processor process inner blocks? It will come surprisingly often that the blocks we care about are nested, for example, when people build with the full-site-editing layout blocks, or the Group block, or the Quote block, or the Gallery block…

Is there a reason to not make the default processor process inner blocks?

For the current implementation of the processors, they are not full-blown parsers, but part of their behavior resembles a parser. Instead, the processors are only concerned with a subset of block types (image, video, etc.). To hopefully clarify: when processContent encounters a Group block, it does not match on that block, but if that block's inner contents contains a media-containing block, that will match and be processed further (via processBlock), while all content preceeding that match is treated as a raw string. This is a way to produce the desired output without the need to implement any serialization logic for all potential block types.

dmsnell · 2020-04-10T21:54:43Z

...java/org/wordpress/android/ui/posts/mediauploadcompletionprocessors/CoverBlockProcessor.java

+    /**
+     * Pattern to match background-image url in cover block html content
+     */
+    private static final Pattern PATTERN_BACKGROUND_IMAGE_URL = Pattern.compile("background-image:url\\([^\\)]+\\)");


so definitely there's an issue with blocks defining their attributes in the "JSON attributes" vs. inside HTML, but instead of hard-coding a string-matching pattern we can go back to jsoup as you were doing and hard-code the known attribute definition, which would then end up looking like this…

JSONAttribute backgroundImageUrl = new JSONAttribute("url", JSONAttribute.String);

I'm sure there's a million ways to do this. The main point I was trying to make is that we can rely on the structural form of the data instead of processing the HTML string as if it were plain text. It's quite possible to have background-image:url inside the block in a way that isn't meant to indicate a different background, and this string-matching might end up paving a road of headaches to debug 😄

It's quite possible to have background-image:url inside the block in a way that isn't meant to indicate a different background, and this string-matching might end up paving a road of headaches to debug

Ah, indeed! I wonder if there is a misunderstanding about how this pattern is currently used. This only matches on the style attribute within the parsed HTML of a matching block, and so the string replacement is restricted to that particular inline style. In other words, it will not replace similar text within the content of HTML (e.g. a caption, a paragraph, etc.). Perhaps the Javadoc could be made more specific.

dmsnell · 2020-04-10T21:56:39Z

...java/org/wordpress/android/ui/posts/mediauploadcompletionprocessors/CoverBlockProcessor.java

+            // replace background-image url in style attribute
+            String style = PATTERN_BACKGROUND_IMAGE_URL.matcher(targetDiv.attr("style"))
+                    .replaceFirst(String.format("background-image:url(%1$s)", mRemoteUrl));
+            targetDiv.attr("style", style);


hm…I'm seeing the cover block actually store the background imagine inside the JSON attributes. this will deifnitely update the style, but won't the newly-uploaded image get wiped out after someone loads the post in the editor again and there's a mismatch between what the attributes indicate and what HTML has been generated?

we probably need to update both places.

we probably need to update both places

The Json attributes are updated in an earlier step via the abstract method processBlockJsonAttributes. For Cover block, the id is used to check for a match first, and the url is mutated a match with the local id is found. The replacement within the style attribute only occurs subsequently, if that condition is met.

chipsnyder

LGTM! Everything was working as expected. Nice work!

✅ Close/Re-open post with an ongoing image upload - steps
✅ Close post with an ongoing image upload - steps

pinarol · 2020-04-17T07:30:11Z

Friendly reminder: We should merge into gutenberg/after_1.26.0 instead of develop until Monday. After Monday we can target develop again.

chipsnyder

Reran some test cases and took a look at the changes. Nice catch! Looks good 🥳

mkevins · 2020-04-22T04:02:56Z

Thanks for reviewing and testing 👍 .

mkevins added 2 commits March 27, 2020 19:20

Update gutenberg-mobile reference

3f6e8e0

Update gutenberg-mobile reference

cccadc8

mkevins added Media Gutenberg Editing and display of Gutenberg blocks. gutenberg-mobile labels Mar 27, 2020

This was referenced Mar 27, 2020

Enable cover block uploads behind dev flag wordpress-mobile/gutenberg-mobile#2070

Merged

[Mobile] Cover block upload progress WordPress/gutenberg#21197

Merged

[Mobile] Enable cover block uploads - dev only WordPress/gutenberg#21196

Merged

mkevins mentioned this pull request Mar 30, 2020

[Testing] Enable cover block uploads behind dev flag wordpress-mobile/WordPress-iOS#13769

Closed

3 tasks

mkevins added 15 commits April 6, 2020 20:39

Add some patterns for media upload completion processing

89cce9d

fixup

e56de3e

Add some tests for cover block upload completion processing

8acc71d

Add cover block upload completion processor

15e381b

fixup

8972433

fixup

17d76d4

Handle nested blocks in processPost

5a22a08

Update gutenberg-mobile reference

3949fce

Merge branch 'develop' into feature/cover-block-uploads

a476918

Move json attributes processor to abstract block processor

57bc35d

Remove some cruft from upload completion processor patterns

812ee7f

Use json parsing in other media-block processors

5e1c92d

Use static map in media block type enum

c38ba61

Use static pattern in media block type enum

d2c8a2f

Add some comments and cleanup lint

80ffb78

mkevins changed the title ~~[Testing] Enable cover block uploads behind dev flag~~ Cover block upload completion processor Apr 7, 2020

mkevins mentioned this pull request Apr 7, 2020

[Mobile] Cover block uploads failure UI WordPress/gutenberg#21198

Merged

6 tasks

dmsnell reviewed Apr 8, 2020

View reviewed changes

mkevins mentioned this pull request Apr 10, 2020

Cover block uploads failure UI wordpress-mobile/gutenberg-mobile#2150

Merged

2 tasks

mkevins added 3 commits April 10, 2020 19:18

Update gutenberg-mobile reference

c50b104

Merge branch 'develop' into feature/cover-block-uploads

0a02cf8

Update gutenberg-mobile reference

e6c78af

mkevins requested a review from chipsnyder April 10, 2020 11:59

mkevins marked this pull request as ready for review April 10, 2020 12:00

dmsnell reviewed Apr 10, 2020

View reviewed changes

mkevins added 5 commits April 14, 2020 14:54

Add unit test for malformed Cover block

5acd7dc

Fix base case in processContent method

57dcd82

Update gutenberg-mobile reference

aba1654

Merge branch 'develop' into feature/cover-block-uploads

d4da054

Update gutenberg-mobile reference

29d19b4

chipsnyder approved these changes Apr 15, 2020

View reviewed changes

mkevins added 7 commits April 21, 2020 17:35

Add upload processor test with different inline style order

f1e2f6e

Add upload processor test with space in inline styles

4ba5103

Allow for spaces in inline styles for cover block upload processor

4716f03

Update gutenberg-mobile reference

fcfbe8f

Merge branch 'develop' into feature/cover-block-uploads

4e11d7b

Fix lint

f1e302d

Fix ktlint

f053f12

chipsnyder approved these changes Apr 21, 2020

View reviewed changes

mkevins merged commit b3d296e into develop Apr 22, 2020

mkevins deleted the feature/cover-block-uploads branch April 22, 2020 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cover block upload completion processor #11541

Cover block upload completion processor #11541

mkevins commented Mar 27, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Mar 27, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Mar 27, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Apr 7, 2020 •

edited

Loading

dmsnell Apr 8, 2020

mkevins Apr 10, 2020

dmsnell Apr 10, 2020

dmsnell Apr 10, 2020

mkevins Apr 13, 2020

dmsnell Apr 10, 2020

mkevins Apr 13, 2020 •

edited

Loading

dmsnell Apr 10, 2020

mkevins Apr 13, 2020

dmsnell Apr 10, 2020

mkevins Apr 13, 2020

chipsnyder left a comment

pinarol commented Apr 17, 2020

chipsnyder left a comment •

edited

Loading

mkevins commented Apr 22, 2020

Cover block upload completion processor #11541

Cover block upload completion processor #11541

Conversation

mkevins commented Mar 27, 2020 • edited Loading

Description

Related PRs:

To test:

Unit tests:

Manual tests:

peril-wordpress-mobile bot commented Mar 27, 2020 • edited Loading

peril-wordpress-mobile bot commented Mar 27, 2020 • edited Loading

peril-wordpress-mobile bot commented Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkevins Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipsnyder left a comment

Choose a reason for hiding this comment

pinarol commented Apr 17, 2020

chipsnyder left a comment • edited Loading

Choose a reason for hiding this comment

mkevins commented Apr 22, 2020

mkevins commented Mar 27, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Mar 27, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Mar 27, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Apr 7, 2020 •

edited

Loading

mkevins Apr 13, 2020 •

edited

Loading

chipsnyder left a comment •

edited

Loading