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

fix(ios): shadow issue fixed #12005

Merged
merged 3 commits into from Sep 9, 2020
Merged

Conversation

vijaysingh-axway
Copy link
Contributor

@vijaysingh-axway vijaysingh-axway commented Sep 3, 2020

https://jira.appcelerator.org/browse/TIMOB-28103
https://jira.appcelerator.org/browse/TIMOB-28110
Note-

  1. If borderRadius has single value, self.layer is used for shadowLayer and borderLayer, to use APIs available with self.layer which will be faster.
  2. If borderRadius has multiple values, use bezierPath to create corner radius. Create custom shadowLayer and borderLayer for shadow and borderColor.
  3. If custom shadowLayer is there, animation on shadowLayer will not work. It will work for view. Reason is UIView has API to animate the view and for CALayer we use CABasicAnimation which do not exactly match with UIView's animation.
  4. If there is no custom shadowLayer (which means there is single borderRadius value), animation will work properly on shadow.

@build build added this to the 9.2.0 milestone Sep 3, 2020
@build build requested a review from a team September 3, 2020 01:29
@build build added the ios label Sep 3, 2020
@build
Copy link
Contributor

build commented Sep 3, 2020

Fails
🚫 Tests have failed, see below for more information.
Messages
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 4 tests have failed There are 4 tests failing and 711 skipped out of 8156 total tests.

Tests:

ClassnameNameTimeError
ios.ipad.Titanium.UI.TabGroupadd Map.View to TabGroup (14.0)15.002
Error: timeout of 15000ms exceeded
file:///Users/build/Library/Developer/CoreSimulator/Devices/083EB293-6DD6-4119-A4D5-72351034E69F/data/Containers/Bundle/Application/EA3118B7-BCF6-456E-A8EC-670186FDF793/mocha.app/ti-mocha.js:4290:32
ios.ipad.Titanium.UI.WebView#findString() #findString with configuration (14.0)29.998
Error: timeout of 30000ms exceeded
file:///Users/build/Library/Developer/CoreSimulator/Devices/083EB293-6DD6-4119-A4D5-72351034E69F/data/Containers/Bundle/Application/EA3118B7-BCF6-456E-A8EC-670186FDF793/mocha.app/ti-mocha.js:4326:27
ios.ipad.Titanium.UI.WebView#findString() #findString without configuration (14.0)29.999
Error: timeout of 30000ms exceeded
file:///Users/build/Library/Developer/CoreSimulator/Devices/083EB293-6DD6-4119-A4D5-72351034E69F/data/Containers/Bundle/Application/EA3118B7-BCF6-456E-A8EC-670186FDF793/mocha.app/ti-mocha.js:4326:27
ios.ipad.Titanium.UI.WebViewbeforeload (14.0)30.001
Error: timeout of 30000ms exceeded
file:///Users/build/Library/Developer/CoreSimulator/Devices/083EB293-6DD6-4119-A4D5-72351034E69F/data/Containers/Bundle/Application/EA3118B7-BCF6-456E-A8EC-670186FDF793/mocha.app/ti-mocha.js:4326:27

Generated by 🚫 dangerJS against 559429b

@sgtcoolguy
Copy link
Contributor

Needs to be rebased now that the cornerRadius fix has landed. Also, we likely want some UI tests to verify the look with a shadow and cornerRadius.

@sgtcoolguy
Copy link
Contributor

This seems very complicated to me. Are we sure we need to mess around with additional layers just to achieve this? There must be a simpler way...

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After going deep down the rabbit hole, it appears it is not easier.

See:

