WebUI: Settings & 4 different ways to override #1909

petemill · 2019-03-11T19:57:29Z

Two main areas to this PR: The Settings page, and architectural changes to the way we modify chromium WebUIs.

WebUI Settings

A fixed page sub-navigation (on wider windows)
Inject a get-started section
Brave-styles across more material components
Fix some issues with Brave's added sections (e.g. Extensions) showing up during all searches

Fix brave/brave-browser#958
Fix brave/brave-browser#960
Fix brave/brave-browser#955
Address brave/brave-browser#26 (this is more of an umbrella issue)
Fix brave/brave-browser#964
Fix brave/brave-browser#3444
Fix brave/brave-browser#3826

WebUI General overriding

Add a behavior to an existing Polymer JS component. This is similar to injecting a superclass which will receive the same lifecycle method calls. It could also be used to add properties and methods to the component, usable from the component's html template.
Modify an existing Polymer HTML template. When the component is registered, we provide the template DOM element which can be modified before it is cloned for use as content and attached to the document.
Inject style to any existing Polymer HTML component. By naming a module 'brave-override-style-my-module', the <style> element from that module will be taken and injected in to the existing 'my-module' component.
Replace icons in an existing iconset. This takes an icon from one existing iconset and replaces the content of an icon in another existing iconset. Can be used to replace a chromium icon with a brave icon.

Whilst all this can be achieved via .patch files at sync / build time, this new methodology:

Keeps all the modifications functional. The advantage here is realized in the 100% inevitable and likely-common scenarios where the chromium webui implementations and underlying technology changes (e.g. html-modules vs es-modules, or simple module refactors). For some scenarios, we may be able to simply modify the central manipulation library (polymer_overrides.js). In more complicated scenarios, we will at least have all modifications functionally and individually labelled, making 'rebasing' potentially more straightforward than a .patch file with mixed intentions.
Maintains performance since these modifications are only performed once, whilst the component is being registered via the Polymer(component) function.

Debugging:

Display (in verbose console log) how long it took to modify each Polymer template
Option to see timing of module and module-override registration (via hard-coded debug variable)

Submitter Checklist:

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 · 2019-03-19T00:08:28Z

I'm done with functionality for this PR. Just dealing with a slightly complicated rebase now.

petemill · 2019-03-19T04:17:56Z

Some pure refactoring follow-ups I'd like to do for WebUIs after this:

Move some of the other patching to this style of functional modification
Split up settings_overrides.js in to functional files instead of a single large file.
Consider using webpack to generate certain JS files (such as settings_overrides.js)

bsclifton · 2019-03-19T19:44:05Z

I rebased this so I can build- after a completely fresh Release build (deleted src/out, no sccache), I see the following (notice the weird border on the right of the sidebar nav):

bsclifton · 2019-03-19T19:48:10Z

I think the padding/margin under About Brave is a bit excessive. From the original mock-up:

bsclifton · 2019-03-19T19:52:41Z

I remember there were some difficulties- but wanted to check.

Shouldn't there be icons next to each of the titles?

mock-up (shown in brave/brave-browser#955 and brave/brave-browser#964):

implementation:

bsclifton · 2019-03-19T20:09:48Z

@srirambv already captured with brave/brave-browser#3633 - but the focus ring (aria?) looks odd. Not sure if border radius can be added? We don't have to solve this now of course

bsclifton · 2019-03-19T20:13:34Z

This new section needs an entry on the sidebar nav (and icon):

bsclifton · 2019-03-19T20:20:07Z

Not sure if it's because I'm on a fresh profile? but trying to visit any of the autofill options leaves me at a blank page

Clicking goes to:

Expected to see something like:

bsclifton · 2019-03-19T20:50:20Z

@petemill I updated all the issues closed and added the appropriate milestone/labels/test plans 😄 Please just double check those

Functionally - I called out everything I found when reviewing above 😄 Great job on this!

Going to continue reviewing / looking at the code

bsclifton · 2019-03-19T22:02:21Z

browser/resources/settings/brave_settings_overrides.js

+      // then section is no longer 'selected'.
+      // TODO(petemill): If this wasn't a chromium module, we'd simply add a handler
+      // for scrolling away, or have the menu change selection as we scroll.
+      stopObservingScroll()


curious about this code... let's say I click Get Started. Sidebar properly shows it selected (orange). As I scroll down, it does "go out of scope" and un-select itself. But I'm not seeing Extensions, Sync, Appearance, etc select themselves. Is the highlighted section supposed to be shown? (also, if so- maybe we can call out in test plan)

@bsclifton no support for scrolling as you move. Just kept the current chromium highlighting behavior and removed it when scrolling away so it's not confusing. As the comment says, if we want to add more custom code then we can add a feature to have the menu change selection as we scroll, but I'm not sure I'd be in favor of that as it gets complicated when there are multiple items on the screen, or the scroll never gets that far.

For now, the selection on clicking an item is a nice feedback to the menu interaction.

// TODO(petemill): If this wasn't a chromium module, we'd simply add a handler // for scrolling away, or have the menu change selection as we scroll.

bsclifton · 2019-03-19T22:03:51Z

browser/resources/settings/brave_settings_overrides.js

+
+// Templates
+BravePatching.RegisterPolymerTemplateModifications({
+  'settings-ui': (templateContent) => {


for other reviewers: the keys here (ex: settings-ui) are the names of components on the page 😄

bsclifton

Feedback left! Awesome job on this. Would love to re-review when it's ready, just hit me up 😄

petemill · 2019-03-20T00:04:59Z

@srirambv already captured with brave/brave-browser#3633 - but the focus ring (aria?) looks odd. Not sure if border radius can be added? We don't have to solve this now of course

Button style was changed before this PR, so can be addressed separately.

I remember there were some difficulties- but wanted to check.

Shouldn't there be icons next to each of the titles?

mock-up (shown in brave/brave-browser#955 and brave/brave-browser#964):

implementation:

The implementation of this will be more involved, which warrants reprioritization and not delaying this one IMO. The complication is that it will require template modification for the settings-section component as well as a new property added to the JS side of the component which can determine which icon to show.

As with other things, I decided to draw the line somewhere considering the size of this PR and the need to improve the current state of the page, so there were some things that were more minor and will be follow-ups:

Further brave-styles for material-styled components (arrow icons, dialog tab colors, more icons) WebUI: Brave component styles (umbrella / ongoing issue) brave-browser#3799
Section Title icons Settings: Icons for Section Headings brave-browser#3797
Extensions Page sidebar (if we still want to do that given the usability issues) Settings sidebar navigation for brave://extensions WebUI page (similar to brave://settings) brave-browser#3771

I'll look at the other issues @bsclifton brought up - I wasn't experiencing some of them after yesterday's rebase, so maybe they're introduced by today's rebase.

rossmoody · 2019-03-20T00:05:31Z

I remember there were some difficulties- but wanted to check.

Shouldn't there be icons next to each of the titles?

mock-up (shown in brave/brave-browser#955 and brave/brave-browser#964):

implementation:

The design mockup did have those icons in the titles but after giving it some thought, I think we should hold off on them. It's not a great UI pattern (in particular for a settings page vs a marketing page) and makes scanning the content quickly quite a bit more difficult.

petemill · 2019-03-20T16:29:32Z

@rossmoody that decision seems timely! Please add the comment to brave/brave-browser#3797 if not already

petemill · 2019-04-13T20:46:15Z

@bsclifton shields description padding has been increased, and @rossmoody 's style updates integrated. Watching PR builder and attempting to fix issues...

1. Add a behavior to an existing Polymer JS component. This is similar to injecting a superclass which will receive the same lifecycle method calls. It could also be used to add properties and methods to the component, usable from the component's html template. 2. Modify an existing Polymer HTML template. When the component is registered, we provide the template DOM element which can be modified before it is cloned for use as content and attached to the document. 3. Inject style to any existing Polymer HTML component. By naming a module 'brave-override-style-my-module', the <style> element from that module will be taken and injected in to the existing 'my-module' component. 4. Replace icons in an existing iconset. This takes an icon from one existing iconset and replaces the content of an icon in another existing iconset. Can be used to replace a chromium icon with a brave icon. Whilst all this can be achieved via .patch files at sync / build time, this: - Keeps all the modifications functional. The advantage here is realized in the 100% inevitable and likely-common scenarios where the chromium webui implementations and underlying technology changes (e.g. html-modules vs es-modules, or simple module refactors). For some scenarios, we may be able to simply modify the central manipulation library (polymer_overrides.js). In more complicated scenarios, we will at least have all modifications functionally and individually labelled, making 'rebasing' potentially more straightforward than a .patch file with mixed intentions. - Maintains performance since these modifications are only performed once, whilst the component is being registered via the Polymer(component) function. Debugging: - Display (in verbose console log) how long it took to modify each Polymer template - Option to see timing of module and module-override registration (via hard-coded debug variable)

…n, re-organize other sections, and brave-styles Ensures each section shows and hides appropriately Fixes brave's custom sections showing up for all searches since they need a settings-section to be inside the settings/basic-page module. Also fixes some extra and missing borders between settings items.

…ive section

…tings, Bookmarks, Downloads, History and Extensions Note that chrome://brave-resources/page_specific/X url-data-source is used for convenience. Technically these belong in //browser/resources and not //ui/resources, and served from, for example, chrome://bookmarks/X instead. However, the build setup neccessary to copy in to the chromium build output directory before the PAK and Vulcanize process is a lot of overhead for a couple lines of simple css. In the future we may wish to do this setup.

Co-authored by: Ross Moody <fiftyfivehis@gmail.com>

Co-authored by: Pete Miller <miller.pete@gmail.com>

bsclifton

++! 😎👌

WebUI: Settings & 4 different ways to override

petemill self-assigned this Mar 11, 2019

petemill force-pushed the webui-settings-brave branch 2 times, most recently from 36a3c1e to 964608f Compare March 15, 2019 21:01

petemill force-pushed the webui-settings-brave branch 3 times, most recently from 3a15388 to 74f5c1d Compare March 19, 2019 03:34

petemill marked this pull request as ready for review March 19, 2019 04:13

petemill requested review from bbondy, yrliou, cezaraugusto, bridiver, bsclifton and rossmoody March 19, 2019 04:14

bsclifton force-pushed the webui-settings-brave branch from 74f5c1d to 633e04d Compare March 19, 2019 17:01

bsclifton added this to the 0.64.x - Nightly milestone Mar 19, 2019

bsclifton reviewed Mar 19, 2019

View reviewed changes

bsclifton requested changes Mar 19, 2019

View reviewed changes

petemill dismissed bsclifton’s stale review via 7456f23 April 12, 2019 19:54

petemill force-pushed the webui-settings-brave branch 2 times, most recently from 5d91ad4 to 3846f52 Compare April 13, 2019 20:44

petemill and others added 15 commits April 13, 2019 13:47

Settings: fix some item borders

1dc9253

WebUI: brave style for all toggle checkboxes

fe9f63b

WebUI: radio button colors

9390970

Settings: Shields section titles and description text

c3f7be0

Settings: clear settings-menu selection after scrolling away from act…

0b376dc

…ive section

Settings: 'About Brave' menu item includes version number and logo

28d4342

Settings: 'Advanced' -> 'Additional Settings'

f6f8164

Settings: Social Media Blocking menu item

2ea87fe

webui: paper-button gets 'warn' style

121148d

WebUI: dark mode overrides

1da934f

Normalize settings titles to "Sentence case"

6d04b49

Co-authored by: Ross Moody <fiftyfivehis@gmail.com>

WebUI: Settings page style design tweaks

d2daf7e

Co-authored by: Pete Miller <miller.pete@gmail.com>

petemill force-pushed the webui-settings-brave branch from 3846f52 to d2daf7e Compare April 13, 2019 20:47

petemill requested a review from bsclifton April 13, 2019 20:49

bsclifton approved these changes Apr 14, 2019

View reviewed changes

petemill merged commit 87c043d into master Apr 15, 2019

petemill deleted the webui-settings-brave branch April 15, 2019 19:12

petemill added a commit that referenced this pull request Apr 15, 2019

Merge pull request #1909 from brave/webui-settings-brave

59a9827

WebUI: Settings & 4 different ways to override

bsclifton mentioned this pull request Apr 16, 2019

Extensions UI in Settings Page brave/brave-browser#4135

Closed

rebron mentioned this pull request Apr 17, 2019

should be "Same as macOS" and not "Same as MacOS" under brave colors selection brave/brave-browser#3832

Closed

LaurenWags mentioned this pull request May 21, 2019

WebUI: History page pre-load UI is not consistent with post-load UI brave/brave-browser#3826

Closed

This was referenced May 27, 2019

use Brave styling for (unchanged) Additional settings brave/brave-browser#964

Closed

Redesign brave://settings brave/brave-browser#955

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebUI: Settings & 4 different ways to override #1909

WebUI: Settings & 4 different ways to override #1909

petemill commented Mar 11, 2019 •

edited

Loading

petemill commented Mar 19, 2019

petemill commented Mar 19, 2019 •

edited

Loading

bsclifton commented Mar 19, 2019 •

edited

Loading

bsclifton commented Mar 19, 2019

bsclifton commented Mar 19, 2019 •

edited

Loading

bsclifton commented Mar 19, 2019

bsclifton commented Mar 19, 2019

bsclifton commented Mar 19, 2019

bsclifton commented Mar 19, 2019

bsclifton Mar 19, 2019

petemill Mar 19, 2019

bsclifton Mar 19, 2019

bsclifton left a comment

petemill commented Mar 20, 2019

rossmoody commented Mar 20, 2019

petemill commented Mar 20, 2019

petemill commented Apr 13, 2019

bsclifton left a comment

WebUI: Settings & 4 different ways to override #1909

WebUI: Settings & 4 different ways to override #1909

Conversation

petemill commented Mar 11, 2019 • edited Loading

WebUI Settings

WebUI General overriding

Submitter Checklist:

Test Plan:

Reviewer Checklist:

petemill commented Mar 19, 2019

petemill commented Mar 19, 2019 • edited Loading

bsclifton commented Mar 19, 2019 • edited Loading

bsclifton commented Mar 19, 2019

bsclifton commented Mar 19, 2019 • edited Loading

bsclifton commented Mar 19, 2019

bsclifton commented Mar 19, 2019

bsclifton commented Mar 19, 2019

bsclifton commented Mar 19, 2019

bsclifton Mar 19, 2019

Choose a reason for hiding this comment

petemill Mar 19, 2019

Choose a reason for hiding this comment

bsclifton Mar 19, 2019

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

petemill commented Mar 20, 2019

rossmoody commented Mar 20, 2019

petemill commented Mar 20, 2019

petemill commented Apr 13, 2019

bsclifton left a comment

Choose a reason for hiding this comment

petemill commented Mar 11, 2019 •

edited

Loading

petemill commented Mar 19, 2019 •

edited

Loading

bsclifton commented Mar 19, 2019 •

edited

Loading

bsclifton commented Mar 19, 2019 •

edited

Loading