Skip to content

fix: Apply proper border radius to box shadow and view sublayers #9881

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

Merged
merged 4 commits into from
Apr 30, 2022

Conversation

CatchABus
Copy link
Contributor

@CatchABus CatchABus commented Apr 26, 2022

PR Checklist

What is the current behavior?

In iOS:

  • Border radius is miscalculated for drop-shadow
  • Gradient background border radius is miscalculated when there is drop-shadow

I found those problems while using an iPad. Box-shadow radius was probably miscalculated because of a scale factor that may be higher than 1.
D1C14C66-80AC-4594-A7ED-AC2BA2CE741E1FC2E34A-D732-4460-A13B-E4B3265F4F83

What is the new behavior?

Border radius will be properly calculated for view box-shadow and sublayers. It seems that toDeviceIndependentPixels works better and returns the same value as the rest of applied view border-radiuses.
This change will also attempt to apply corner radius to all sublayers. Sublayers will usually consist of a gradient layer and a border layer.
There are also cases that border layer has sublayers but there is a need for a whole refactor to make those look good, that is why we apply radius to top-level layers for now.

This is new shadow radius with gradient background:
2DD8A8F8-9BCF-4DB4-AB60-8F1D323198BB_1_101_o

Fixes/Closes #9869 .

@cla-bot cla-bot bot added the cla: yes label Apr 26, 2022
@NathanWalker NathanWalker changed the base branch from master to alpha April 30, 2022 17:58
@NathanWalker NathanWalker merged commit 3d882b0 into NativeScript:alpha Apr 30, 2022
@CatchABus
Copy link
Contributor Author

CatchABus commented May 4, 2022

@NathanWalker Whenever possible, I believe we should revert this one as it's unsafe.
There is the case that corner radius will cause unexpected results if children are added to view "before" view shadow is applied for some reason, as children layers will be appended to parent sublayers.
I have thought of a good way to solve box shadow problems and maintain a good performance at the same time.

@farfromrefug
Copy link
Collaborator

@CatchABus if of any interest to you I had done this in another cross platform framework Titanium. I can fins the code if you need

@CatchABus
Copy link
Contributor Author

@CatchABus if of any interest to you I had done this in another cross platform framework Titanium. I can fins the code if you need

Sure! It could be a good addition!

Note: I tend to think that using main layer with masksToBounds and clipsToBounds for shadows will keep causing trouble in the future. Maybe we should also consider the use of a sublayer for shadows as one solution?

@farfromrefug
Copy link
Collaborator

farfromrefug commented May 5, 2022

@CatchABus it s all here https://github.com/Akylas/titanium_mobile/blob/master/iphone/Classes/TiUIView.m#L610= . If you have any question ping me on discord.
BTW a lot of things can be taken from there. I had animation working for almost anything!

@rigor789
Copy link
Member

rigor789 commented May 5, 2022

Note: I tend to think that using main layer with masksToBounds and clipsToBounds for shadows will keep causing trouble in the future. Maybe we should also consider the use of a sublayer for shadows as one solution?

I agree - I was working on the initial box-shadow implementation and had tinkered with the idea of using proper sublayers for shadows - allowing stacking multiple shadows etc, however there was a major roadblock (fixable I would imagine, but still was major)...

Given an ios view, you can add as many subviews as you want, however you cannot move them behind the parent view, only above it (at least if I remember correctly, been a while...) - which meant we couldn't easily add a shadow layer to existing views and move them behind the view. The approach I was playing around with was adding an extra parent view to hold the original view + the shadow layers, but managing that was kinda tricky as core View handling is set up to handle the root view and not anything above.

Wrapping every view in a extra view seemed like a bad solution, as it would double the number of views rendered even if most of them don't render a shadow.

Ideally the following structure could work:

  View
    -> nativeView (UIView or whatever)
  View with shadow
    -> decorView
       -> nativeView (UIView or whatever)
       -> shadowLayer1
       -> shadowLayer2
       -> etc.

@CatchABus
Copy link
Contributor Author

Note: I tend to think that using main layer with masksToBounds and clipsToBounds for shadows will keep causing trouble in the future. Maybe we should also consider the use of a sublayer for shadows as one solution?

I agree - I was working on the initial box-shadow implementation and had tinkered with the idea of using proper sublayers for shadows - allowing stacking multiple shadows etc, however there was a major roadblock (fixable I would imagine, but still was major)...

Given an ios view, you can add as many subviews as you want, however you cannot move them behind the parent view, only above it (at least if I remember correctly, been a while...) - which meant we couldn't easily add a shadow layer to existing views and move them behind the view. The approach I was playing around with was adding an extra parent view to hold the original view + the shadow layers, but managing that was kinda tricky as core View handling is set up to handle the root view and not anything above.

Wrapping every view in a extra view seemed like a bad solution, as it would double the number of views rendered even if most of them don't render a shadow.

Ideally the following structure could work:

  View
    -> nativeView (UIView or whatever)
  View with shadow
    -> decorView
       -> nativeView (UIView or whatever)
       -> shadowLayer1
       -> shadowLayer2
       -> etc.

Perhaps, this issue gets solved using insertSublayerAtIndex or setting layer zPosition to negative value (default is 0 for all layers).
I'm using those in my neumorphism shadow plugin.

@rigor789
Copy link
Member

rigor789 commented May 5, 2022

Note: I tend to think that using main layer with masksToBounds and clipsToBounds for shadows will keep causing trouble in the future. Maybe we should also consider the use of a sublayer for shadows as one solution?

I agree - I was working on the initial box-shadow implementation and had tinkered with the idea of using proper sublayers for shadows - allowing stacking multiple shadows etc, however there was a major roadblock (fixable I would imagine, but still was major)...
Given an ios view, you can add as many subviews as you want, however you cannot move them behind the parent view, only above it (at least if I remember correctly, been a while...) - which meant we couldn't easily add a shadow layer to existing views and move them behind the view. The approach I was playing around with was adding an extra parent view to hold the original view + the shadow layers, but managing that was kinda tricky as core View handling is set up to handle the root view and not anything above.
Wrapping every view in a extra view seemed like a bad solution, as it would double the number of views rendered even if most of them don't render a shadow.
Ideally the following structure could work:

  View
    -> nativeView (UIView or whatever)
  View with shadow
    -> decorView
       -> nativeView (UIView or whatever)
       -> shadowLayer1
       -> shadowLayer2
       -> etc.

Perhaps, this issue gets solved using insertSublayerAtIndex or setting layer zPosition to negative value (default is 0 for all layers). I'm using those in my neumorphism shadow plugin.

I'm not sure - I'm pretty sure I tried every possible combination, and could not move my shadow layer below the actual view - inserting it at various indices or zPositions, was always above the view. But if that works, that'd be perfect 👍

@CatchABus
Copy link
Contributor Author

@rigor789 I'm not sure if I understood correctly but I have this plugin for neumorphism styling: https://github.com/CatchABus/nativescript-plugins/tree/master/packages/ui-neumorphiclayout
It uses 2 sublayers because support for 2 different shadows is needed. So far, I have nested children inside those views without problems.
Maybe, this concept could serve for adding multiple shadows support to NS.

@rigor789
Copy link
Member

rigor789 commented May 5, 2022

@CatchABus I just remembered - I never deleted the tests I did here (they are ignored, as we return early):

console.log(`--- ${create ? 'CREATE' : 'READ'}`);
/**
* UIView
* -> Shadow
*
*
* UIView
* -> UIView
* -> Shadow
*/
if (!nativeView) {
return null;
}
if (!nativeView.layer) {
// should never hit this?
console.log('- no layer! -');
return null;
}
// if the nativeView's layer is the shadow layer?
if (nativeView.layer.name === name) {
console.log('- found shadow layer - reusing.');
return nativeView.layer;
}
console.log('>> layer :', nativeView.layer);
if (nativeView.layer.sublayers?.count) {
const count = nativeView.layer.sublayers.count;
for (let i = 0; i < count; i++) {
const subLayer = nativeView.layer.sublayers.objectAtIndex(i);
console.log(`>> subLayer ${i + 1}/${count} :`, subLayer);
console.log(`>> subLayer ${i + 1}/${count} name :`, subLayer.name);
if (subLayer.name === name) {
console.log('- found shadow sublayer - reusing.');
return subLayer;
}
}
// if (nativeView instanceof UITextView) {
// return nativeView.layer.sublayers.objectAtIndex(1);
// } else {
// return nativeView.layer.sublayers.objectAtIndex(nativeView.layer.sublayers.count - 1);
// }
}
// else {
// layer = nativeView.layer;
// }
// we're not interested in creating a new layer
if (!create) {
return null;
}
console.log(`- adding a new layer for - ${name}`);
const viewLayer = nativeView.layer;
const newLayer = CALayer.layer();
newLayer.name = name;
newLayer.zPosition = 0.0;
// nativeView.layer.insertSublayerBelow(newLayer, nativeView.layer)
// newLayer.insertSublayerAtIndex(nativeView.layer, 0)
// nativeView.layer.zPosition = 1.0;
// nativeView.layer.addSublayer(newLayer);
// nativeView.layer = CALayer.layer()
nativeView.layer.insertSublayerAtIndex(newLayer, 0);
// nativeView.layer.insertSublayerAtIndex(viewLayer, 1)
// nativeView.layer.replaceSublayerWith(newLayer, nativeView.layer);
return newLayer;
}

