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

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

Merged
merged 1 commit into from
Sep 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions app/brave_generated_resources.grd
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ By installing this extension, you are agreeing to the Google Widevine Terms of U
<message name="IDS_SETTINGS_APPEARANCE_SETTINGS_BRAVE_THEMES" desc="The label for brave theme change setting options">
Brave colors
</message>
<message name="IDS_SETTINGS_APPEARANCE_SETTINGS_LOCATION_BAR_IS_WIDE" desc="The label for whether the location bar should display wide or not">
Use wide location bar
</message>
<!-- Tor -->
<message name="IDS_NEW_TOR_IDENTITY" desc="The text label of a menu item for requesting new Tor identity">
New Tor Identity
Expand Down
2 changes: 2 additions & 0 deletions browser/brave_profile_prefs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {

RegisterAlternatePrivateSearchEngineProfilePrefs(registry);

// appearance
BraveThemeService::RegisterProfilePrefs(registry);
registry->RegisterBooleanPref(kLocationBarIsWide, false);

tor::TorProfileService::RegisterProfilePrefs(registry);

Expand Down
2 changes: 2 additions & 0 deletions browser/extensions/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ source_set("extensions") {
"api/brave_shields_api.h",
"api/content_settings/brave_content_settings_store.cc",
"api/content_settings/brave_content_settings_store.h",
"api/settings_private/brave_prefs_util.cc",
"api/settings_private/brave_prefs_util.h",
"brave_component_extension.cc",
"brave_component_extension.h",
"brave_component_extension_resource_manager.cc",
Expand Down
39 changes: 39 additions & 0 deletions browser/extensions/api/settings_private/brave_prefs_util.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "brave/browser/extensions/api/settings_private/brave_prefs_util.h"

#include "brave/common/pref_names.h"
#include "chrome/browser/extensions/api/settings_private/prefs_util.h"
#include "chrome/common/pref_names.h"
#include "chrome/common/extensions/api/settings_private.h"

namespace extensions{

namespace settings_api = api::settings_private;

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if (s_brave_whitelist)
return *s_brave_whitelist;
s_brave_whitelist = new PrefsUtil::TypedPrefMap();
// Start with parent class whitelist
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)
Copy link
Member

Choose a reason for hiding this comment

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

nit: this define can be removed

// import data
(*s_brave_whitelist)[::prefs::kImportDialogCookies] =
settings_api::PrefType::PREF_TYPE_BOOLEAN;
(*s_brave_whitelist)[::prefs::kImportDialogStats] =
settings_api::PrefType::PREF_TYPE_BOOLEAN;
#endif
// appearance prefs
(*s_brave_whitelist)[kLocationBarIsWide] =
settings_api::PrefType::PREF_TYPE_BOOLEAN;
return *s_brave_whitelist;
}

}
22 changes: 22 additions & 0 deletions browser/extensions/api/settings_private/brave_prefs_util.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#ifndef BRAVE_BROWSER_EXTENSIONS_API_SETTINGS_PRIVATE_BRAVE_PREFS_UTIL_H
#define BRAVE_BROWSER_EXTENSIONS_API_SETTINGS_PRIVATE_BRAVE_PREFS_UTIL_H

#include "chrome/browser/extensions/api/settings_private/prefs_util.h"
namespace extensions{

class BravePrefsUtil : public PrefsUtil {
public:
using PrefsUtil::PrefsUtil;
// Gets the list of whitelisted pref keys -- that is, those which correspond
// to prefs that clients of the settingsPrivate API may retrieve and
// manipulate.
const PrefsUtil::TypedPrefMap& GetWhitelistedKeys() override;
};

}

#endif
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Copy link
Member Author

Choose a reason for hiding this comment

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

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

<template>
<style include="settings-shared md-select iron-flex">
</style>
Expand All @@ -25,3 +25,15 @@
</template>
<script src="brave_appearance_page.js"></script>
</dom-module>

