Fixes color picker segmented control rendering and scrolling issues #30994
Conversation
antonis
changed the title
|
Size Change: 0 B Total Size: 1.31 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
I encountered two issues while testing these changes on a Samsung Galaxy S20.
First, selecting a custom color and toggling the segment resulted in the horizontal color list to be incorrectly scrolled. One can merely swipe to "recover" the scrollable list, but it does create an odd looking state.
Custom color segment overflow
Screen_Recording_20210420-224225_WordPress.Pre-Alpha.mp4
Second, when switching from a customized gradient to a built-in gradient, the Custom button does not "slide out" with an animation. This is likely not a big issue, but it is included in the TC005 color settings test case expectation, so I wanted to draw attention to it.
Custom gradient button animation
Screen_Recording_20210420-234416_WordPress.Pre-Alpha.mp4
| }, [ | ||
| shouldShowCustomIndicatorOption, | ||
| isGradientSegment, | ||
| isCustomGradientColor, | ||
| ] ); |
There was a problem hiding this comment.
I think reverting #28740 is likely a fine approach, but I am curious as to why it was not working.
I haven't explored it fully, but I wonder if the dependency array is not inclusive enough. It included isCustomGradientColor, but that value is computed with isSelectedCustom, which relies upon the values activeColor, colors, isGradientSegment, and isGradientColor.
I would think React would appropriately "watch" for changes to all those sub-dependencies of isSelectedCustom, but I wonder if maybe that is not occurring. Maybe we need to declare all of those values as dependencies for this hook?
There was a problem hiding this comment.
Thank you for your feedback and reviewing this so quickly @dcalhoun 🙇
I was able to reproduce both the reported issues.
Maybe we need to declare all of those values as dependencies for this hook?
I will revisit this with this approach and report back 👍
There was a problem hiding this comment.
Hey @dcalhoun 👋
Thank you once more for the tip on this. Passing all the values and doing the calculation within the hook (85ff940) only fixes the disappearing custom gradient and not the original issue. Tbh I'm still a bit confused on why this happens only only compiled builds. I'll continue the investigation and get back 😕
There was a problem hiding this comment.
Interesting. #30878 seems like either a style issue (i.e. some flex styles are conflicting or causing rendering issues), a calculation issue (i.e. there is somewhere else where we do not always re-calculate when we should), or a race condition (e.g. a computation occurs during layout and finishes at different times).
I might start by placing "debug styles" on each element to see which element is the offending item "cutting off" the children. Specifically, I know ScrollView contains multiple elements, a container and a content container. Which one is rendering too skinny?
Relatedly, the broken scroll-to-end for custom colors/gradients (wordpress-mobile/gutenberg-mobile#3104) is likely caused by the same issue where we compute the status within the hook, but do not have a dependency declared on the sub-dependency values.
gutenberg/packages/components/src/color-palette/index.native.js
Lines 102 to 110 in 85ff940
Applying a similar change as 85ff940 would likely resolve the scroll issue.
antonis
force-pushed
the
rnmobile/30878-colorpicker
branch
from
d8156c1
to
a18a140
Compare
There was a problem hiding this comment.
I noticed a few issues on Android in my testing.
First, often times when a Custom gradient is not enabled, the space remains for the Custom gradient to display. So, there is a gap at the end of the gradient list.
Screenshot: Custom gradient space
Screen_Recording_20210423-210413_WordPress.Pre-Alpha.mp4
Second, when switching between the two segments, there is often a "jump" in the animation. I would imagine this may be related to the delay added to setting the scroll position. The UI is still functional, but the transition isn't the smoothest.
Screenshot: Segment animation jump
Screen_Recording_20210423-210011_WordPress.Pre-Alpha.mp4
For iOS, I wanted to point out conflicting statements in this PR's description where it claims to fix wordpress-mobile/gutenberg-mobile#3104, but then lists it as a "known issue." That issue is not necessarily directly related to #30878, so I understand if we do not want to address it in this PR, but we should be clear about what is included within the description.
Fixes: #30878 and wordpress-mobile/gutenberg-mobile#3104
The disappearing custom gradient persists on iOS release builds and is further investigated.
| const delayedScroll = setTimeout( () => { | ||
| resetScrollPosition(); | ||
| }, SEGMENTED_CONTROL_ANIMATION_DURATION_DELAY ); |
There was a problem hiding this comment.
What is the delayed scroll addressing?
There was a problem hiding this comment.
Thank you once more for the feedback @dcalhoun 🙇
My understanding is that the scrolling doesn't complete because when the value of currentSegment changes there is an extra delay before the selected segment ScrollView is rendered due to the SegmentedControl animation. Thus the scrolling occurs on the old segment selection.
I tried various approaches (e.g. using useLayoutEffect instead of useEffect) and tweaking SegmentedControl implementation but nothing did the trick :(
I understand that this is more of a hack and not a proper solution. Any suggestions or hints on better approaches are more than welcome.
There was a problem hiding this comment.
Ah, OK. I see. If that is the case, my recommendation would be to rely upon the pre-existing ANIMATION_DURATION constant. To me, that would communicate much clearer what the function is waiting on. Additionally, I do not believe we need the inline anonymous function. We could likely reference pass resetScrollPosition directly.
const delayedScroll = setTimeout( resetScrollPosition, ANIMATION_DURATION );That said, I suppose setting a delay is an alternative approach to "track the contentWidth and scroll when it changes" that was removed, correct? Did you find that original approach was causing issues?
There was a problem hiding this comment.
Thank you for your feedback David 🙇
Ah, OK. I see. If that is the case, my recommendation would be to rely upon the pre-existing
ANIMATION_DURATIONconstant. To me, that would communicate much clearer what the function is waiting on. Additionally, I do not believe we need the inline anonymous function. We could likely reference passresetScrollPositiondirectly.
Great points. Updated with cc018b7
That said, I suppose setting a delay is an alternative approach to "track the
contentWidthand scroll when it changes" that was removed, correct? Did you find that original approach was causing issues?
The original approach was not causing the issue but did not help in solving it either. The content size changes still set the scroll position directly. I found that tracking the contentWidth was not necessary since it was actually used only on Android and the workaround seemed to have no actual effect any longer.
There was a problem hiding this comment.
Hey @dcalhoun 👋
I updated the PR and added some extra notes on the known issues. Both the original issues are now resolved (
#30878 and wordpress-mobile/gutenberg-mobile#3104) and the missing custom gradient button is now back on both platforms.
antonis
force-pushed
the
rnmobile/30878-colorpicker
branch
2 times, most recently
from
e25ab69
to
cc018b7
Compare
There was a problem hiding this comment.
I tested the latest builds. Overall things are operating well. The two original issues no longer occur. However, there do appear to be lingering issues. I will attempt to make time to investigate further, but I wanted to share what I have noticed sooner rather than later.
| Priority | Issue | Reference | Thoughts |
|---|---|---|---|
| 🔴 | Android Colors unscrollable | New | Sporadically the Colors segment will not be scrollable. I haven't been able to pin down why it occurs at this point, but often times it shows up when I switch segments a lot. |
| 🟡 | Empty Custom gradient space | #30994 (review) | Sporadically there is an empty space where a Custom gradient button would appear if it were enabled. |
| 🟡 | Segment animation jump | #30994 (review) | I understand if we do not want to spend time smoothing the animation now, but we may want to create a new issue for it. |
| 🟡 | Custom gradient button animation | #30994 (review) | Another transition, which we could postpone fixing, but it is referenced in the test cases, so we should likely update the test case with a new issue. |
Thank you for thoroughly testing this @dcalhoun 🙇 I haven't been able to reproduce the 1st issue yet. I will check this and report back. |
antonis
force-pushed
the
rnmobile/30878-colorpicker
branch
from
509588e
to
9094f38
Compare
|
Hey @dcalhoun 👋
I was able to reproduce the reported unscrollable colors issue on an older Pixel device but unfortunately wasn't able to find a solution to this. Thus after several failed attempts I retracted to the initial solution of reverting #28740 and adding a delay to solve the Custom color segment overflow issue.
The only issue that persisted on my tests was the smooth slide out animation when deselecting a custom gradient(TC005). For this we can create a new issue and reference it in the test case till it is resolved. Wdyt? |
I think the reversion makes since at this point. Thank you for exploring all the options.
Agreed. Moving forward with the fix we have in place seems like the best approach. We can create a new issue for the remain bug and update the test case with a reference. |
Fixes: #30878 and wordpress-mobile/gutenberg-mobile#3104
gutenberg-mobile: wordpress-mobile/gutenberg-mobile#3394WordPress-Android: wordpress-mobile/WordPress-Android#14492WordPress-iOS: wordpress-mobile/WordPress-iOS#16366Description
This reverts #28740 falling back to the previous workaround for fixing the crash occurring when segments are changed quickly.
The applied solution is based on rearranging components which might not be optimal but does not cause the following two issues: #30878 and #3104.
The applied solution additionally adds a delay when scrolling to the end to counteract the segment selection delay.
How has this been tested?
Notes: The tests bellow should be performed with builds from the Android PR and iOS PR
Crash regression check
Custom gradient button is visible
ButtonsblockBackground coloroptionGradientoptionCustomize GradientGradient TypetoRadial(it also works changing other options)SolidoptionGradientoption againColor picker is not cropped
Sanity testing test cases
Known issues:
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).