Skip to content
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

Buttons get Brave style #3569

Merged
merged 2 commits into from
Oct 15, 2019
Merged

Buttons get Brave style #3569

merged 2 commits into from
Oct 15, 2019

Conversation

petemill
Copy link
Member

@petemill petemill commented Oct 1, 2019

Uplifts should include:

Fix brave/brave-browser#6398

Native buttons - creates brave style
WebUI buttons - perfects existing brave style to be consistent with latest design iteration that native buttons now get

Buttons have 2 types: regular and prominent (native) or action (webui).
Each button has styles for 4 states: regular, hover, focus, active and active states.
Each state controls: text color, background color, ink highlight color, ink color, focus ring color, border color, and shadows.

That native button strategy is to replace the Create functions which are used to create MdTextButtons across the browser Views UI code and have those functions instead return BraveTextButtons. This class inherits from MdTextButton.
Unfortunately the definitions for colors seems to be split between a UI-common function (GetSystemColor) and hard-coded within the button class.

The WebUI strategy is improved by relying on css variables for code simplification: the variables change for different states, but the css properties applying those variables stay the same, and therefore css is only applied once and not overriden multiple times for different states.

Testing

The following should be the same buttons styles for WebUI and Native. The only difference is that WebUI buttons get a slightly better shadow effect when pressed ...because css is better ;-) (but really because AFAIK it would require a lot of new native code to get the shadow like this whilst still using a lot of the existing chromium ink drop and other button features).

  • Native buttons can both be seen by visiting an installable web app site like maps.google.com, clicking the (+) icon in the Location Bar. There'll be Install and Cancel buttons on the bubble dialog.
  • WebUI buttons can both be seen by visiting the Settings page and clicking 'Import bookmarks and settings' at the top. A dialog with both buttons will be show.

Prominent (light and dark mode)

image

Hover

image

Pressed

image

Regular (light mode)

image

Hover

image

Pressed (on Windows gets a ink bubble animation)

image

Regular (dark mode)

image

Hover

image

Pressed (on Windows gets a ink bubble animation)

image

Focus (same for all styles)

image

Submitter Checklist:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@petemill petemill self-assigned this Oct 1, 2019
@petemill petemill force-pushed the native-button-styles branch 2 times, most recently from b23d737 to 67f5810 Compare October 2, 2019 23:55
@bsclifton
Copy link
Member

@petemill per the triage meeting, can you create an issue for this? (capturing what areas you're looking at updating)

@petemill petemill force-pushed the native-button-styles branch 2 times, most recently from e6a3e10 to be9dc0a Compare October 8, 2019 23:06
@petemill petemill changed the title Native Buttons get Brave style Buttons get Brave style Oct 8, 2019
@petemill petemill marked this pull request as ready for review October 8, 2019 23:13
@petemill
Copy link
Member Author

petemill commented Oct 9, 2019

This is working and ready on macOS, but I'm getting an unknown immediate crash on Windows. Will see how the CI does, and do a Debug build locally in the mean time.

bsclifton
bsclifton previously approved these changes Oct 10, 2019
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Just gave this a go; works great! I used in both light and dark mode, went through all the different pages and situations I could think of (another good UI to try for seeing native components is clicking the lock and going to the cookie manager OR adding a new bookmark folder to toolbar)

Code changes also look good to me!


// Insert our own definition for `CreateXYZ`, and (in concert with the .cc)
// move chromium's definition to `CreateXYZ_ChromiumImpl`
#define CreateSecondaryUiButton CreateSecondaryUiButton_ChromiumImpl( \
Copy link
Member

Choose a reason for hiding this comment

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

Wow!! great idea! 👍

simonhong
simonhong previously approved these changes Oct 11, 2019
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

LGTM

@petemill petemill dismissed stale reviews from simonhong and bsclifton via cedd28d October 11, 2019 05:10
@petemill petemill force-pushed the native-button-styles branch 2 times, most recently from cedd28d to fc13588 Compare October 11, 2019 05:42
@petemill
Copy link
Member Author

Pushed a rebase on c78

simonhong
simonhong previously approved these changes Oct 11, 2019
@petemill petemill force-pushed the native-button-styles branch 5 times, most recently from 4a19b7e to a25ec60 Compare October 11, 2019 22:38
@petemill
Copy link
Member Author

I've pushed a fix for the issue that the Buttons would first paint as square, and then paint as rounded when re-painted (due to hover or focus, etc).

@petemill
Copy link
Member Author

Not sure why CI lint and build failed yet again, even after a rebase. Just trying again...

@petemill
Copy link
Member Author

CI seems to have failed due to a single browser test in unrelated code

15:57:00  ../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2572: Failure
15:57:00  Expected equality of these values:
15:57:00    ac_reconcile_status_
15:57:00      Which is: Result::LEDGER_ERROR
15:57:00    ledger::Result::LEDGER_OK
15:57:00      Which is: Result::LEDGER_OK
15:57:00  Stack trace:

@bsclifton
Copy link
Member

hmm... I just pulled latest and noticed a few irregularities (macOS):
Screen Shot 2019-10-14 at 12 58 26 PM

Screen Shot 2019-10-14 at 12 58 50 PM

Screen Shot 2019-10-14 at 12 59 20 PM

I did do a sync + then an incremental build. Can you double check this, @petemill?

@petemill
Copy link
Member Author

Thanks @bsclifton. A last minute change of where the radius was applied didn't get adequate testing. The fix was pushed. https://github.com/brave/brave-core/compare/7cee53ecf4ec18662eee5f8b9109f746dfa5adda..54e40cea0457dac3c6d635915442a88bd3745e24

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Retested after last change- looks good! 😄

@petemill
Copy link
Member Author

Build seems 'unstable', not failed, and seems to have errored at the intermittent rewards test case.

@bsclifton
Copy link
Member

@petemill looks like you're good to go- the test that failed is intermittent:
brave/brave-browser#6468

cc: @NejcZdovc
https://staging.ci.brave.com/job/brave-browser-build-pr/job/native-button-styles/15/execution/node/658/log/

@bsclifton bsclifton merged commit 22a8916 into master Oct 15, 2019
@bsclifton bsclifton deleted the native-button-styles branch October 15, 2019 07:13
@petemill petemill added this to the 0.72.x - Dev milestone Oct 20, 2019
petemill pushed a commit that referenced this pull request Oct 20, 2019
bsclifton added a commit that referenced this pull request Oct 31, 2019
bsclifton added a commit that referenced this pull request Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Native buttons should use brave brand style
3 participants