<dom-module id="settings-brave-appearance-toolbar">
<template>
<style include="settings-shared md-select iron-flex">
</style>
<settings-toggle-button
pref="{{prefs.brave.location_bar_is_wide}}"
label="$i18n{appearanceSettingsLocationBarIsWide}"
>
</settings-toggle-button>
</template>
</dom-module>
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,8 @@
(function() {
'use strict';

/**
* 'settings-brave-appearance-page' is the settings page containing brave's
* appearance settings.
*/
Polymer({
is: 'settings-brave-appearance-page',
is: 'settings-brave-appearance-theme',

properties: {
braveThemeTypes_: {
Expand Down Expand Up @@ -54,4 +50,12 @@ Polymer({
this.browserProxy_.setBraveThemeType(this.$.braveThemeType.value);
},
});
})();

/**
* 'settings-brave-appearance-toolbar' is the settings page area containing
* brave's appearance settings related to the toolbar.
*/
Polymer({
is: 'settings-brave-appearance-toolbar',
});
})();
144 changes: 124 additions & 20 deletions browser/ui/views/toolbar/brave_toolbar_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "brave/browser/ui/views/toolbar/brave_toolbar_view.h"

#include "brave/browser/ui/views/toolbar/bookmark_button.h"
#include "brave/common/pref_names.h"
#include "chrome/browser/defaults.h"
#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
#include "chrome/browser/extensions/extension_util.h"
Expand Down Expand Up @@ -43,7 +44,11 @@ void BraveToolbarView::Init() {
bookmarks::prefs::kEditBookmarksEnabled, browser()->profile()->GetPrefs(),
base::Bind(&BraveToolbarView::OnEditBookmarksEnabledChanged,
base::Unretained(this)));

// track changes in wide locationbar setting
location_bar_is_wide_.Init(
kLocationBarIsWide, browser()->profile()->GetPrefs(),
base::Bind(&BraveToolbarView::OnLocationBarIsWideChanged,
base::Unretained(this)));
// Only location bar in non-normal mode
if (!is_display_mode_normal()) {
return;
Expand All @@ -59,6 +64,11 @@ void BraveToolbarView::OnEditBookmarksEnabledChanged() {
Update(nullptr);
}

void BraveToolbarView::OnLocationBarIsWideChanged() {
Layout();
SchedulePaint();
}

void BraveToolbarView::Update(content::WebContents* tab) {
ToolbarView::Update(tab);
// Decide whether to show the bookmark button
Expand Down Expand Up @@ -89,6 +99,88 @@ void BraveToolbarView::ShowBookmarkBubble(
star_view->OnBubbleWidgetCreated(bubble_widget);
}

// Returns right-margin
int BraveToolbarView::SetLocationBarBounds(const int available_width,
const int location_bar_min_width,
const int next_element_x,
const int element_padding) {
DCHECK(initialized_);
DCHECK(is_display_mode_normal());
// Allow the option of the LocationBar having horizontal margin.
// With this option, we support a dynamic percentage of margin, with
// a maximum width.
const bool restrict_location_bar_width = !location_bar_is_wide_.GetValue();
const int location_bar_max_width = restrict_location_bar_width
? 1080
petemill marked this conversation as resolved.
Show resolved Hide resolved
: available_width;
const auto toolbar_width = width();
int location_bar_width = available_width;
int location_bar_margin_h = 0;
int location_bar_center_offset = 0;
if (restrict_location_bar_width) {
double location_bar_margin_h_pc = 0.07;
petemill marked this conversation as resolved.
Show resolved Hide resolved
if (toolbar_width < 700)
location_bar_margin_h_pc = 0;
else if (toolbar_width < 850)
location_bar_margin_h_pc = 0.03;
else if (toolbar_width < 1000)
location_bar_margin_h_pc = 0.05;
// Apply the target margin, adjusting for min and max width of LocationBar
// Make sure any margin doesn't shrink the LocationBar beyond minimum width
if (available_width > location_bar_min_width) {
int location_bar_max_margin_h = (
available_width -
location_bar_min_width
) /
2;
location_bar_margin_h = std::min(
(int)(
toolbar_width * location_bar_margin_h_pc
),
location_bar_max_margin_h
);
location_bar_width = available_width - (location_bar_margin_h * 2);
// Allow the margin to expand so LocationBar is restrained to max width
if (location_bar_width > location_bar_max_width) {
location_bar_margin_h += (location_bar_width - location_bar_max_width) /
2;
location_bar_width = location_bar_max_width;
}
}
// Center LocationBar as much as possible within Toolbar
const int location_bar_toolbar_center_point =
next_element_x +
location_bar_margin_h +
(location_bar_width / 2);
// Calculate offset - positive for move left and negative for move right
location_bar_center_offset = location_bar_toolbar_center_point -
(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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@petemill petemill Sep 17, 2018

Choose a reason for hiding this comment

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

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):
image

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

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

(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:
image

Copy link
Member

Choose a reason for hiding this comment

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

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

location_bar_center_offset = (location_bar_center_offset > 0)
? std::min(location_bar_margin_h,
location_bar_center_offset)
: std::max((int)(-location_bar_margin_h * .25),
location_bar_center_offset);
}
// Apply offset to margin
const int location_bar_margin_l = location_bar_margin_h -
location_bar_center_offset;
const int location_bar_margin_r = location_bar_margin_h +
location_bar_center_offset;

const int location_x = next_element_x + location_bar_margin_l;
const int location_height = location_bar_->GetPreferredSize().height();
const int location_y = (height() - location_height) / 2;

location_bar_->SetBounds(location_x, location_y,
location_bar_width,
location_height);

return location_bar_margin_r;
}

void BraveToolbarView::Layout() {
// If we have not been initialized yet just do nothing.
if (!initialized_)
Expand Down Expand Up @@ -144,15 +236,15 @@ void BraveToolbarView::Layout() {
} else {
home_->SetVisible(false);
}

// position Brave's BookmarkButton
if (bookmark_ && bookmark_->visible()) {
next_element_x += element_padding;
bookmark_->SetBounds(next_element_x, toolbar_button_y, bookmark_->GetPreferredSize().width(), toolbar_button_height);
next_element_x = bookmark_->bounds().right();
}
next_element_x += GetLayoutConstant(TOOLBAR_STANDARD_SPACING);

next_element_x += element_padding;
Copy link
Member Author

Choose a reason for hiding this comment

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

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


// Position Brave's BookmarkButton
// Only reserve space since the positioning will happen after LocationBar
// position is calculated so we can anchor BookmarkButton inside LocationBar's
// margin.
const bool bookmark_show = (bookmark_ && bookmark_->visible());
if (bookmark_show)
next_element_x += bookmark_->GetPreferredSize().width() + element_padding;
int app_menu_width = app_menu_button_->GetPreferredSize().width();
const int right_padding = GetLayoutConstant(TOOLBAR_STANDARD_SPACING);

Expand All @@ -169,19 +261,31 @@ void BraveToolbarView::Layout() {
available_width -= avatar_->GetPreferredSize().width();
available_width -= element_padding;
}
// Don't allow the omnibox to shrink to the point of non-existence, so
// subtract its minimum width from the available width to reserve it.

// Allow the extension actions to take up a share of the available width,
// but consider the minimum for the location bar
const int location_bar_min_width = location_bar_->GetMinimumSize().width();
const int browser_actions_width = browser_actions_->GetWidthForMaxWidth(
available_width - location_bar_->GetMinimumSize().width());
available_width - location_bar_min_width);
available_width -= browser_actions_width;
const int location_bar_width = available_width;

const int location_height = location_bar_->GetPreferredSize().height();
const int location_y = (height() - location_height) / 2;
location_bar_->SetBounds(next_element_x, location_y,
location_bar_width, location_height);

next_element_x = location_bar_->bounds().right();
const int location_bar_margin_r = SetLocationBarBounds(available_width,
location_bar_min_width,
next_element_x,
element_padding);
// Position the bookmark button so it is anchored to the LocationBar
auto location_bar_bounds = location_bar_->bounds();
if (bookmark_show) {
const int bookmark_width = bookmark_->GetPreferredSize().width();
const int bookmark_x = location_bar_bounds.x() -
bookmark_width -
element_padding;
bookmark_->SetBounds(bookmark_x,
toolbar_button_y,
bookmark_width,
toolbar_button_height);
}
next_element_x = location_bar_bounds.right() + location_bar_margin_r;

// Note height() may be zero in fullscreen.
const int browser_actions_height =
Expand Down
6 changes: 6 additions & 0 deletions browser/ui/views/toolbar/brave_toolbar_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ class BraveToolbarView : public ToolbarView {
void Layout() override;
void Update(content::WebContents* tab) override;
void OnEditBookmarksEnabledChanged();
void OnLocationBarIsWideChanged();
void ShowBookmarkBubble(const GURL& url,
bool already_bookmarked,
bookmarks::BookmarkBubbleObserver* observer) override;
Expand All @@ -29,9 +30,14 @@ class BraveToolbarView : public ToolbarView {
// These two functions call through to GetSizeInternal(), passing themselves
// as the function pointer |View::*get_size|.
gfx::Size GetSizeInternal(gfx::Size (View::*get_size)() const) const override;
int SetLocationBarBounds(const int available_width,
const int location_bar_min_width,
const int next_element_x,
const int element_padding);
BookmarkButton* bookmark_ = nullptr;
// Tracks the preference to determine whether bookmark editing is allowed.
BooleanPrefMember edit_bookmarks_enabled_;
BooleanPrefMember location_bar_is_wide_;
};

#endif
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#include "brave/browser/extensions/api/settings_private/brave_prefs_util.h"

#define PrefsUtil BravePrefsUtil
#include "../../../../../../chrome/browser/extensions/api/settings_private/settings_private_delegate.cc"
#undef PrefsUtil
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#include "brave/browser/extensions/api/settings_private/brave_prefs_util.h"

#define PrefsUtil BravePrefsUtil
#include "../../../../../../chrome/browser/extensions/api/settings_private/settings_private_event_router.cc"
#undef PrefsUtil
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ void BraveAddCommonStrings(content::WebUIDataSource* html_source, Profile* profi
IDS_SETTINGS_SITE_SETTINGS_AUTOPLAY_ASK_RECOMMENDED},
{"appearanceSettingsBraveTheme",
IDS_SETTINGS_APPEARANCE_SETTINGS_BRAVE_THEMES},
{"appearanceSettingsLocationBarIsWide",
IDS_SETTINGS_APPEARANCE_SETTINGS_LOCATION_BAR_IS_WIDE},
};
AddLocalizedStringsBulk(html_source, localized_strings,
arraysize(localized_strings));
Expand Down
1 change: 1 addition & 0 deletions common/pref_names.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,4 @@ const char kWidevineOptedIn[] = "brave.widevine_opted_in";
const char kUseAlternatePrivateSearchEngine[] =
"brave.use_alternate_private_search_engine";
const char kBraveThemeType[] = "brave.theme.type";
const char kLocationBarIsWide[] = "brave.location_bar_is_wide";
1 change: 1 addition & 0 deletions common/pref_names.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@ extern const char kAdBlockCurrentRegion[];
extern const char kWidevineOptedIn[];
extern const char kUseAlternatePrivateSearchEngine[];
extern const char kBraveThemeType[];
extern const char kLocationBarIsWide[];

#endif // BRAVE_COMMON_PREF_NAMES_H_
Loading