[RNMobile] Fix visual glitch when loading Aztec view #30838
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fluiddot
added
[Type] Bug
|
Size Change: +5.21 kB (0%) Total Size: 1.46 MB
ℹ️ View Unchanged
|
42 tasks
guarani
reviewed
Apr 20, 2021
There was a problem hiding this comment.
React Native Aztec view is initialized by default with size (width: 10, height: 10), this produces weird sizes when calculating the content size via the
sizeThatFitsmethod.
I was wondering where the size of 10x10 was coming from and it looks like it's from in Aztec here. Viewing the previous commit for that suggests an arbitrary default size might have been used. (I'm not 100% sure if that comment is still valid, after all it was deleted).
Do you think setting a zero default size in Aztec would be a viable alternative to doing it here in Aztec's RN wrapper?
packages/react-native-aztec/ios/RNTAztecView/RCTAztecView.swift
Outdated
Show resolved
Hide resolved
Co-authored-by: Paul Von Schrottky <[email protected]>
Yeah, the 10x10 frame is set when the view is initialized from Aztec (thanks for pointing the code reference 🙇 ) but I couldn't find either the reason of having that specific size. It's still unclear to me the implications of having a zero vs random frame but surprisingly this affects the calculations for the
I thought about applying this change in Aztec at the beginning, but since this is a workaround for calculating the view's size for RN, I finally decided to apply it only here. I guess it would be safe to move the fix to the Aztec repo but I wanted to reduce the potential side effects of this change, wdyt? EDIT: I'm thinking to add a comment in this line explaining the workaround. |
guarani
approved these changes
Apr 20, 2021
There was a problem hiding this comment.
I agree it's fine to merge this change here. I tested this and all looks good, I think the change makes sense given that it's a zero default size which is more reasonable than the arbitrary default size of 10x10 this had before.
I tested on WPiOS using reusable blocks and all good! Thanks a lot for the fix.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes wordpress-mobile/gutenberg-mobile#2976.
React Native Aztec view is initialized by default with size (width: 10, height: 10), this produces weird sizes when calculating the content size via the
sizeThatFitsmethod.As an example, here are the different sizes produced when a block using Aztec is rendered and calls
sizeThatFits:Values with current changes
Paragraph block with one line content
Content:
<p>Hello world!</p>First render:
size: (w: 10, h: 10)[Initial size]fittingSize: (w: 10.0, h: 218.0)[For some reason it calculates a bigger height that it should 🤷♂️]minimumHeight: 22.0resultSize: (w: 10.0, h: 218.0)Second render:
size:(w: 343.0, h: 218.0)fittingSize:(w: 95.5, h: 22.0)[On the second pass it gets the correct height, in this case it's the minimum because the content is one line]minimumHeight: 22.0result: (w: 95.5, h: 22.0)Paragraph block with three lines content
Content:
<p>Line 1<br>Line 2<br>Line 3</p>First render:
size:(w: 10.0, 10.0)fittingSize:(w: 10.0, 327.0)[Same as in the previous example, the height is bigger than it should]minimumHeight: 22.0result: (w: 10.0, 327.0)Second render:
size:(w: 343.0, 327.0)fittingSize:(w: 47.5, 65.5)[Same as in the previous example, the height is properly calculated on the second pass, in this case bigger because the content is three lines]minimumHeight: 22.0result: (w: 47.5, 65.5)Values with changes from this PR
Surprisingly if the initial size is set to zero
(w: 0, h: 0), the calculations are correct:Paragraph block with one line content
Content:
<p>Hello world!</p>First render:
size:(0.0, 0.0)fittingSize:(95.5, 22.0)[The size is properly calculated in the first pass]minimumHeight: 22.0result: (95.5, 22.0)Second render:
sizeThatFits: size:(343.0, 22.0)fittingSize:(95.5, 22.0)minimumHeight: 22.0result: (95.5, 22.0)Paragraph block with three lines content
Content:
<p>Line 1<br>Line 2<br>Line 3</p>First render:
size:(0.0, 0.0)fittingSize:(47.5, 65.5)[Same as in the previous example, the size is properly calculated in the first pass]minimumHeight: 22.0result: (47.5, 65.5)Second render:
size:(343.0, 65.5)fittingSize:(47.5, 65.5)minimumHeight: 22.0result: (47.5, 65.5)How has this been tested?
The visual glitch can be hard to identify because it only happens on a second’s fraction, the way I tested it involves the following:
Check size calculation
print("sizeThatFits: size:\(size), fittingSize:\(fittingSize), minimumHeight: \(minimumHeight), content: \(storage.getHTML()), result: \(CGSize(width: fittingSize.width, height: height))")Visual glitch
...{ backgroundColor: 'red' },Screenshots
NOTE: Play it frame by frame to verify the fix.
fix-loading-glitch.mp4
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).