Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

Conversation

clshortfuse
Copy link
Contributor

@clshortfuse clshortfuse commented Aug 29, 2016

Webkit has a rendering bug with child elements whos parent uses a
rounded corner. In this case, this is a ripple on a rounded button.
To circumvent this issue, we applied a 1x1 PNG image mask on the
ripple element.

The image mask is causing issues with Chrome rendering opaque under
certain conditions. An image mask is only needed for Safari 8, which
is no longer supported. Instead we will just Safari and Chrome to
use a compositing layer by forcing an empty Z-axis translation.

  • Inherit border radius from buttons
  • Remove image mask CSS
  • Use translateZ to force webkit to create a compositing layer

Fixes #9154, #10086

References:

https://bugs.webkit.org/show_bug.cgi?id=30475

@topherfangio
Copy link
Contributor

Can you add a bit more detail to the commit message about why the old hack was there and why this is an improvement?

Otherwise LGTM 👍

@clshortfuse clshortfuse force-pushed the button-fixRippleCorner branch 2 times, most recently from 86976ac to df35c54 Compare August 29, 2016 19:49
@ThomasBurleson ThomasBurleson added this to the 1.1.2 milestone Aug 30, 2016
@topherfangio topherfangio added the needs: review This PR is waiting on review from the team label Aug 31, 2016
@topherfangio
Copy link
Contributor

@ThomasBurleson Ready for your review. Carlos and I have both done manual testing on various browsers.

@ThomasBurleson ThomasBurleson added needs: manual testing This issue or PR needs to have some manual testing and verification done in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed needs: review This PR is waiting on review from the team labels Sep 7, 2016
@clshortfuse
Copy link
Contributor Author

clshortfuse commented Sep 8, 2016

I've done a bit more research on the issue. There is a webkit issue with one element bleeding through a parent element (ripple bleeding through corner). The previous fix created used a 1x1 pixel mask-image which worked, but in reality wasn't really doing anything to the actual rendering. What was going on, is that in webkit, certain operations on elements, will create their own backing surface (aka a compositing layer).

When you add a mask to an image, the browser will break up the elements into layers and overlay a mask layer over the element. Technically, yes, we are going to force 3D, but for the express objective of making sure the element has a compositing layer, eliminating the overlay bleed issue.

There are many ways to force a composition layer, and transform3D is just another one, that's much safer than applying a mask.

References:
Chrome Design Doc on GPU - See From RenderLayers to GraphicsLayers
List of Compositing Reasons in Chrome
Webkit overview of rendering and compositing, referencing masks as compositing
Webkit source code showing Masks use GraphicsLayer (compositing layer)

@devversion ping, you brought up concerns. Basically, before it was 3D + mask layer. Now it's just 3D (no mask needed)

@devversion
Copy link
Member

@clshortfuse This sounds valid. LGTM then!

@clshortfuse clshortfuse force-pushed the button-fixRippleCorner branch 2 times, most recently from c786f94 to 3c1ec60 Compare September 11, 2016 20:31
@clshortfuse
Copy link
Contributor Author

Well, well, well. Seems like it was right to retest all this stuff with Browserstack. Safari 8 needs both compositing and image mask, not just compositing. (Safari 9 and Chrome are okay with just transformZ).

Sure, -webkit-transform:translateZ(0) is 14 characters less in CSS, but this way, we can support Safari 8 as well.

I've rewritten the code and commit, using this codepen for reviewing: http://codepen.io/shortfuse/full/dpYQaz

@clshortfuse clshortfuse added needs: review This PR is waiting on review from the team and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: manual testing This issue or PR needs to have some manual testing and verification done labels Sep 11, 2016
@@ -170,11 +156,13 @@ button.md-button::-moz-focus-inner {
}

.md-ripple-container {
border-radius: $button-border-radius;
border-radius: inherit;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we inherit? we want it to always be 50%

Copy link
Contributor Author

Choose a reason for hiding this comment

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

border-radius isn't the shape of the ripple, it's the border limit to which the ripple will reach on its container. See #9154

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if they use md-cornered, then it shouldn't be 50%, it should be 0.

@EladBezalel EladBezalel added the needs: manual testing This issue or PR needs to have some manual testing and verification done label Sep 11, 2016
@ThomasBurleson ThomasBurleson modified the milestones: 1.1.2, - Backlog Nov 29, 2016
@clshortfuse clshortfuse force-pushed the button-fixRippleCorner branch from cf42675 to c80dbb1 Compare December 15, 2016 18:12
@clshortfuse
Copy link
Contributor Author

I reverted back to using translateZ because the image mask is causing issues with Chrome. We don't need it the image mask anymore since that was only for Safari 8.

@clshortfuse clshortfuse removed the needs: manual testing This issue or PR needs to have some manual testing and verification done label Dec 15, 2016
@marshall007
Copy link

@clshortfuse what's the status on this? Your patch is working great for us in Chrome. Do we just need some manual testing in other browsers?

@ThomasBurleson ThomasBurleson added needs: presubmit and removed needs: review This PR is waiting on review from the team labels May 8, 2017
@ThomasBurleson ThomasBurleson modified the milestones: 1.1.5, - Backlog May 8, 2017
@ThomasBurleson ThomasBurleson modified the milestones: 1.1.5, 1.1.6 Jun 12, 2017
@Splaktar Splaktar modified the milestones: 1.1.6, 1.1.7 Jan 17, 2018
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM

@Splaktar Splaktar added hotlist: animations browser: Safari browser: Chrome P4: minor Minor issues. May not be fixed without community contributions. pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer labels Feb 7, 2018
@Splaktar Splaktar self-assigned this Feb 7, 2018
@Splaktar Splaktar modified the milestones: 1.1.7, 1.1.8 Feb 8, 2018
@mmalerba mmalerba merged commit 0f15e39 into angular:master Feb 9, 2018
chmelevskij pushed a commit to chmelevskij/material that referenced this pull request Jun 19, 2018
…gular#9449)

Webkit has a rendering bug with child elements whos parent uses a
rounded corner. In this case, this is a ripple on a rounded button.
To circumvent this issue, we applied a 1x1 PNG image mask on the
ripple element.

The image mask is causing issues with Chrome rendering opaque under
certain conditions. An image mask is only needed for Safari 8, which
is no longer supported. Instead we will just Safari and Chrome to
use a compositing layer by forcing an empty Z-axis translation.

* Inherit border radius from buttons
* Remove image mask CSS
* Use translateZ to force webkit to create a compositing layer

Fixes angular#9154, angular#10086

References:

https://bugs.webkit.org/show_bug.cgi?id=30475
Splaktar pushed a commit that referenced this pull request Jul 31, 2018
)

Webkit has a rendering bug with child elements whos parent uses a
rounded corner. In this case, this is a ripple on a rounded button.
To circumvent this issue, we applied a 1x1 PNG image mask on the
ripple element.

The image mask is causing issues with Chrome rendering opaque under
certain conditions. An image mask is only needed for Safari 8, which
is no longer supported. Instead we will just Safari and Chrome to
use a compositing layer by forcing an empty Z-axis translation.

* Inherit border radius from buttons
* Remove image mask CSS
* Use translateZ to force webkit to create a compositing layer

Fixes #9154, #10086

References:

https://bugs.webkit.org/show_bug.cgi?id=30475
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
browser: Chrome browser: Safari cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ hotlist: animations P4: minor Minor issues. May not be fixed without community contributions. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review ui: CSS ui: ink
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants