Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22744003:
Fix LabelButtonBorder image blending during state transitions (Closed)
|
Created:
7 years, 4 months ago by bartfab (slow) Modified:
7 years, 4 months ago Reviewers:
msw CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionObsoleted by http://crrev.com/217629.
Fix LabelButtonBorder image blending during state transitions
When a button is transitioning between states, it should fade between its
background and foreground states. This can be done by painting the two
states into layers and performing a weighted additive blending of these
onto the |canvas|.
BUG=239121
TEST=Manual
Patch Set 1 #Patch Set 2 : One more compositing fix. #
Total comments: 4
Patch Set 3 : Nit addressed. #Messages
Total messages: 17 (0 generated)
Hi Mike, Could you take a look at this CL please? It is a simpler replacement for your CL which you put on hold a while ago: https://codereview.chromium.org/14921006/
I tried kPlusMode and it doesn't work quite right, if you slow the animation down a lot and look closely at the before/after and mid-animation states, you'll see that additive mode will get darker than expected and cause a flicker during animation. This is why we need to use SkLerpXfermode somehow. More importantly, this drawing code should be moved to NativeTheme (which won't use Views Painter), then the animation can be resolved there. I'll be out the rest of the day on vacation, sorry.
On 2013/08/09 18:08:44, msw wrote: > I tried kPlusMode and it doesn't work quite right, if you slow the animation > down a lot and look closely at the before/after and mid-animation states, you'll > see that additive mode will get darker than expected and cause a flicker during > animation. This is why we need to use SkLerpXfermode somehow. More importantly, > this drawing code should be moved to NativeTheme (which won't use Views > Painter), then the animation can be resolved there. I'll be out the rest of the > day on vacation, sorry. Interesting. For me, kPlusMode seems to work perfectly. I did slow down the animation and I did zoom the screen by 200%. There were no visual glitches whatsoever. I even dumped out the color of a representative pixel and saw that it transitioned through the enimation correctly. Since the code is not using NativeTheme yet, what is the short-term solution we can use to address this regression? Public sessions are a launched feature and the big, red logout button is a very visible part of the public session UI.
On 2013/08/09 18:22:31, bartfab wrote: > On 2013/08/09 18:08:44, msw wrote: > > I tried kPlusMode and it doesn't work quite right, if you slow the animation > > down a lot and look closely at the before/after and mid-animation states, > you'll > > see that additive mode will get darker than expected and cause a flicker > during > > animation. This is why we need to use SkLerpXfermode somehow. More > importantly, > > this drawing code should be moved to NativeTheme (which won't use Views > > Painter), then the animation can be resolved there. I'll be out the rest of > the > > day on vacation, sorry. > > Interesting. For me, kPlusMode seems to work perfectly. I did slow down the > animation and I did zoom the screen by 200%. There were no visual glitches > whatsoever. I even dumped out the color of a representative pixel and saw that > it transitioned through the enimation correctly. > > Since the code is not using NativeTheme yet, what is the short-term solution we > can use to address this regression? Public sessions are a launched feature and > the big, red logout button is a very visible part of the public session UI. Try it on both light and dark backgrounds, and clip the button to draw in thirds before/during/after states. Take a look back through http://crbug.com/239121 and the related changes to see what was done before and the problems I experienced. I'm on vacation today, but I can take a closer look on Monday.
On 2013/08/09 18:26:28, msw wrote: > On 2013/08/09 18:22:31, bartfab wrote: > > On 2013/08/09 18:08:44, msw wrote: > > > I tried kPlusMode and it doesn't work quite right, if you slow the animation > > > down a lot and look closely at the before/after and mid-animation states, > > you'll > > > see that additive mode will get darker than expected and cause a flicker > > during > > > animation. This is why we need to use SkLerpXfermode somehow. More > > importantly, > > > this drawing code should be moved to NativeTheme (which won't use Views > > > Painter), then the animation can be resolved there. I'll be out the rest of > > the > > > day on vacation, sorry. > > > > Interesting. For me, kPlusMode seems to work perfectly. I did slow down the > > animation and I did zoom the screen by 200%. There were no visual glitches > > whatsoever. I even dumped out the color of a representative pixel and saw that > > it transitioned through the enimation correctly. > > > > Since the code is not using NativeTheme yet, what is the short-term solution > we > > can use to address this regression? Public sessions are a launched feature and > > the big, red logout button is a very visible part of the public session UI. > > Try it on both light and dark backgrounds, and clip the button to draw in thirds > before/during/after states. > Take a look back through http://crbug.com/239121 and the related changes to see > what was done before and the problems I experienced. > I'm on vacation today, but I can take a closer look on Monday. You can also talk with Ben Wells and Mike Reed for more context and ideas.
On 2013/08/09 18:27:37, msw wrote: > On 2013/08/09 18:26:28, msw wrote: > > On 2013/08/09 18:22:31, bartfab wrote: > > > On 2013/08/09 18:08:44, msw wrote: > > > > I tried kPlusMode and it doesn't work quite right, if you slow the > animation > > > > down a lot and look closely at the before/after and mid-animation states, > > > you'll > > > > see that additive mode will get darker than expected and cause a flicker > > > during > > > > animation. This is why we need to use SkLerpXfermode somehow. More > > > importantly, > > > > this drawing code should be moved to NativeTheme (which won't use Views > > > > Painter), then the animation can be resolved there. I'll be out the rest > of > > > the > > > > day on vacation, sorry. > > > > > > Interesting. For me, kPlusMode seems to work perfectly. I did slow down the > > > animation and I did zoom the screen by 200%. There were no visual glitches > > > whatsoever. I even dumped out the color of a representative pixel and saw > that > > > it transitioned through the enimation correctly. > > > > > > Since the code is not using NativeTheme yet, what is the short-term solution > > we > > > can use to address this regression? Public sessions are a launched feature > and > > > the big, red logout button is a very visible part of the public session UI. > > > > Try it on both light and dark backgrounds, and clip the button to draw in > thirds > > before/during/after states. > > Take a look back through http://crbug.com/239121 and the related changes to > see > > what was done before and the problems I experienced. > > I'm on vacation today, but I can take a closer look on Monday. > > You can also talk with Ben Wells and Mike Reed for more context and ideas. The button I am dealing with falls into the category "light button on a dark background". With the code as-is, I can clearly see it darkening (background shining through) and then brightening again. With this CL, it looks fine to me. I will try on different background though. Since I am in EMEA, the week is over for me as well - let's continue our discussion on Monday.
On 2013/08/09 18:35:06, bartfab wrote: > On 2013/08/09 18:27:37, msw wrote: > > On 2013/08/09 18:26:28, msw wrote: > > > On 2013/08/09 18:22:31, bartfab wrote: > > > > On 2013/08/09 18:08:44, msw wrote: > > > > > I tried kPlusMode and it doesn't work quite right, if you slow the > > animation > > > > > down a lot and look closely at the before/after and mid-animation > states, > > > > you'll > > > > > see that additive mode will get darker than expected and cause a flicker > > > > during > > > > > animation. This is why we need to use SkLerpXfermode somehow. More > > > > importantly, > > > > > this drawing code should be moved to NativeTheme (which won't use Views > > > > > Painter), then the animation can be resolved there. I'll be out the rest > > of > > > > the > > > > > day on vacation, sorry. > > > > > > > > Interesting. For me, kPlusMode seems to work perfectly. I did slow down > the > > > > animation and I did zoom the screen by 200%. There were no visual glitches > > > > whatsoever. I even dumped out the color of a representative pixel and saw > > that > > > > it transitioned through the enimation correctly. > > > > > > > > Since the code is not using NativeTheme yet, what is the short-term > solution > > > we > > > > can use to address this regression? Public sessions are a launched feature > > and > > > > the big, red logout button is a very visible part of the public session > UI. > > > > > > Try it on both light and dark backgrounds, and clip the button to draw in > > thirds > > > before/during/after states. > > > Take a look back through http://crbug.com/239121 and the related changes to > > see > > > what was done before and the problems I experienced. > > > I'm on vacation today, but I can take a closer look on Monday. > > > > You can also talk with Ben Wells and Mike Reed for more context and ideas. > > The button I am dealing with falls into the category "light button on a dark > background". With the code as-is, I can clearly see it darkening (background > shining through) and then brightening again. With this CL, it looks fine to me. > I will try on different background though. Since I am in EMEA, the week is over > for me as well - let's continue our discussion on Monday. It was a while ago, but the problems I recall with using plus mode were to do with buttons which had a similar color to their background, e.g. pick a neutral grey, and blend a button of the same grey to another button of the same grey (e.g. if the text only is changing). That is, background and both buttons are the same neutral grey. There should be no change, but with plus mode it went darker first IIRC. Having said that, I remember being pretty frustrated about it as it seemed like plus mode should have been able to do what we needed ... maybe something has been fixed in the implementation since. Blue buttons don't animate at all as a result of this.
On 2013/08/12 00:15:36, benwells wrote: > On 2013/08/09 18:35:06, bartfab wrote: > > On 2013/08/09 18:27:37, msw wrote: > > > On 2013/08/09 18:26:28, msw wrote: > > > > On 2013/08/09 18:22:31, bartfab wrote: > > > > > On 2013/08/09 18:08:44, msw wrote: > > > > > > I tried kPlusMode and it doesn't work quite right, if you slow the > > > animation > > > > > > down a lot and look closely at the before/after and mid-animation > > states, > > > > > you'll > > > > > > see that additive mode will get darker than expected and cause a > flicker > > > > > during > > > > > > animation. This is why we need to use SkLerpXfermode somehow. More > > > > > importantly, > > > > > > this drawing code should be moved to NativeTheme (which won't use > Views > > > > > > Painter), then the animation can be resolved there. I'll be out the > rest > > > of > > > > > the > > > > > > day on vacation, sorry. > > > > > > > > > > Interesting. For me, kPlusMode seems to work perfectly. I did slow down > > the > > > > > animation and I did zoom the screen by 200%. There were no visual > glitches > > > > > whatsoever. I even dumped out the color of a representative pixel and > saw > > > that > > > > > it transitioned through the enimation correctly. > > > > > > > > > > Since the code is not using NativeTheme yet, what is the short-term > > solution > > > > we > > > > > can use to address this regression? Public sessions are a launched > feature > > > and > > > > > the big, red logout button is a very visible part of the public session > > UI. > > > > > > > > Try it on both light and dark backgrounds, and clip the button to draw in > > > thirds > > > > before/during/after states. > > > > Take a look back through http://crbug.com/239121 and the related changes > to > > > see > > > > what was done before and the problems I experienced. > > > > I'm on vacation today, but I can take a closer look on Monday. > > > > > > You can also talk with Ben Wells and Mike Reed for more context and ideas. > > > > The button I am dealing with falls into the category "light button on a dark > > background". With the code as-is, I can clearly see it darkening (background > > shining through) and then brightening again. With this CL, it looks fine to > me. > > I will try on different background though. Since I am in EMEA, the week is > over > > for me as well - let's continue our discussion on Monday. > > It was a while ago, but the problems I recall with using plus mode were to do > with buttons which had a similar color to their background, e.g. pick a neutral > grey, and blend a button of the same grey to another button of the same grey > (e.g. if the text only is changing). That is, background and both buttons are > the same neutral grey. There should be no change, but with plus mode it went > darker first IIRC. > > Having said that, I remember being pretty frustrated about it as it seemed like > plus mode should have been able to do what we needed ... maybe something has > been fixed in the implementation since. > > Blue buttons don't animate at all as a result of this. I experimented with light buttons on a light background and found that the blending has to be done in an intermediate layer for it to work correctly. My brain refuses to understand right now why that intermediate layer is needed. But either way, with the layer added, everything works 100% correctly for me, with any combination of light/dark buttons and light/dark backgrounds. I tried it in a chromeos=1 build, in a VM and on a Lumpy.
On 2013/08/12 11:16:11, bartfab wrote: > On 2013/08/12 00:15:36, benwells wrote: > > On 2013/08/09 18:35:06, bartfab wrote: > > > On 2013/08/09 18:27:37, msw wrote: > > > > On 2013/08/09 18:26:28, msw wrote: > > > > > On 2013/08/09 18:22:31, bartfab wrote: > > > > > > On 2013/08/09 18:08:44, msw wrote: > > > > > > > I tried kPlusMode and it doesn't work quite right, if you slow the > > > > animation > > > > > > > down a lot and look closely at the before/after and mid-animation > > > states, > > > > > > you'll > > > > > > > see that additive mode will get darker than expected and cause a > > flicker > > > > > > during > > > > > > > animation. This is why we need to use SkLerpXfermode somehow. More > > > > > > importantly, > > > > > > > this drawing code should be moved to NativeTheme (which won't use > > Views > > > > > > > Painter), then the animation can be resolved there. I'll be out the > > rest > > > > of > > > > > > the > > > > > > > day on vacation, sorry. > > > > > > > > > > > > Interesting. For me, kPlusMode seems to work perfectly. I did slow > down > > > the > > > > > > animation and I did zoom the screen by 200%. There were no visual > > glitches > > > > > > whatsoever. I even dumped out the color of a representative pixel and > > saw > > > > that > > > > > > it transitioned through the enimation correctly. > > > > > > > > > > > > Since the code is not using NativeTheme yet, what is the short-term > > > solution > > > > > we > > > > > > can use to address this regression? Public sessions are a launched > > feature > > > > and > > > > > > the big, red logout button is a very visible part of the public > session > > > UI. > > > > > > > > > > Try it on both light and dark backgrounds, and clip the button to draw > in > > > > thirds > > > > > before/during/after states. > > > > > Take a look back through http://crbug.com/239121 and the related changes > > to > > > > see > > > > > what was done before and the problems I experienced. > > > > > I'm on vacation today, but I can take a closer look on Monday. > > > > > > > > You can also talk with Ben Wells and Mike Reed for more context and ideas. > > > > > > The button I am dealing with falls into the category "light button on a dark > > > background". With the code as-is, I can clearly see it darkening (background > > > shining through) and then brightening again. With this CL, it looks fine to > > me. > > > I will try on different background though. Since I am in EMEA, the week is > > over > > > for me as well - let's continue our discussion on Monday. > > > > It was a while ago, but the problems I recall with using plus mode were to do > > with buttons which had a similar color to their background, e.g. pick a > neutral > > grey, and blend a button of the same grey to another button of the same grey > > (e.g. if the text only is changing). That is, background and both buttons are > > the same neutral grey. There should be no change, but with plus mode it went > > darker first IIRC. > > > > Having said that, I remember being pretty frustrated about it as it seemed > like > > plus mode should have been able to do what we needed ... maybe something has > > been fixed in the implementation since. > > > > Blue buttons don't animate at all as a result of this. > > I experimented with light buttons on a light background and found that the > blending has to be done in an intermediate layer for it to work correctly. My > brain refuses to understand right now why that intermediate layer is needed. But > either way, with the layer added, everything works 100% correctly for me, with > any combination of light/dark buttons and light/dark backgrounds. I tried it in > a chromeos=1 build, in a VM and on a Lumpy. Your code looks similar to things I tried, and I tried a lot of things. I remember thinking the plus mode didn't seem to be working as advertised. Maybe plus mode has been fixed, or maybe when I was trying it I didn't have quite the right magic layers in place. Can you test on Windows as well?
On 2013/08/12 11:32:03, benwells wrote: > On 2013/08/12 11:16:11, bartfab wrote: > > On 2013/08/12 00:15:36, benwells wrote: > > > On 2013/08/09 18:35:06, bartfab wrote: > > > > On 2013/08/09 18:27:37, msw wrote: > > > > > On 2013/08/09 18:26:28, msw wrote: > > > > > > On 2013/08/09 18:22:31, bartfab wrote: > > > > > > > On 2013/08/09 18:08:44, msw wrote: > > > > > > > > I tried kPlusMode and it doesn't work quite right, if you slow the > > > > > animation > > > > > > > > down a lot and look closely at the before/after and mid-animation > > > > states, > > > > > > > you'll > > > > > > > > see that additive mode will get darker than expected and cause a > > > flicker > > > > > > > during > > > > > > > > animation. This is why we need to use SkLerpXfermode somehow. More > > > > > > > importantly, > > > > > > > > this drawing code should be moved to NativeTheme (which won't use > > > Views > > > > > > > > Painter), then the animation can be resolved there. I'll be out > the > > > rest > > > > > of > > > > > > > the > > > > > > > > day on vacation, sorry. > > > > > > > > > > > > > > Interesting. For me, kPlusMode seems to work perfectly. I did slow > > down > > > > the > > > > > > > animation and I did zoom the screen by 200%. There were no visual > > > glitches > > > > > > > whatsoever. I even dumped out the color of a representative pixel > and > > > saw > > > > > that > > > > > > > it transitioned through the enimation correctly. > > > > > > > > > > > > > > Since the code is not using NativeTheme yet, what is the short-term > > > > solution > > > > > > we > > > > > > > can use to address this regression? Public sessions are a launched > > > feature > > > > > and > > > > > > > the big, red logout button is a very visible part of the public > > session > > > > UI. > > > > > > > > > > > > Try it on both light and dark backgrounds, and clip the button to draw > > in > > > > > thirds > > > > > > before/during/after states. > > > > > > Take a look back through http://crbug.com/239121 and the related > changes > > > to > > > > > see > > > > > > what was done before and the problems I experienced. > > > > > > I'm on vacation today, but I can take a closer look on Monday. > > > > > > > > > > You can also talk with Ben Wells and Mike Reed for more context and > ideas. > > > > > > > > The button I am dealing with falls into the category "light button on a > dark > > > > background". With the code as-is, I can clearly see it darkening > (background > > > > shining through) and then brightening again. With this CL, it looks fine > to > > > me. > > > > I will try on different background though. Since I am in EMEA, the week is > > > over > > > > for me as well - let's continue our discussion on Monday. > > > > > > It was a while ago, but the problems I recall with using plus mode were to > do > > > with buttons which had a similar color to their background, e.g. pick a > > neutral > > > grey, and blend a button of the same grey to another button of the same grey > > > (e.g. if the text only is changing). That is, background and both buttons > are > > > the same neutral grey. There should be no change, but with plus mode it went > > > darker first IIRC. > > > > > > Having said that, I remember being pretty frustrated about it as it seemed > > like > > > plus mode should have been able to do what we needed ... maybe something has > > > been fixed in the implementation since. > > > > > > Blue buttons don't animate at all as a result of this. > > > > I experimented with light buttons on a light background and found that the > > blending has to be done in an intermediate layer for it to work correctly. My > > brain refuses to understand right now why that intermediate layer is needed. > But > > either way, with the layer added, everything works 100% correctly for me, with > > any combination of light/dark buttons and light/dark backgrounds. I tried it > in > > a chromeos=1 build, in a VM and on a Lumpy. > > Your code looks similar to things I tried, and I tried a lot of things. I > remember thinking the plus mode didn't seem to be working as advertised. > > Maybe plus mode has been fixed, or maybe when I was trying it I didn't have > quite the right magic layers in place. Can you test on Windows as well? I can spin a Windows build but since I use Windows very rarely, I am not sure what exactly to test for. Where would I find a button that uses this code? Do I need any special flags to test this?
On 2013/08/12 11:38:05, bartfab wrote: > On 2013/08/12 11:32:03, benwells wrote: > > On 2013/08/12 11:16:11, bartfab wrote: > > > On 2013/08/12 00:15:36, benwells wrote: > > > > On 2013/08/09 18:35:06, bartfab wrote: > > > > > On 2013/08/09 18:27:37, msw wrote: > > > > > > On 2013/08/09 18:26:28, msw wrote: > > > > > > > On 2013/08/09 18:22:31, bartfab wrote: > > > > > > > > On 2013/08/09 18:08:44, msw wrote: > > > > > > > > > I tried kPlusMode and it doesn't work quite right, if you slow > the > > > > > > animation > > > > > > > > > down a lot and look closely at the before/after and > mid-animation > > > > > states, > > > > > > > > you'll > > > > > > > > > see that additive mode will get darker than expected and cause a > > > > flicker > > > > > > > > during > > > > > > > > > animation. This is why we need to use SkLerpXfermode somehow. > More > > > > > > > > importantly, > > > > > > > > > this drawing code should be moved to NativeTheme (which won't > use > > > > Views > > > > > > > > > Painter), then the animation can be resolved there. I'll be out > > the > > > > rest > > > > > > of > > > > > > > > the > > > > > > > > > day on vacation, sorry. > > > > > > > > > > > > > > > > Interesting. For me, kPlusMode seems to work perfectly. I did slow > > > down > > > > > the > > > > > > > > animation and I did zoom the screen by 200%. There were no visual > > > > glitches > > > > > > > > whatsoever. I even dumped out the color of a representative pixel > > and > > > > saw > > > > > > that > > > > > > > > it transitioned through the enimation correctly. > > > > > > > > > > > > > > > > Since the code is not using NativeTheme yet, what is the > short-term > > > > > solution > > > > > > > we > > > > > > > > can use to address this regression? Public sessions are a launched > > > > feature > > > > > > and > > > > > > > > the big, red logout button is a very visible part of the public > > > session > > > > > UI. > > > > > > > > > > > > > > Try it on both light and dark backgrounds, and clip the button to > draw > > > in > > > > > > thirds > > > > > > > before/during/after states. > > > > > > > Take a look back through http://crbug.com/239121 and the related > > changes > > > > to > > > > > > see > > > > > > > what was done before and the problems I experienced. > > > > > > > I'm on vacation today, but I can take a closer look on Monday. > > > > > > > > > > > > You can also talk with Ben Wells and Mike Reed for more context and > > ideas. > > > > > > > > > > The button I am dealing with falls into the category "light button on a > > dark > > > > > background". With the code as-is, I can clearly see it darkening > > (background > > > > > shining through) and then brightening again. With this CL, it looks fine > > to > > > > me. > > > > > I will try on different background though. Since I am in EMEA, the week > is > > > > over > > > > > for me as well - let's continue our discussion on Monday. > > > > > > > > It was a while ago, but the problems I recall with using plus mode were to > > do > > > > with buttons which had a similar color to their background, e.g. pick a > > > neutral > > > > grey, and blend a button of the same grey to another button of the same > grey > > > > (e.g. if the text only is changing). That is, background and both buttons > > are > > > > the same neutral grey. There should be no change, but with plus mode it > went > > > > darker first IIRC. > > > > > > > > Having said that, I remember being pretty frustrated about it as it seemed > > > like > > > > plus mode should have been able to do what we needed ... maybe something > has > > > > been fixed in the implementation since. > > > > > > > > Blue buttons don't animate at all as a result of this. > > > > > > I experimented with light buttons on a light background and found that the > > > blending has to be done in an intermediate layer for it to work correctly. > My > > > brain refuses to understand right now why that intermediate layer is needed. > > But > > > either way, with the layer added, everything works 100% correctly for me, > with > > > any combination of light/dark buttons and light/dark backgrounds. I tried it > > in > > > a chromeos=1 build, in a VM and on a Lumpy. > > > > Your code looks similar to things I tried, and I tried a lot of things. I > > remember thinking the plus mode didn't seem to be working as advertised. > > > > Maybe plus mode has been fixed, or maybe when I was trying it I didn't have > > quite the right magic layers in place. Can you test on Windows as well? > > I can spin a Windows build but since I use Windows very rarely, I am not sure > what exactly to test for. Where would I find a button that uses this code? Do I > need any special flags to test this? I tried to get a Windows build going but after 8 hour, gclient sync is still not done. I will see whether it finishes overnight. But I fear that my Windows environment may be broken and I may be unable to create a Windows build.
On 2013/08/12 17:18:36, bartfab wrote: > On 2013/08/12 11:38:05, bartfab wrote: > > On 2013/08/12 11:32:03, benwells wrote: > > > On 2013/08/12 11:16:11, bartfab wrote: > > > > On 2013/08/12 00:15:36, benwells wrote: > > > > > On 2013/08/09 18:35:06, bartfab wrote: > > > > > > On 2013/08/09 18:27:37, msw wrote: > > > > > > > On 2013/08/09 18:26:28, msw wrote: > > > > > > > > On 2013/08/09 18:22:31, bartfab wrote: > > > > > > > > > On 2013/08/09 18:08:44, msw wrote: > > > > > > > > > > I tried kPlusMode and it doesn't work quite right, if you slow > > the > > > > > > > animation > > > > > > > > > > down a lot and look closely at the before/after and > > mid-animation > > > > > > states, > > > > > > > > > you'll > > > > > > > > > > see that additive mode will get darker than expected and cause > a > > > > > flicker > > > > > > > > > during > > > > > > > > > > animation. This is why we need to use SkLerpXfermode somehow. > > More > > > > > > > > > importantly, > > > > > > > > > > this drawing code should be moved to NativeTheme (which won't > > use > > > > > Views > > > > > > > > > > Painter), then the animation can be resolved there. I'll be > out > > > the > > > > > rest > > > > > > > of > > > > > > > > > the > > > > > > > > > > day on vacation, sorry. > > > > > > > > > > > > > > > > > > Interesting. For me, kPlusMode seems to work perfectly. I did > slow > > > > down > > > > > > the > > > > > > > > > animation and I did zoom the screen by 200%. There were no > visual > > > > > glitches > > > > > > > > > whatsoever. I even dumped out the color of a representative > pixel > > > and > > > > > saw > > > > > > > that > > > > > > > > > it transitioned through the enimation correctly. > > > > > > > > > > > > > > > > > > Since the code is not using NativeTheme yet, what is the > > short-term > > > > > > solution > > > > > > > > we > > > > > > > > > can use to address this regression? Public sessions are a > launched > > > > > feature > > > > > > > and > > > > > > > > > the big, red logout button is a very visible part of the public > > > > session > > > > > > UI. > > > > > > > > > > > > > > > > Try it on both light and dark backgrounds, and clip the button to > > draw > > > > in > > > > > > > thirds > > > > > > > > before/during/after states. > > > > > > > > Take a look back through http://crbug.com/239121 and the related > > > changes > > > > > to > > > > > > > see > > > > > > > > what was done before and the problems I experienced. > > > > > > > > I'm on vacation today, but I can take a closer look on Monday. > > > > > > > > > > > > > > You can also talk with Ben Wells and Mike Reed for more context and > > > ideas. > > > > > > > > > > > > The button I am dealing with falls into the category "light button on > a > > > dark > > > > > > background". With the code as-is, I can clearly see it darkening > > > (background > > > > > > shining through) and then brightening again. With this CL, it looks > fine > > > to > > > > > me. > > > > > > I will try on different background though. Since I am in EMEA, the > week > > is > > > > > over > > > > > > for me as well - let's continue our discussion on Monday. > > > > > > > > > > It was a while ago, but the problems I recall with using plus mode were > to > > > do > > > > > with buttons which had a similar color to their background, e.g. pick a > > > > neutral > > > > > grey, and blend a button of the same grey to another button of the same > > grey > > > > > (e.g. if the text only is changing). That is, background and both > buttons > > > are > > > > > the same neutral grey. There should be no change, but with plus mode it > > went > > > > > darker first IIRC. > > > > > > > > > > Having said that, I remember being pretty frustrated about it as it > seemed > > > > like > > > > > plus mode should have been able to do what we needed ... maybe something > > has > > > > > been fixed in the implementation since. > > > > > > > > > > Blue buttons don't animate at all as a result of this. > > > > > > > > I experimented with light buttons on a light background and found that the > > > > blending has to be done in an intermediate layer for it to work correctly. > > My > > > > brain refuses to understand right now why that intermediate layer is > needed. > > > But > > > > either way, with the layer added, everything works 100% correctly for me, > > with > > > > any combination of light/dark buttons and light/dark backgrounds. I tried > it > > > in > > > > a chromeos=1 build, in a VM and on a Lumpy. > > > > > > Your code looks similar to things I tried, and I tried a lot of things. I > > > remember thinking the plus mode didn't seem to be working as advertised. > > > > > > Maybe plus mode has been fixed, or maybe when I was trying it I didn't have > > > quite the right magic layers in place. Can you test on Windows as well? > > > > I can spin a Windows build but since I use Windows very rarely, I am not sure > > what exactly to test for. Where would I find a button that uses this code? Do > I > > need any special flags to test this? > > I tried to get a Windows build going but after 8 hour, gclient sync is still not > done. I will see whether it finishes overnight. But I fear that my Windows > environment may be broken and I may be unable to create a Windows build. I finally got a Windows build with my CL. It works perfectly for me. I looked at the bookmark popup bubble, both with and without --enable-new-dialog-style. The buttons have a very subtle hover effect that fades in and out without any border or background darkening.
My local testing (clipping the button into 10 vertical strips with a gradient of alpha values and testing various hardcoded animation states of the regular and blue button over light/dark backgrounds) all looked good, but your CL inspired me to implement this more correctly. Please consider my WIP CL using SkLerpXfermode instead: https://codereview.chromium.org/14921006/ https://chromiumcodereview.appspot.com/22744003/diff/10001/ui/views/controls/... File ui/views/controls/button/label_button_border.cc (right): https://chromiumcodereview.appspot.com/22744003/diff/10001/ui/views/controls/... ui/views/controls/button/label_button_border.cc:127: canvas->sk_canvas()->saveLayer(NULL, NULL); nit: I would suggest using canvas->Save() since you're not setting an SkPaint here, but that seemed to break the rendering for me... so you can leave this as-is, I guess. https://chromiumcodereview.appspot.com/22744003/diff/10001/ui/views/controls/... ui/views/controls/button/label_button_border.cc:136: canvas->Restore(); nit: Though use of sk_canvas is discouraged, I think I'd prefer that the canvas->sk_canvas()->saveLayer calls were instead paired with canvas->sk_canvas()->restore() calls (not canvas->Restore()).
I updated the CL to address the nits. Mike, I had seen your CL but you seemed to have put it on hold indefinitely 2 months ago. If you want to revive your CL and land that instead, that SGTM. Otherwise, I think this CL is a good solution, even if only in the interim. https://chromiumcodereview.appspot.com/22744003/diff/10001/ui/views/controls/... File ui/views/controls/button/label_button_border.cc (right): https://chromiumcodereview.appspot.com/22744003/diff/10001/ui/views/controls/... ui/views/controls/button/label_button_border.cc:127: canvas->sk_canvas()->saveLayer(NULL, NULL); On 2013/08/13 21:43:20, msw wrote: > nit: I would suggest using canvas->Save() since you're not setting an SkPaint > here, but that seemed to break the rendering for me... so you can leave this > as-is, I guess. I had tried canvas->Save() first but it broke rendering (the same thing you observed). Hence, I see no other option than canvas->sk_canvas()->saveLayer(NULL, NULL). https://chromiumcodereview.appspot.com/22744003/diff/10001/ui/views/controls/... ui/views/controls/button/label_button_border.cc:136: canvas->Restore(); On 2013/08/13 21:43:20, msw wrote: > nit: Though use of sk_canvas is discouraged, I think I'd prefer that the > canvas->sk_canvas()->saveLayer calls were instead paired with > canvas->sk_canvas()->restore() calls (not canvas->Restore()). Done.
On 2013/08/14 08:28:01, bartfab wrote: > Mike, I had seen your CL but you seemed to have put it on hold indefinitely 2 > months ago. If you want to revive your CL and land that instead, that SGTM. > Otherwise, I think this CL is a good solution, even if only in the interim. Sorry, I linked to the wrong CL, here's a new one: https://codereview.chromium.org/23058003/ This uses SkLerpXfermode similar to your approach. Mike Reed just posted some comments, I'll address them today.
On 2013/08/14 14:53:44, msw wrote: > On 2013/08/14 08:28:01, bartfab wrote: > > Mike, I had seen your CL but you seemed to have put it on hold indefinitely 2 > > months ago. If you want to revive your CL and land that instead, that SGTM. > > Otherwise, I think this CL is a good solution, even if only in the interim. > > Sorry, I linked to the wrong CL, here's a new one: > https://codereview.chromium.org/23058003/ > This uses SkLerpXfermode similar to your approach. > Mike Reed just posted some comments, I'll address them today. I landed http://crrev.com/217629 I hope that works for your case.
On 2013/08/15 18:18:15, msw wrote: > On 2013/08/14 14:53:44, msw wrote: > > On 2013/08/14 08:28:01, bartfab wrote: > > > Mike, I had seen your CL but you seemed to have put it on hold indefinitely > 2 > > > months ago. If you want to revive your CL and land that instead, that SGTM. > > > Otherwise, I think this CL is a good solution, even if only in the interim. > > > > Sorry, I linked to the wrong CL, here's a new one: > > https://codereview.chromium.org/23058003/ > > This uses SkLerpXfermode similar to your approach. > > Mike Reed just posted some comments, I'll address them today. > > I landed http://crrev.com/217629 I hope that works for your case. Thanks! I will close this issue. |
