Fix skeletons interfering with attributed strings in text-based views

[p4checo]

Summary

When enabling skeleton mode in a text-based view (UILabel, UITextView, UITextField), it sets the textColor to .clear, which is fine when text is used, but causes problems when attributedText is used, as it effectively "resets" the string to have a single color.

Additionally, when a UILabel is nested inside a UIStackView a dummy string " " was set on the label's text so that it didn't have a 0 height content size. However, this workaround didn't consider the case where the label already had a non-empty text, meaning that this (intrusive) text = " " broke existing code by clearing the label's contents.

By improving the corresponding RecoverableXState structs, we are able to preserve each element's contents and state as skeleton is disabled.

Fixes #518.

Changes

• Create new RecoverableLabelState containing a attributedText and text, and use it on UILabel.

• Update RecoverableTextViewState and RecoverableTextFieldState to have a attributedText.

• Check if UILabel's text is empty before setting dummy value when enabling skeleton mode in a label nested inside a UIStackView.

Requirements (place an x in each of the [ ])

• [x] I've read and understood the Contributing guidelines and have done my best effort to follow them.
• [x] I've read and agree to the Code of Conduct.

[markos92i]

Is there an estimated elapsed time for this to be merged and a new release of the library to come out? We had to implement a very ugly fix on our own, to all the labels that are affected by this, and it would be nice to get rid of it as soon as possible.

Thank you por taking your time to develop this fix.

[Juanpe]

Hi there! Thanks for this fix. LGTM! 👍🏼

It will be merged and release now 🚀

[lifely]

I'm using skeleton view inside a simple stack view that gets updated after a network calls, This change broken my code.

I can't access the saveState or force a new state after I updated the content while the skeleton is shown, thus once the skeleton is hidden i loose my data through that saveState.

There also no control of this, it would be nice if all feature integrated was non breaking and optionable before it is merged.

[p4checo]

hi @lifely,

I'm sorry this change broke your code. Can you clarify how exactly it broke your code?

These changes restore the text-based views state between enabling and disabling skeleton mode, so if you update their contents (e.g. text, attributedText) while skeleton mode is enabled then those changes will be lost. Have you tried setting the content from the network call after calling hideSkeleton on the views?

TBH, I am also not a fan of how intrusive SkeletonView can be in regards to the UI elements' contents (e.g. background color, text color, etc), but that's how the library works and I don't see any straightforward alternative.

[lawmaestro]

This change causes an unexpected break for me too. My use-case is the one you described @p4checo I'm populating some text views on the completion of a network request and then calling hideSkeleton. Setting the text and then unhiding seems (at least to my mind) the logical way to order this. I'm unfortunately not in a position to be able to hide before the text is set as this is in two unrelated parts of the code. So I'll have to figure out some other workaround for now or fix to the previous version :/

Looking at the change, I'm wondering what the value in having self.text = storedLabelState.text in recoverViewState is? This would certainly seem to be the offending line anyway...

[p4checo]

hi @lawmaestro

I'm populating some text views on the completion of a network request and then calling hideSkeleton. Setting the text and then unhiding seems (at least to my mind) the logical way to order this.

This is debatable IMO. Ideally we should do both things as a part of the same transition/animation, otherwise you need to perfectly align the skeletons with the content underneath to not have visual artifacts. Unfortunately SkeletonView doesn't provide an API for this, so one has to work around it "manually" wrapping changes or simply do it sequentially either before or after.

Previous to this change, didn't you notice any visual artifacts? Unless your skeletons perfectly align with the text on the label, when you set the text before hiding the skeleton view you could likely see the text and the skeleton simultaneously for a while.

I'm unfortunately not in a position to be able to hide before the text is set as this is in two unrelated parts of the code.

Should they be unrelated though? They seem pretty related to me 😅

Anyway, if you look at the change you will see the scenario where SkeletonView sets the label's text. So, even before this change you would already have problems if your label was nested inside a UIStackView.

As SkeletonView relies on clearing the text/making it invisible when enabling skeleton mode, to address your problem we could check if the view's contents have changed while in skeleton mode, and if so don't apply the respective recoverable state.

So I'll have to figure out some other workaround for now or fix to the previous version :/

I heard they accept PRs in this repo, so maybe take a stab at it? 😇

[lawmaestro]

Ideally we should do both things as a part of the same transition/animation, otherwise you need to perfectly align the skeletons with the content underneath to not have visual artifacts. Unfortunately SkeletonView doesn't provide an API for this, so one has to work around it "manually" wrapping changes or simply do it sequentially either before or after.

In my example the view controller responds to the loaded model state by hiding the skeleton view. The view model which actually drives the views textual state also responds to the model loaded state. It's however added as a subscriber before the VC. So they both respond within the same main thread frame, albeit they are different observers responding in their different ways.

As for alignment of the skeleton with the actual text - this is always an estimation as the length of the text can (in probably most cases) only be estimated. So a sensible whitespace dummy padding of the strings initial state provides a skeleton which is visually representative. At least this is how I seemed able to get the best results.

Previous to this change, didn't you notice any visual artifacts? Unless your skeletons perfectly align with the text on the label, when you set the text before hiding the skeleton view you could likely see the text and the skeleton simultaneously for a while.

No. There's no artefact when the skeleton hides and the text shows as both are done in the same frame.

Anyway, if you look at the change you will see the scenario where SkeletonView sets the label's text. So, even before this change you would already have problems if your label was nested inside a UIStackView.

No. In the case where the label has text set (in my case a dummy whitespace initial setting), this setting of the text by SkeletonView doesn't happen - perhaps this is why I've never previously had an issue with it. If you take a step back from it, having any setting of text by what is essentially an eye candy UI library seems strange (not to mention a little unexpected).

I'm open to making a PR but I feel at this stage like we've got differing opinions on this, or at the very least we're seeing it in different ways...

[p4checo]

In my example the view controller responds to the loaded model state by hiding the skeleton view. The view model which actually drives the views textual state also responds to the model loaded state. It's however added as a subscriber before the VC. So they both respond within the same main thread frame, albeit they are different observers responding in their different ways.

Not sure what a main thread frame is, but if they are different observers they will be executed in sequence, even if both being observed on the main thread or runloop. Have you tried swapping the observation order? Not sure in Combine, but it does have an effect in ReactiveSwift for instance.

No. There's no artefact when the skeleton hides and the text shows as both are done in the same frame.

Ok.

If you take a step back from it, having any setting of text by what is essentially an eye candy UI library seems strange (not to mention a little unexpected).

I totally agree with you on this and I have mentioned it already. I really dislike the intrusive nature of SkeletonView. However, I I can't think of a better solution in UIKit. Can you?

I'm open to making a PR but I feel at this stage like we've got differing opinions on this, or at the very least we're seeing it in different ways...

I am not the maintainer of this repo, so I would say my opinion is not very relevant. Just curious, what would be your suggestion to fix this?