But I do think we could benefit greatly from explicitly defining a root layer that we attach the clip/content/shadow layers to as sublayers - then I think it may also properly animate them together. The first link shows some code where they're explicitly making the UIView.layer the rootLayer and then adding shadowLayer as sublayer, then a contentLayer as sublayer of the shadow - and apply the same clipping path as the mask for the content and the path of the shadow.
So:

 -- rootLayer (UIView.layer)
  |-- shadowLayer
     |-- contentLayer (mask is the same as parent's path, defined by the cornerRadius path stuff) 

The second link sort of sneakily "copies" the contents into a sublayer if we have a shadow and cornerRadius. (so in effect, it converts a single layer to a a rootLayer with shadow properties and sublayer with the contents and the cornerRadius w/mask) I think it relies on the fact that a shadow is supposed to automatically look at sublayers to determine it's path/look.

So the concept of introducing a shadow layer as you do here makes sense, but I do think your changes place it outside the view's layers (basically it appears to stick it in the view's parent as a sibling of the view). I think that's the incorrect approach and likely why the shadow would get disconnected when we do animations.

Obviously multiple-value borderRadius has made this a giant pain in the butt compared to single value (since iOS has a nice cornerRadius property for that case).

Looking at our code, for a multi-radius value view likely need a layer hierarchy like this:

root layer (UIView.layer, masksToBounds = NO)
`-- shadowLayer (set shadow properties, fill color clear, path set to the borderRadius bezier path)
     `-- contentLayer (actual contents, mask set to the borderRadius bezier path)
         |-- borderLayer (for drawing custom border when radius is multi-value)
         |-- backgroundLayer (for background images)
         |-- gradientLayer (for gradients)

(Note I'm unclear on if it would make more sense to make those child of the content layer get nested rather than all be siblings?)

For no radius, single radius value, or case where we can use CACornerMask (only some corners get radius but must be same value) we should be able to simplify and use the cornerRadius property and not need the shadowLayer or borderLayer.

@sgtcoolguy sgtcoolguy dismissed their stale review September 8, 2020 16:03

we will open a new ticket to track edge case of animations with view having shadow and custom borderRadius with more than single value

@sgtcoolguy
Copy link
Contributor

OK, we discussed on call - @vijaysingh-axway will open a new ticket for the specific edge case of animating a view that has a shadow and borderRadius with multiple values. I believe the underlying issue is the layer hierarchy here and that a shadowLayer as a sublayer of the view root with a child content layer would be the ultimate fix for it. But we shouldn't hold up the fix here for the much more severe, obvious issue of single value borderRadius with shadows.

@ssekhri
Copy link

ssekhri commented Sep 8, 2020

FR Passed.
The view shadow shown with view having no border radius, single border radius or multiple radius defined.
Also borders of views shown properly with different border radius values. No truncation of borders.

There is an issue with animation of view having multiple border radius. This would be handled as a separate ticket https://jira.appcelerator.org/browse/TIMOB-28115.

Verified on:
Mac OS: 10.15.4
SDK: 9.2.0.v20200907172046
Appc CLI: 8.1.0
JDK: 11.0.4
Node: 10.17.0
Studio: 6.0.0.202005141803
Xcode: 12.0 Beta6
iPhone 7Plus(v14.0 Beta6), iOS simulator 14.0, 13.5

@build
Copy link
Contributor

build commented Sep 9, 2020

The backport to 9_3_X failed:

The process 'git' failed with exit code 128

Check the run for full details
To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Check out the target branch
git checkout 9_3_X
# Make sure it's up to date
git pull
# Check out your branch
git checkout -b backport-12005-to-9_3_X
# Apply the commits from the PR
curl -s https://github.com/appcelerator/titanium_mobile/commit/b1d6d7cc99c02148f810128e6f4258e99b311e1a.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/467a91685d86e1e0cc45205d5ae37f6f84d0cf51.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/559429ba770894cd5a16d0e91e7e829aac51b1aa.patch | git am -3 --ignore-whitespace
# Push it to GitHub
git push --set-upstream origin backport-12005-to-9_3_X

Then, create a pull request where the base branch is 9_3_X and the compare/head branch is backport-12005-to-9_3_X.

@sgtcoolguy
Copy link
Contributor

manually cherry-picked to 9_3_X

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants