Add horizontal margin to LocationBar, centering on Toolbar where possible #454

petemill · 2018-09-14T23:01:17Z

Also, anchor BookmarkButton to floating LocationBar

Whilst a target margin is determined according to window's width and a maximum LocationBar width, such as 3%, 5%, 7% or more consider we determined that it's a nicer effect if the LocationBar is centered on the Toolbar, and not within the space between items on the left and right side of the Toolbar, which can be varying widths, especially with many extensions. Therefore space is allocated from the right margin to the left in order to 'shim' the center point of the LocationBar towards the center point of the Toolbar. Similarly space can also be allocated from the left margin to the right for the same purpose, but not so much that it encroaches more than 25% in to the right margin, so not to get too close to extension Browser Actions.

LocationBar max width is 1080px which is calculated from Brave 0.23's 900px width and increased with appropriate space for the Brave Actions.

Note that I experimented with 3 algorithms for calculating how much space to allocate on each side before settling on the following. These explorations are documented here: https://projects.invisionapp.com/boards/GD3PV3FKN4V/

Screenshots

With 4 extensions

Without extensions

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.
Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Test Plan:

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

petemill · 2018-09-17T04:35:18Z

Now added new boolean preference 'Use wide location bar', defaulting to false.

petemill · 2018-09-17T04:41:49Z

patches/chrome-browser-extensions-api-settings_private-prefs_util.cc.patch

@@ -1,15 +0,0 @@
-diff --git a/chrome/browser/extensions/api/settings_private/prefs_util.cc b/chrome/browser/extensions/api/settings_private/prefs_util.cc


These pref keys are being whitelisted from the javascript settingsPrivate API. Moved from patch to override, anything extra need testing?

petemill · 2018-09-17T04:57:32Z

browser/resources/settings/brave_appearance_page/brave_appearance_page.html

@@ -6,7 +6,7 @@
 <link rel="import" href="../settings_shared_css.html">
 <link rel="import" href="../settings_vars_css.html">

-<dom-module id="settings-brave-appearance-page">
+<dom-module id="settings-brave-appearance-theme">


Changes since we want to insert items to two separate places within the chromium list

petemill · 2018-09-17T04:59:18Z

browser/ui/views/toolbar/brave_toolbar_view.cc

-  }
-  next_element_x += GetLayoutConstant(TOOLBAR_STANDARD_SPACING);
-
+  next_element_x += element_padding;


we now always want element_padding distance between all items, even LocationBar, instead of GetLayoutConstant(TOOLBAR_STANDARD_SPACING) for the spacing to the left of LocationBar

browser/ui/views/toolbar/brave_toolbar_view.cc

simonhong · 2018-09-17T05:32:51Z

browser/ui/views/toolbar/brave_toolbar_view.cc

+                                                            (toolbar_width / 2);
+    // Can't shim more than we have space for, so restrict to margin size
+    // or in the case of moving-right, 25% of the space since we want to avoid
+    // touching browser actions where possible


I can't understand this comment about why right-side moving is restricted to 25% of margin.
There are always gap between location bar and browser action area because browser actions has it's internal padding.

This is to prevent the gap being larger on the left side than the right side, reducing the extra margin (the gap that does not include the left padding on the browser actions area). Whilst we aim to center the LocationBar as much as possible, a more important priority is to not be in the situation where there is some gap on the left, and no gap on the right, due to the proximity of the brave actions bar and the browser actions bar.

Of course, when there is no extra margin (either user-configurable or because the window is too narrow), then we are ok with no extra gap on either side.

Without shim-right restriction (moves right, towards center):

With 100% shim-right restriction (not allowed a smaller gap on right than left):

With 25% shim-right restriction (can move right towards center, but not more than 25% of the right-margin):

(Apologies for the discrepancy of the bookmark button position, some screenshots were taken before this moved!)

However, we still allow shim-left to 0% of the margin, moving all margin to the right:

Thanks for explanation! I thought location bar view should be center on toolbar always.

emerick

LGTM, a couple of minor comments

emerick · 2018-09-17T22:04:49Z

browser/extensions/api/settings_private/brave_prefs_util.cc

+
+const PrefsUtil::TypedPrefMap& BravePrefsUtil::GetWhitelistedKeys() {
+  // Static cache, similar to parent class
+  static PrefsUtil::TypedPrefMap* s_brave_whitelist = nullptr;


Do we want s_brave_whitelist in a std::unique_ptr or something like that? Or does it not matter for this?

Discussed in PM - followed parent class pattern of using static variable that hangs around for the whole browser process life, seems ok since this is a global whitelist.

browser/ui/views/toolbar/brave_toolbar_view.cc

…ible Can be disabled via user preference under Appearance section of settings page. Takes the opportunity to create a Brave whitelist for the settingsPrivate javascript API without adding items through patching chromium directly. This is achieved via subclassing `extensions::PrefsUtil`. Whilst a target margin is determined according to window's width and a maximum LocationBar width, such as 3%, 5%, 7% or more consider we determined that it's a nicer effect if the LocationBar is centered on the Toolbar, and not within the space between items on the left and right side of the Toolbar, which can be varying widths, especially with many extensions. Therefore space is allocated from the right margin to the left in order to 'shim' the center point of the LocationBar towards the center point of the Toolbar. Similarly space can also be allocated from the left margin to the right for the same purpose, but not so much that it encroaches more than 25% in to the right margin, so not to get too close to extension Browser Actions. BookmarkButton is then anchored to the floating LocationBar. LocationBar max width is 1080px which is calculated from Brave 0.23's 900px width and increased with appropriate space for the Brave Actions.

simonhong

LGTM2!

simonhong · 2018-09-17T12:15:47Z

browser/extensions/api/settings_private/brave_prefs_util.cc

+  const auto chromium_prefs = PrefsUtil::GetWhitelistedKeys();
+  s_brave_whitelist->insert(chromium_prefs.begin(), chromium_prefs.end());
+  // Add Brave values to the whitelist
+#if !defined(OS_CHROMEOS)


nit: this define can be removed

simonhong · 2018-09-17T22:30:41Z

browser/ui/views/toolbar/brave_toolbar_view.cc

+                                                            (toolbar_width / 2);
+    // Can't shim more than we have space for, so restrict to margin size
+    // or in the case of moving-right, 25% of the space since we want to avoid
+    // touching browser actions where possible


Thanks for explanation! I thought location bar view should be center on toolbar always.

browser/ui/views/toolbar/brave_toolbar_view.cc

Add horizontal margin to LocationBar, centering on Toolbar where possible

petemill · 2018-09-18T00:21:32Z

0.55.x d27d1db

petemill added the UI label Sep 14, 2018

petemill self-assigned this Sep 14, 2018

petemill requested review from bsclifton and simonhong September 14, 2018 23:01

petemill force-pushed the ui/locationbar-centering branch 2 times, most recently from 85c2df1 to 4361221 Compare September 17, 2018 04:33

petemill force-pushed the ui/locationbar-centering branch from 4361221 to a62f253 Compare September 17, 2018 04:35

petemill mentioned this pull request Sep 17, 2018

Add default Brave shields #436

Merged

11 tasks

petemill commented Sep 17, 2018

View reviewed changes

petemill force-pushed the ui/locationbar-centering branch from a62f253 to 90fe311 Compare September 17, 2018 05:14

simonhong reviewed Sep 17, 2018

View reviewed changes

petemill force-pushed the ui/locationbar-centering branch from 90fe311 to ea12792 Compare September 17, 2018 18:27

emerick previously approved these changes Sep 17, 2018

View reviewed changes

petemill dismissed emerick’s stale review via 5ac363f September 17, 2018 22:31

petemill force-pushed the ui/locationbar-centering branch from ea12792 to 5ac363f Compare September 17, 2018 22:31

simonhong approved these changes Sep 17, 2018

View reviewed changes

petemill merged commit 54ab499 into master Sep 18, 2018

petemill added a commit that referenced this pull request Sep 18, 2018

Merge pull request #454 from brave/ui/locationbar-centering

d27d1db

Add horizontal margin to LocationBar, centering on Toolbar where possible

petemill deleted the ui/locationbar-centering branch September 18, 2018 00:21

petemill added the merged/0.55.x label Sep 18, 2018

bbondy added this to the 0.55.x - Release milestone Jan 14, 2019

This was referenced May 16, 2023

Remove hyphen from vpn os entry name #18466

Merged

Fix windows vpn entry write failure #18510

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add horizontal margin to LocationBar, centering on Toolbar where possible #454

Add horizontal margin to LocationBar, centering on Toolbar where possible #454

petemill commented Sep 14, 2018 •

edited

Loading

petemill commented Sep 17, 2018

petemill Sep 17, 2018

petemill Sep 17, 2018

petemill Sep 17, 2018

simonhong Sep 17, 2018

petemill Sep 17, 2018 •

edited

Loading

simonhong Sep 17, 2018

emerick left a comment

emerick Sep 17, 2018

petemill Sep 17, 2018

simonhong left a comment

simonhong Sep 17, 2018

simonhong Sep 17, 2018

petemill commented Sep 18, 2018

		@@ -1,15 +0,0 @@
		diff --git a/chrome/browser/extensions/api/settings_private/prefs_util.cc b/chrome/browser/extensions/api/settings_private/prefs_util.cc

Add horizontal margin to LocationBar, centering on Toolbar where possible #454

Add horizontal margin to LocationBar, centering on Toolbar where possible #454

Conversation

petemill commented Sep 14, 2018 • edited Loading

Screenshots

With 4 extensions

Without extensions

Submitter Checklist:

Test Plan:

Reviewer Checklist:

petemill commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emerick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Sep 18, 2018

petemill commented Sep 14, 2018 •

edited

Loading

petemill Sep 17, 2018 •

edited

Loading