Skip to content

Commit 1d08f4d

Browse files
Caroline RisingChromium LUCI CQ
Caroline Rising
authored and
Chromium LUCI CQ
committed
Revert "Side Panel: update focus rings to have correct contrast and respect accent color settings."
This reverts commit 95a3da5. Reason for revert: This breaks for incognito since views passes in a dark color for the background while webui returns light colors for text. Reverting this in favor of waiting for ongoing working for supporting color provider in webui to land before making these changes. Original change's description: > Side Panel: update focus rings to have correct contrast and respect accent color settings. > > Update side panel focus ring colors on theme change using cascading > accent color. > > Bug: 1250506 > Change-Id: Ie61cacb9a8c41382a21b735ff359d369a101a0f2 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3193213 > Reviewed-by: John Lee <[email protected]> > Reviewed-by: Alex Gough <[email protected]> > Reviewed-by: Peter Boström <[email protected]> > Commit-Queue: Caroline Rising <[email protected]> > Cr-Commit-Position: refs/heads/main@{#929675} Bug: 1250506 Change-Id: Id339cc618892f39968e08b610780c0144978dfc7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3216950 Bot-Commit: Rubber Stamper <[email protected]> Reviewed-by: Peter Boström <[email protected]> Reviewed-by: Alex Gough <[email protected]> Reviewed-by: John Lee <[email protected]> Commit-Queue: Caroline Rising <[email protected]> Cr-Commit-Position: refs/heads/main@{#933209}
1 parent 210086e commit 1d08f4d

15 files changed

+10
-111
lines changed

chrome/browser/resources/read_later/BUILD.gn

-1
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,6 @@ ts_library("build_ts") {
148148
definitions = [
149149
"//tools/typescript/definitions/bookmark_manager_private.d.ts",
150150
"//tools/typescript/definitions/bookmarks.d.ts",
151-
"//tools/typescript/definitions/chrome_send.d.ts",
152151
"//tools/typescript/definitions/chrome_event.d.ts",
153152
"//tools/typescript/definitions/metrics_private.d.ts",
154153
]

chrome/browser/resources/read_later/app.html

-1
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@
5353
}
5454

5555
:host([side-panel]) #currentPageActionButton {
56-
--focus-shadow-color: var(--side-panel-focus-outline-color);
5756
margin: 8px 16px 0 16px;
5857
}
5958

chrome/browser/resources/read_later/read_later_api_proxy.js

-11
Original file line numberDiff line numberDiff line change
@@ -49,12 +49,6 @@ export class ReadLaterApiProxy {
4949

5050
closeUI() {}
5151

52-
/**
53-
* @return {!Promise<{colors: !Object<string, string>}>} Object with CSS
54-
* variables as keys and rgba strings as values
55-
*/
56-
getColors() {}
57-
5852
/** @return {!readLater.mojom.PageCallbackRouter} */
5953
getCallbackRouter() {}
6054
}
@@ -119,11 +113,6 @@ export class ReadLaterApiProxyImpl {
119113
this.handler.closeUI();
120114
}
121115

122-
/** @override */
123-
getColors() {
124-
return this.handler.getThemeColors();
125-
}
126-
127116
/** @override */
128117
getCallbackRouter() {
129118
return this.callbackRouter;

chrome/browser/resources/read_later/side_panel/app.html

-3
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,5 @@
11
<style include="mwb-element-shared-style">
22
:host {
3-
--mwb-icon-button-focus-ring-color: var(--side-panel-focus-outline-color);
4-
--mwb-background-color: var(--side-panel-background-color);
5-
background: var(--side-panel-background-color);
63
color: var(--cr-primary-text-color);
74
display: flex;
85
flex-direction: column;

chrome/browser/resources/read_later/side_panel/app.ts

+2-24
Original file line numberDiff line numberDiff line change
@@ -14,19 +14,13 @@ import '../strings.m.js';
1414
import './bookmarks_list.js';
1515

1616
import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
17-
import {WebUIListenerBehavior} from 'chrome://resources/js/web_ui_listener_behavior.m.js';
18-
import {html, mixinBehaviors, PolymerElement} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
19-
17+
import {html, PolymerElement} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
2018
import {ReadLaterApiProxy, ReadLaterApiProxyImpl} from '../read_later_api_proxy.js';
2119

2220
// Key for localStorage object that refers to the last active tab's ID.
2321
export const LOCAL_STORAGE_TAB_ID_KEY = 'lastActiveTab';
2422

25-
const SidePanelAppElementBase =
26-
mixinBehaviors([WebUIListenerBehavior], PolymerElement) as
27-
{new (): PolymerElement & WebUIListenerBehavior};
28-
29-
export class SidePanelAppElement extends SidePanelAppElementBase {
23+
export class SidePanelAppElement extends PolymerElement {
3024
static get is() {
3125
return 'side-panel-app';
3226
}
@@ -58,14 +52,6 @@ export class SidePanelAppElement extends SidePanelAppElementBase {
5852

5953
connectedCallback() {
6054
super.connectedCallback();
61-
62-
this.fetchAndUpdateColors_();
63-
this.addWebUIListener('theme-changed', () => {
64-
// Refetch theme colors on theme change.
65-
this.fetchAndUpdateColors_();
66-
});
67-
chrome.send('observeThemeChanges');
68-
6955
const lastActiveTab = window.localStorage[LOCAL_STORAGE_TAB_ID_KEY];
7056
if (loadTimeData.getBoolean('hasUnseenReadingListEntries')) {
7157
window.localStorage[LOCAL_STORAGE_TAB_ID_KEY] = 'readingList';
@@ -77,14 +63,6 @@ export class SidePanelAppElement extends SidePanelAppElementBase {
7763
this.apiProxy_.showUI();
7864
}
7965

80-
private fetchAndUpdateColors_() {
81-
this.apiProxy_.getColors().then(({colors}) => {
82-
for (const [cssVariable, value] of Object.entries(colors)) {
83-
this.style.setProperty(cssVariable, value);
84-
}
85-
});
86-
}
87-
8866
private getTabNames_(): string[] {
8967
return Object.values(this.tabs_);
9068
}

chrome/browser/resources/read_later/side_panel/side_panel.html

+7
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,15 @@
1616
}
1717

1818
body {
19+
background: white;
1920
overflow: auto;
2021
}
22+
23+
@media (prefers-color-scheme: dark) {
24+
body {
25+
background: var(--google-grey-900);
26+
}
27+
}
2128
</style>
2229
</head>
2330
<body>

chrome/browser/ui/views/bubble/bubble_contents_wrapper.cc

-10
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
#include "content/public/browser/native_web_keyboard_event.h"
1010
#include "content/public/browser/render_widget_host_view.h"
1111
#include "ui/base/models/menu_model.h"
12-
#include "ui/gfx/color_palette.h"
1312
#include "ui/gfx/geometry/rounded_corners_f.h"
1413
#include "ui/views/widget/widget.h"
1514

@@ -30,11 +29,6 @@ content::WebContents::CreateParams GetWebContentsCreateParams(
3029

3130
} // namespace
3231

33-
SkColor BubbleContentsWrapper::Host::GetColorProviderColor(ui::ColorId id) {
34-
NOTREACHED();
35-
return gfx::kPlaceholderColor;
36-
}
37-
3832
bool BubbleContentsWrapper::Host::HandleKeyboardEvent(
3933
content::WebContents* source,
4034
const content::NativeWebKeyboardEvent& event) {
@@ -143,10 +137,6 @@ void BubbleContentsWrapper::HideContextMenu() {
143137
host_->HideCustomContextMenu();
144138
}
145139

146-
SkColor BubbleContentsWrapper::GetColorProviderColor(ui::ColorId id) {
147-
return host_ ? host_->GetColorProviderColor(id) : gfx::kPlaceholderColor;
148-
}
149-
150140
base::WeakPtr<BubbleContentsWrapper::Host> BubbleContentsWrapper::GetHost() {
151141
return host_;
152142
}

chrome/browser/ui/views/bubble/bubble_contents_wrapper.h

-3
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
#include "content/public/browser/web_contents_delegate.h"
1717
#include "content/public/common/referrer.h"
1818
#include "ui/base/models/menu_model.h"
19-
#include "ui/color/color_id.h"
2019
#include "ui/webui/mojo_bubble_web_ui_controller.h"
2120

2221
namespace content {
@@ -40,7 +39,6 @@ class BubbleContentsWrapper : public content::WebContentsDelegate,
4039
gfx::Point point,
4140
std::unique_ptr<ui::MenuModel> menu_model) {}
4241
virtual void HideCustomContextMenu() {}
43-
virtual SkColor GetColorProviderColor(ui::ColorId id);
4442
virtual void ResizeDueToAutoResize(content::WebContents* source,
4543
const gfx::Size& new_size) {}
4644
virtual bool HandleKeyboardEvent(
@@ -79,7 +77,6 @@ class BubbleContentsWrapper : public content::WebContentsDelegate,
7977
void ShowContextMenu(gfx::Point point,
8078
std::unique_ptr<ui::MenuModel> menu_model) override;
8179
void HideContextMenu() override;
82-
SkColor GetColorProviderColor(ui::ColorId id) override;
8380

8481
// Reloads the WebContents hosting the WebUI.
8582
virtual void ReloadWebContents() = 0;

chrome/browser/ui/views/toolbar/read_later_toolbar_button.cc

-17
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
#include "chrome/app/vector_icons/vector_icons.h"
1313
#include "chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.h"
1414
#include "chrome/browser/feature_engagement/tracker_factory.h"
15-
#include "chrome/browser/themes/theme_properties.h"
1615
#include "chrome/browser/ui/bookmarks/bookmark_utils.h"
1716
#include "chrome/browser/ui/browser.h"
1817
#include "chrome/browser/ui/read_later/reading_list_model_factory.h"
@@ -27,10 +26,7 @@
2726
#include "components/feature_engagement/public/feature_constants.h"
2827
#include "components/feature_engagement/public/tracker.h"
2928
#include "ui/base/l10n/l10n_util.h"
30-
#include "ui/color/color_id.h"
31-
#include "ui/color/color_provider.h"
3229
#include "ui/views/accessibility/view_accessibility.h"
33-
#include "ui/views/cascading_property.h"
3430
#include "ui/views/controls/button/button_controller.h"
3531
#include "ui/views/controls/dot_indicator.h"
3632
#include "ui/views/controls/menu/menu_runner.h"
@@ -62,9 +58,6 @@ class ReadLaterSidePanelWebView : public views::WebView,
6258
SetWebContents(contents_wrapper_->web_contents());
6359
set_allow_accelerators(true);
6460

65-
views::SetCascadingColorProviderColor(
66-
this, views::kCascadingBackgroundColor, ui::kColorBubbleBackground);
67-
6861
if (base::FeatureList::IsEnabled(features::kSidePanelDragAndDrop)) {
6962
extensions::BookmarkManagerPrivateDragEventRouter::CreateForWebContents(
7063
contents_wrapper_->web_contents());
@@ -125,16 +118,6 @@ class ReadLaterSidePanelWebView : public views::WebView,
125118
if (context_menu_runner_)
126119
context_menu_runner_->Cancel();
127120
}
128-
SkColor GetColorProviderColor(ui::ColorId id) override {
129-
switch (id) {
130-
case ui::kColorDialogBackground:
131-
return GetCascadingBackgroundColor(this);
132-
case ui::kColorFocusableBorderFocused:
133-
return GetCascadingAccentColor(this);
134-
default:
135-
return GetColorProvider()->GetColor(id);
136-
}
137-
}
138121
bool HandleKeyboardEvent(
139122
content::WebContents* source,
140123
const content::NativeWebKeyboardEvent& event) override {

chrome/browser/ui/webui/read_later/read_later.mojom

-6
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,6 @@ interface PageHandler {
6262

6363
// Notify the backend that the dialog should be closed.
6464
CloseUI();
65-
66-
// Get colors of the current theme with CSS variables as keys and rgba strings
67-
// as values.
68-
// TODO(corising): This is also done in tab_strip.mojom. We should find a way
69-
// to unify this.
70-
GetThemeColors() => (map<string, string> colors);
7165
};
7266

7367
enum CurrentPageActionButtonState {

chrome/browser/ui/webui/read_later/read_later_page_handler.cc

-16
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@
3636
#include "ui/base/models/simple_menu_model.h"
3737
#include "ui/base/mojom/window_open_disposition.mojom.h"
3838
#include "ui/base/window_open_disposition.h"
39-
#include "ui/gfx/color_utils.h"
4039
#include "url/gurl.h"
4140

4241
namespace {
@@ -279,21 +278,6 @@ void ReadLaterPageHandler::CloseUI() {
279278
embedder->CloseUI();
280279
}
281280

282-
void ReadLaterPageHandler::GetThemeColors(GetThemeColorsCallback callback) {
283-
base::flat_map<std::string, std::string> colors;
284-
auto embedder = read_later_ui_->embedder();
285-
if (!embedder) {
286-
std::move(callback).Run(std::move(colors));
287-
return;
288-
}
289-
colors["--side-panel-focus-outline-color"] = color_utils::SkColorToRgbaString(
290-
embedder->GetColorProviderColor(ui::kColorFocusableBorderFocused));
291-
colors["--side-panel-background-color"] = color_utils::SkColorToRgbaString(
292-
embedder->GetColorProviderColor(ui::kColorDialogBackground));
293-
294-
std::move(callback).Run(std::move(colors));
295-
}
296-
297281
void ReadLaterPageHandler::ReadingListModelCompletedBatchUpdates(
298282
const ReadingListModel* model) {
299283
DCHECK(model == reading_list_model_);

chrome/browser/ui/webui/read_later/read_later_page_handler.h

-1
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ class ReadLaterPageHandler : public read_later::mojom::PageHandler,
5353
void UpdateCurrentPageActionButtonState() override;
5454
void ShowUI() override;
5555
void CloseUI() override;
56-
void GetThemeColors(GetThemeColorsCallback callback) override;
5756

5857
// ReadingListModelObserver:
5958
void ReadingListModelLoaded(const ReadingListModel* model) override {}

chrome/browser/ui/webui/read_later/read_later_ui.cc

+1-4
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
#include "chrome/browser/ui/webui/favicon_source.h"
1515
#include "chrome/browser/ui/webui/read_later/read_later_page_handler.h"
1616
#include "chrome/browser/ui/webui/read_later/side_panel/bookmarks_page_handler.h"
17-
#include "chrome/browser/ui/webui/theme_handler.h"
1817
#include "chrome/browser/ui/webui/webui_util.h"
1918
#include "chrome/common/webui_url_constants.h"
2019
#include "chrome/grit/generated_resources.h"
@@ -43,7 +42,7 @@ void AddLocalizedString(content::WebUIDataSource* source,
4342
} // namespace
4443

4544
ReadLaterUI::ReadLaterUI(content::WebUI* web_ui)
46-
: ui::MojoBubbleWebUIController(web_ui, /*enable_chrome_send=*/true),
45+
: ui::MojoBubbleWebUIController(web_ui),
4746
webui_load_timer_(web_ui->GetWebContents(),
4847
"ReadingList.WebUI.LoadDocumentTime",
4948
"ReadingList.WebUI.LoadCompletedTime") {
@@ -100,8 +99,6 @@ ReadLaterUI::ReadLaterUI(content::WebUI* web_ui)
10099
: IDR_READ_LATER_READ_LATER_HTML);
101100
content::WebUIDataSource::Add(web_ui->GetWebContents()->GetBrowserContext(),
102101
source);
103-
104-
web_ui->AddMessageHandler(std::make_unique<ThemeHandler>());
105102
}
106103

107104
ReadLaterUI::~ReadLaterUI() = default;

chrome/test/data/webui/read_later/test_read_later_api_proxy.js

-10
Original file line numberDiff line numberDiff line change
@@ -20,17 +20,13 @@ export class TestReadLaterApiProxy extends TestBrowserProxy {
2020
'updateCurrentPageActionButtonState',
2121
'showUI',
2222
'closeUI',
23-
'getColors',
2423
]);
2524

2625
/** @type {!readLater.mojom.PageCallbackRouter} */
2726
this.callbackRouter = new readLater.mojom.PageCallbackRouter();
2827

2928
/** @private {!readLater.mojom.ReadLaterEntriesByStatus} */
3029
this.entries_;
31-
32-
/** @private {!Object<string, string>} */
33-
this.colors_ = {};
3430
}
3531

3632
/** @override */
@@ -79,12 +75,6 @@ export class TestReadLaterApiProxy extends TestBrowserProxy {
7975
this.methodCalled('closeUI');
8076
}
8177

82-
/** @override */
83-
getColors() {
84-
this.methodCalled('getColors');
85-
return Promise.resolve({colors: this.colors_});
86-
}
87-
8878
/** @override */
8979
getCallbackRouter() {
9080
return this.callbackRouter;

ui/webui/mojo_bubble_web_ui_controller.h

-4
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,6 @@ namespace ui {
2020

2121
class MenuModel;
2222

23-
using ColorId = int;
24-
using SkColor = uint32_t;
25-
2623
class MojoBubbleWebUIController : public MojoWebUIController {
2724
public:
2825
class Embedder {
@@ -32,7 +29,6 @@ class MojoBubbleWebUIController : public MojoWebUIController {
3229
virtual void ShowContextMenu(gfx::Point point,
3330
std::unique_ptr<ui::MenuModel> menu_model) = 0;
3431
virtual void HideContextMenu() = 0;
35-
virtual SkColor GetColorProviderColor(ColorId id) = 0;
3632
};
3733

3834
// By default MojoBubbleWebUIController do not have normal WebUI bindings.

0 commit comments

Comments
 (0)