Skip to content

fix(android): box shadow and border radius white border resolution #10125

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 16 commits into from
Mar 17, 2023

Conversation

vallemar
Copy link
Contributor

@vallemar vallemar commented Dec 7, 2022

When you have a dark screen and you apply box-shadow and border radius you see a rather strange white border. Applying this change solves it. I haven't seen any difference on a clear screen, so I think it doesn't break anything

Old:
Screenshot_2022-12-07-13-49-01-283-edit_org nativescript abllapp

Fix:
Screenshot_2022-12-07-13-47-43-779-edit_org nativescript abllapp

@nx-cloud
Copy link

nx-cloud bot commented Dec 7, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit dc3e10c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@cla-bot cla-bot bot added the cla: yes label Dec 7, 2022
@vallemar vallemar changed the title Box shadow and border radius fix(android): Box shadow and border radius Dec 7, 2022
@vallemar vallemar changed the title fix(android): Box shadow and border radius fix(android): Box shadow and border radius, white strange border Dec 7, 2022
@rigor789
Copy link
Member

rigor789 commented Dec 7, 2022

Mentioned on Discord already, just adding here for visibility:

The WHITE rect layer is added to mimic the web behavior:
image

So setting it to TRANSPARENT is redundant, as we might as well not even add the overlay layer in that case. A proposed solution is to check whether the view already has a background, and only in case it doesn't add the white rect overlay, otherwise skip adding the overlay layer at all.

@rigor789 rigor789 marked this pull request as draft December 7, 2022 14:41
@vallemar
Copy link
Contributor Author

vallemar commented Dec 7, 2022

@rigor789 I had misunderstood you, I thought it was just the passing of the color, now the layer will not be added if it already has a background

@vallemar
Copy link
Contributor Author

vallemar commented Dec 27, 2022

@NathanWalker @rigor789 can we review this pr?


// add our layers
this.addLayer(shadowLayer);
this.addLayer(overlayLayer);
if(!((BorderDrawable) this.wrappedLayer).hasBackgroundColor()){
Copy link
Member

Choose a reason for hiding this comment

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

This might be unsafe, this.wrappedLayer might not always be a BorderDrawable, so casting to it can lead to exceptions. A safer approach would be to check the type of wrappedLayer.

@vallemar
Copy link
Contributor Author

@rigor789 @NathanWalker I have updated the PR with the type check

@NathanWalker NathanWalker marked this pull request as ready for review March 17, 2023 16:50
rigor789
rigor789 previously approved these changes Mar 17, 2023
Copy link
Member

@rigor789 rigor789 left a comment

Choose a reason for hiding this comment

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

Looks good to me, just left some small nits about the method naming. 👍

rigor789
rigor789 previously approved these changes Mar 17, 2023
Copy link
Contributor

@NathanWalker NathanWalker left a comment

Choose a reason for hiding this comment

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

This PR is invalid at this point but if fixed we could include with 8.5 still.

ns-android-box-shadow-pr-invalid.mov

@rigor789
Copy link
Member

Simplified the logic to hide the overlay layer when the wrapped drawable has a background color, image or gradient. The above issue was caused by changing the order of the addLayer calls mainly. Might still try not creating the layer at all - but the order of layers important.

@rigor789
Copy link
Member

Refactored to not create the layer as originally intended - but preserving the order of layers now correctly.

@vallemar
Copy link
Contributor Author

@rigor789 oh sorry, I thought the order wouldn't matter and put it in an order that I thought was more readable

@NathanWalker NathanWalker changed the title fix(android): Box shadow and border radius, white strange border fix(android): box shadow and border radius white border resolution Mar 17, 2023
@NathanWalker NathanWalker merged commit ea45758 into NativeScript:main Mar 17, 2023
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.

3 participants