[WebPayments] Blue buttons render as black on Mac |
||||||||||
Issue descriptionSometime after M60 branch, trunk builds started showing blue buttons as black. So far only observed on Mac with secondary-ui-md off.
,
Jun 13 2017
ellyjones@, mind taking a look? Any chance this is related to your recent a11y changes?
,
Jun 13 2017
This is because of non-Harmony MacViews. I believe that in NativeThemeMac::GetSystemColor() we'll fall back to the case for kColorId_ProminentButtonColor, which returns NSSystemColorToSkColor([NSColor controlTextColor]), which is probably black. That said, I haven't made any recent changes to this. As far as I can tell, this was introduced by afakhry@'s <https://codereview.chromium.org/2922263002>, which made it so that on Mac, MdTextButton::CreateSecondaryUiButton() will always create an MD button even when --secondary-ui-md is not active. Creating an MD button while --secondary-ui-md is not actually enabled will produce a bizarre look like this. I think that part of afakhry@'s CL must be a left-over from debugging or something. +cc afakhry@: did you intend to always use MD buttons in MacViews? (Is the profile error dialog using MacViews currently? If it is, the MacViews team is not aware of that and there are very significant caveats you should be aware of!) +cc tmartino@
,
Jun 13 2017
+tapted@ No this wasn't a left-over from debugging. This was intended as we believed that IsSecondaryUiMaterial() should always be true for tooklit-views on Mac. tapted@ mentioned on the CL that "web payments may launch a views dialog on Mac before we launch Harmony. So, IsSecondaryUiMaterial() may be false in that case". I believe this bug is what he was referring to.
,
Jun 13 2017
Aha, okay :) Well, in that case, we should probably undo that specific part of that CL.
,
Jun 13 2017
Re: c#1, you can invoke the dialog from, e.g., https://paymentrequest.show/demo/
,
Jun 14 2017
Hm. [NSColor controlTextColor] doesn't seem right for kColorId_ProminentButtonColor (regardless of MD settings)... but I'm not sure AppKit offers up the right color via a public API. The closest might be something like
case kColorId_ProminentButtonColor:
return NSSystemColorToSkColor([NSColor colorForControlTint:[NSColor currentControlTint]]);
But that's really washed out.
I think
// NSColor doesn't offer a color for prominent buttons. Use the Aura color,
// but apply the system tint.
case kColorId_ProminentButtonColor:
return ApplySystemControlTint(GetAuraColor(color_id, this));
,
Jun 14 2017
For reference, here's the discussion between Trent and me that led to us trying to do this: <pkasting> I wonder if we should just hack CreateSecondaryUiBlueButton() to always use MdTextButton #if OS_MACOSX. That would mean that web payments would launch a Harmony-style button early, instead of launching BlueButton early. That seems better. WDYT? <tapted> We'd probably want to tweak CreateSecondaryUiButton() as well, so the buttons are consistent. But I think it would work. <tapted> There's already precedent for MdTextButton outside of Harmony (and once you have an MdTextButton its behaviour shouldn't change with/without harmony), so I don't think this would expose too many obscure codepaths. So basically, this was an intentional behavior change for web payments, but we thought it would actually work correctly -- you'd get a Harmony-style button here. Apparently it doesn't work as well as we thought it would.
,
Jun 14 2017
And also for reference, the reason we even tried to do things this way, instead of just launching BlueButton early on Mac, is because the change here broke unittests on Mac when we did that, and it didn't seem worth the trouble to find and fix the issues. See further discussion on the original CL review comments, it goes on for some time.
,
Jun 15 2017
In original bug report, tmartino mentions "Sometime after M60 branch, trunk builds started showing blue buttons as black", so I'm guessing this should be affecting M61+ only. Please adjust if necessary.
,
Jun 15 2017
,
Jun 21 2017
https://codereview.chromium.org/2948083002/ is my suggestion from #c7
,
Jun 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2fad1a80b82ed7df3a3967192a07d8f55e1bab0a commit 2fad1a80b82ed7df3a3967192a07d8f55e1bab0a Author: tapted <tapted@chromium.org> Date: Fri Jun 23 00:27:28 2017 MacViews: Use the Aura color for prominent button backgrounds. Do this even without --secondary-ui-md, since we blacklisted non-MD dialog buttons for secondary UI in r478352. NSColor doesn't offer a color for prominent buttons. Use the Aura color, but apply the system tint. This is a good match for the blue Cocoa uses to draw buttons that are given a \n key equivalent. BUG= 732889 Review-Url: https://codereview.chromium.org/2948083002 Cr-Commit-Position: refs/heads/master@{#481744} [modify] https://crrev.com/2fad1a80b82ed7df3a3967192a07d8f55e1bab0a/ui/native_theme/native_theme_mac.mm
,
Jun 26 2017
A couple of differences remain: - textfield corners are round with --secondary-ui-md and square without - some text colors change. E.g. Some grey values are probably too light with --secondary-ui-md This bug is fixed though.
,
Jun 27 2017
Tested the issue on Latest Dev# 61.0.3141.0 using Mac OS X 10.12.5 and observed the label background in blue whether secondary-ui-md is enabled/disabled. Attached a screenshot for reference. @tapted -- Could you please look into the issue and confirm whether this is the required behavior. Thank You.
,
Jun 27 2017
,
Jun 28 2017
732889.png is probably the wrong screenshot. But yes, on Mac the label background should be in blue whether secondary-ui-md is enabled/disabled. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by sdy@chromium.org
, Jun 13 2017