New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 732889 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

[WebPayments] Blue buttons render as black on Mac

Project Member Reported by tmartino@chromium.org, Jun 13 2017

Issue description

Sometime after M60 branch, trunk builds started showing blue buttons as black. So far only observed on Mac with secondary-ui-md off.
 
Screen Shot 2017-06-13 at 2.32.03 PM.png
83.0 KB View Download

Comment 1 by sdy@chromium.org, Jun 13 2017

What's the best way to trigger this dialog?

Comment 2 by sdy@chromium.org, Jun 13 2017

Labels: -Pri-3 M-60 Pri-2
Owner: ellyjo...@chromium.org
Status: Assigned (was: Untriaged)
ellyjones@, mind taking a look? Any chance this is related to your recent a11y changes?
Cc: afakhry@chromium.org tmartino@chromium.org
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@
Cc: pkasting@chromium.org tapted@chromium.org
+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.

Aha, okay :) Well, in that case, we should probably undo that specific part of that CL.
Re: c#1, you can invoke the dialog from, e.g., https://paymentrequest.show/demo/

Comment 7 by tapted@chromium.org, 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));
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.
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.

Comment 10 by ma...@chromium.org, Jun 15 2017

Labels: -M-60 M-61
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.
Cc: zkoch@chromium.org
Cc: ellyjo...@chromium.org
Owner: tapted@chromium.org
Status: Started (was: Assigned)
https://codereview.chromium.org/2948083002/ is my suggestion from #c7
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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.
Cc: msrchandra@chromium.org
Labels: Needs-Feedback
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.
732889.png
110 KB View Download
Components: -UI>Browser>Autofill>Payments UI>Browser>Payments
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