removing the early return yields the following:
Screenshot 2022-05-05 at 8 06 58 PM

With some logging - here are the resulting layers:

Screenshot 2022-05-05 at 8 08 38 PM

Haven't had a closer look at your plugin, or what it does differently - but if you want you can try the above:

  • run the toolbox app -> box-shadow -> toggle solid background -> apply shadow
  • comment out the return in the native-helper.ios.ts and let the test logic run
  • make any adjustments in there to see if there's a way to move that shadow layer below the actual view...

I guess the difference is that in your plugin you have a fillColor property - and not the regular backgroundColor/backgroundImage I guess, so you have more control of the drawing there?

@CatchABus
Copy link
Contributor Author

CatchABus commented May 7, 2022

@CatchABus I just remembered - I never deleted the tests I did here (they are ignored, as we return early):

console.log(`--- ${create ? 'CREATE' : 'READ'}`);
/**
* UIView
* -> Shadow
*
*
* UIView
* -> UIView
* -> Shadow
*/
if (!nativeView) {
return null;
}
if (!nativeView.layer) {
// should never hit this?
console.log('- no layer! -');
return null;
}
// if the nativeView's layer is the shadow layer?
if (nativeView.layer.name === name) {
console.log('- found shadow layer - reusing.');
return nativeView.layer;
}
console.log('>> layer :', nativeView.layer);
if (nativeView.layer.sublayers?.count) {
const count = nativeView.layer.sublayers.count;
for (let i = 0; i < count; i++) {
const subLayer = nativeView.layer.sublayers.objectAtIndex(i);
console.log(`>> subLayer ${i + 1}/${count} :`, subLayer);
console.log(`>> subLayer ${i + 1}/${count} name :`, subLayer.name);
if (subLayer.name === name) {
console.log('- found shadow sublayer - reusing.');
return subLayer;
}
}
// if (nativeView instanceof UITextView) {
// return nativeView.layer.sublayers.objectAtIndex(1);
// } else {
// return nativeView.layer.sublayers.objectAtIndex(nativeView.layer.sublayers.count - 1);
// }
}
// else {
// layer = nativeView.layer;
// }
// we're not interested in creating a new layer
if (!create) {
return null;
}
console.log(`- adding a new layer for - ${name}`);
const viewLayer = nativeView.layer;
const newLayer = CALayer.layer();
newLayer.name = name;
newLayer.zPosition = 0.0;
// nativeView.layer.insertSublayerBelow(newLayer, nativeView.layer)
// newLayer.insertSublayerAtIndex(nativeView.layer, 0)
// nativeView.layer.zPosition = 1.0;
// nativeView.layer.addSublayer(newLayer);
// nativeView.layer = CALayer.layer()
nativeView.layer.insertSublayerAtIndex(newLayer, 0);
// nativeView.layer.insertSublayerAtIndex(viewLayer, 1)
// nativeView.layer.replaceSublayerWith(newLayer, nativeView.layer);
return newLayer;
}

removing the early return yields the following: Screenshot 2022-05-05 at 8 06 58 PM

With some logging - here are the resulting layers:

Screenshot 2022-05-05 at 8 08 38 PM

Haven't had a closer look at your plugin, or what it does differently - but if you want you can try the above:

  • run the toolbox app -> box-shadow -> toggle solid background -> apply shadow
  • comment out the return in the native-helper.ios.ts and let the test logic run
  • make any adjustments in there to see if there's a way to move that shadow layer below the actual view...

I guess the difference is that in your plugin you have a fillColor property - and not the regular backgroundColor/backgroundImage I guess, so you have more control of the drawing there?

I see, I get it now! To solve this, we can apply view background color to shadow layer.
After all, it's only the color that's on the bottom.
Screenshot 2022-05-07 at 5 24 52 PM

Seeing where this goes, perhaps we should stop setting background color to native views directly and have a backgroundLayer that replaces shadow and border layers functionality, and background color would also apply to it. It could be used for multiple purposes.

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

Successfully merging this pull request may close these issues.

[Box shadow iOS] combining linear-gradient background, border-radius and box-shadow
4 participants