New Tab Page & Favicons Refactor #2490

kylehickinson · 2020-04-28T19:50:29Z

Summary of Changes

Blanket Refactor Issues

This pull request fixes #2578
This pull request fixes #2579

NTP Issues

This pull request fixes #2380
This pull request fixes #2071
This pull request fixes #2036
This pull request fixes #2351
This pull request fixes #2059
This pull request fixes #2516

Keyboard Issues

Note that the "Auto-Open Keyboard" preference has been removed

This pull request fixes #2078
This pull request fixes #2073
This pull request fixes #2321 (because the pref no longer exists)

Favicon Issues

This pull request fixes #2051
This pull request fixes #1162
This pull request fixes #933
This pull request fixes #528
This pull request fixes #2362
This pull request fixes #2485
This pull request fixes #2099
This pull request fixes #1865
This pull request fixes #1030
This pull request fixes #2364

Submitter Checklist:

Unit Tests are updated to cover new or changed functionality
User-facing strings use NSLocalizableString()

Test Plan:

Test New Tab Page related functions:
- Verify stats appear & function
- Verify favorites appear and are left-aligned even with less than 4 items
- Verify show more only appears when there's more than the visible number of favorites on NTP (between 4-6 depending on iPhone vs iPad portrait & landscape)
- Verify iPhone/iPad landscape vs portrait
- Verify background images
- Verify background images source button
- Verify SI's & SI button
- Verify CHP's & QR code button
- Verify private mode callouts
- Verify themes
- Verify no-image-bg light theme shows dark-blue bg now and no gradient
Test all places favicons are seen: Favorites, Bookmarks, Add Bookmark, History, Search, Back/Forward list, Tab Tray, Rewards Panel, Rewards Tipping/Auto-Contribute/Pending Contributions list, etc.
- Verify custom icons are shown for super-referrers
- Verify bundled icons are shown until you visit the page (therefore parsing and downloading the websites up-to-date favicon)
- Verify websites that only have small favicons display monograms on the favorites overlay & NTP. (i.e. https://eqbank.ca) but display the icon in other places
- Verify websites that don't have any valid favicon display monograms (e.g. https://kylehickinson.com)
Test new favorites overlay basic functionality (tap, edit, reorder, scroll, etc.)
Test upgrading from older versions that already have favicons downloaded

Screenshots:

Screenshots

Reviewer Checklist:

Issues include necessary QA labels:
- QA/(Yes|No)
- release-notes/(include|exclude)
- bug / enhancement
Necessary security reviews have taken place.
Adequate unit test coverage exists to prevent regressions.
Adequate test plan exists for QA to validate (if applicable).
Issue is assigned to a milestone (should happen at merge time).

iccub

Some comments and questions just by looking at the code, haven't run it yet.

There will be some more involved after custom homepage code is added, one view to add and some logic.
In general the refactor looks good, there's only one hacky solution of aligning the image credits and other 1 item sections I guess

Will do some functional review later, these are just some initial comments from me

iccub · 2020-04-29T13:18:58Z

BraveUI/Buttons/SpringButton.swift

@@ -0,0 +1,30 @@
+// This Source Code Form is subject to the terms of the Mozilla Public


Need to update license for all newly added files.
#2441

iccub · 2020-04-29T13:21:10Z

Client/Application/ClientPreferences.swift

@@ -98,8 +98,6 @@ extension Preferences {
        static let backgroundImages = Option<Bool>(key: "newtabpage.background-images", default: true)
        /// Whether sponsored images are included into the background image rotation
        static let backgroundSponsoredImages = Option<Bool>(key: "newtabpage.background-sponsored-images", default: true)
-        /// Whether the iOS keyboard auto-opens on a NTP or not
-        static let autoOpenKeyboard = Option<Bool>(key: "newtabpage.auto-open-keyboard", default: false)


Are we getting rid of this setting option?

Yeah, partly because of Brave Today, party because tapping on URL bar will now display an entirely separate VC from NTP with just favs (no more stats, etc.)

iccub · 2020-04-29T13:24:02Z

Client/Frontend/Browser/BrowserViewController.swift

@@ -3352,6 +3344,27 @@ extension BrowserViewController: ToolbarUrlActionsDelegate {
    }
 }

+extension BrowserViewController: NewTabPageDelegate {
+    func navigateToInput(_ input: String, inNewTab: Bool, switchingToPrivateMode: Bool) {


What is this for, looks like it focuses on url bar with some url/text typed in, is this some new behavior or we use it somewhere already?
Hmm or is this what happens when you tap on favorites, or on bg image's credits

I copied the base of it from FavoritesDelegate:

protocol FavoritesDelegate: AnyObject { func didSelect(input: String) ...

Not sure why it passed in String (maybe for bookmarklets?) but I just kept it that way just in case. Its called based on a fav being clicked, or context menu actioned it to open in a new tab/new private tab

iccub · 2020-04-29T13:27:09Z

Client/Frontend/Browser/Favorites/FavoriteCell.swift

@@ -31,6 +31,8 @@ class FavoriteCell: UICollectionViewCell {
    var imageInsets: UIEdgeInsets = UIEdgeInsets.zero
    var cellInsets: UIEdgeInsets = UIEdgeInsets.zero

+    var longPressed: ((FavoriteCell) -> Void)?


rename to something like longPressHandler or didLongPress

Client/Frontend/Browser/Favorites/FavoriteCell.swift

iccub · 2020-04-29T14:26:34Z

Client/Frontend/Browser/New Tab Page/Sections/FavoritesSectionProvider.swift

+        // some padding between the title and the image.
+        let scale = 1.0 / UIScreen.main.scale
+        return CGSize(width: (scale * (cellWidth / scale)).rounded(.down),
+                      height: 1000)


Why this hardcoded height, just to tell the item to take as much space as needed?

iccub · 2020-04-29T14:27:37Z

Client/Frontend/Browser/New Tab Page/Sections/FavoritesSectionProvider.swift

+        cell.longPressed = { [weak self] cell in
+            let alert = UIAlertController(title: nil, message: nil, preferredStyle: .actionSheet)
+
+            let edit = UIAlertAction(title: Strings.editBookmark, style: .default) { (action) in


Can remove brackets here and in action below

iccub · 2020-04-29T14:27:52Z

Client/Frontend/Browser/New Tab Page/Sections/FavoritesSectionProvider.swift

+    @available(iOS 13.0, *)
+    func collectionView(_ collectionView: UICollectionView, contextMenuConfigurationForItemAt indexPath: IndexPath, point: CGPoint) -> UIContextMenuConfiguration? {
+        guard let favourite = frc.fetchedObjects?[indexPath.item] else { return nil }
+        return UIContextMenuConfiguration(identifier: indexPath as NSCopying, previewProvider: nil) { (elemenst) -> UIMenu? in


typo in elemenst and remove brackets

iccub · 2020-04-29T14:31:39Z

Client/Frontend/Browser/New Tab Page/NewTabPageViewController.swift

+    /// The section is given half the available space
+    ///
+    /// Layout is decided by device type (iPad vs iPhone)
+    case halfWidth


What do we need this half width for?

This is for iPhone/iPad landscape, where the NTP is half the width of the screen and aligned to the left or right depending on the device (iPad = left, iPhone = right) but Brave Today will not have this same logic (it will just be centered regardless)

iccub · 2020-04-29T14:41:23Z

Client/Frontend/Browser/New Tab Page/NewTabPageViewController.swift

+
+    private let notifications: NewTabPageNotifications
+
+    private let sponsorButton = SpringButton()


This sponsor button should have its own collectionview section, no? Otherwise when BraveToday is added this will be still attached to bottom of the screen

As far as I know, the sponsored image is not supposed to move, which is why I did this. It also doesn't fit the design either. On landscape iPhone its placed off on the left side next to the other elements :(

Adds `NewTabPageBackgroundButtonsView` which will now house any static static elements that may be placed on the NTP such image credit, brand logos and share via QR code buttons.

This is no longer required as NTPs are linked to Tabs and thus do not need to regenerate background images when switching modes

Allows for 5 icon on iPad portrait 2/3

Tests were invalid since these controllers were split

iccub · 2020-05-26T16:06:24Z

Client/Frontend/Browser/Favicons/LargeFaviconView.swift

+            if let domain = domain, let url = domain.url?.asURL, !url.absoluteString.isEmpty {
+                // Use the base domain's first character, but if that isn't valid
+                // use the favorites title as the monogram instead
+                monogramFallbackLabel.text = url.baseDomain?.first?.uppercased() ??


Two things here:

We use this logic in two places, how about adding a helper method in URL extension to get first letter of the url? This way we could also add some basic unit tests, for example check if www part is stripped

What if all these three methods fail, we show no letter right? I think it would be better to add a fallback letter just in case could be W for website

baseDomain gets rid of any www, and only seems to fail for direct IPs (192.168.1.1 for example) which won't include www anyways and url.host should theoretically never be nil in our cases unless someone edited the URL and just got rid of the entire host? I'm not sure what the best course of action is really if they all somehow fail. Maybe just the first character in the absoluteString? 😂

iccub · 2020-05-26T16:15:02Z

Client/Frontend/Browser/Favicons/FaviconFetcher.swift

+    ///
+    /// If the app does not contain a custom icon for the site provided `nil`
+    /// will be returned
+    var customIcon: FaviconAttributes? {


We should make all these custom, bundled, downloaded icon getters private so we never directly access them by accident.
I assume we should always call load in order to retrieve a favicon

They were actually private before, I made them ~~public~~ internal so I could unit test them :)

iccub · 2020-05-26T16:17:11Z

Client/Frontend/Browser/Favicons/FaviconFetcher.swift

+    }
+
+    /// Begin the search for a favicon for the site.
+    func load(_ completion: @escaping (URL, FaviconAttributes) -> Void) {


Maybe add a documentation comment that completion returns on main thread?

iccub · 2020-05-26T17:53:15Z

Client/Frontend/Browser/New Tab Page/NewTabPageViewController.swift

+        delegate?.tappedQRCodeButton(url: url)
+    }
+
+    private func handleBookmarkAction(bookmark: Bookmark, action: BookmarksAction) {


This is almost the same as implementation in BVC, maybe centralize this logic somewhere? At least reuse the code for popup in edit action

iccub

Good job, clean refactor and very much needed

Data/models/Domain.swift

Client/Frontend/Browser/Favicons/FaviconFetcher.swift

jumde · 2020-06-05T17:32:42Z

Client/Frontend/Browser/Favicons/FaviconFetcher.swift

+                return
+            }
+
+            self.downloadIcon(url: faviconURL, addingToDatabase: true) { [weak self] image in


Check for isPrivate before adding to database?

It is already checked on completion on line 220: https://github.com/brave/brave-ios/pull/2490/files/6740bd8a934071e866f7db5d53b1d809c9da925c#diff-d47742238791692d201e5deaa57d0377R220

if addingToDatabase && !PrivateBrowsingManager.shared.isPrivateBrowsing { FaviconMO.add(favicon, forSiteUrl: self.url) }

Fix issue where unsized icon would be used instead of sized ones

kylehickinson · 2020-06-08T15:03:48Z

CI passed, but for some reason was still showing pending

kylehickinson added 2 commits April 28, 2020 15:49

WIP: New Tab Page Refactor

6ebf1a4

iOS 12 long-press support

531d6d3

iccub reviewed Apr 29, 2020

View reviewed changes

kylehickinson added 9 commits April 29, 2020 15:55

Share favourites count logic for overflow section + small feedback fixes

8277ec8

WIP: Favorites overlay

1fd02b0

Merge development into new-tab-page-refactor

bf2b452

Support for custom themes changes & merge fixes

2e1e4a5

Adds `NewTabPageBackgroundButtonsView` which will now house any static static elements that may be placed on the NTP such image credit, brand logos and share via QR code buttons.

Fix background focal point calculation offsets

e8acd37

Fix crash if NTP section updates before dataSource is setup

e3aaf3a

Don't log warning about Rewards favicon on for local pages

83b074d

Anchor favorites overlay & NTP to layout guide

b892d9a

Add drag & drop reorder to favorites overlay

cc7bc2a

kylehickinson force-pushed the new-tab-page-refactor branch from 19628c9 to cc7bc2a Compare May 2, 2020 00:04

kylehickinson added 15 commits May 1, 2020 20:20

Remove animation for hiding NTP;

7bec53a

Add interactive keyboard dismiss to fav overlay

9b4b78d

Fix height of favs on NTP

d2e13f5

Finish up favourites reordering

94c8367

Move NTP to Tab

67e7915

Show correct favicon on tab tray for NTP

02815f4

Cleanup & remove old FavoritesViewController

1004913

Rename favorites view controller

e7d6502

Favorites overlay now hides toolbar

77cf228

Fix NTP Memory Leak

bbcd23a

Fix bug where an NTP would remain in view hierarchy when switching modes

08e7e12

Don't reset background image when switching private modes

421bd41

This is no longer required as NTPs are linked to Tabs and thus do not need to regenerate background images when switching modes

Update NTP favicon layout to look better on iPad portrait & multitasking

0531815

Allows for 5 icon on iPad portrait 2/3

Adjust favorite cell pointer interaction

92e7d2b

Add FRC batch updates for favs overlay

4a74de7

kylehickinson force-pushed the new-tab-page-refactor branch from aaa6410 to 4a74de7 Compare May 7, 2020 17:01

Remove favs data source & old favs tests

7972a52

Tests were invalid since these controllers were split

This was linked to issues May 26, 2020

Favourites icon mismatch when they are reordered #2099

Closed

different icons are sometimes being displayed when scrolling through History #1865

Closed

kylehickinson added 2 commits May 26, 2020 11:33

Remove redundant check for URL on favicon image view extension

459ad04

Fix favorites overlay text color on dark themes in iOS 12

b459c5a

kylehickinson requested review from iccub and Brandon-T May 26, 2020 16:06

Merge development into new-tab-page-refactor

dd3cd33

iccub reviewed May 26, 2020

View reviewed changes

kylehickinson added 3 commits May 26, 2020 15:26

Consolidate monogram letter logic

d8c1767

Consolidate handling bookmark actions logic

aa60e3f

Add additional documentation in favicon fetcher

3d7c870

kylehickinson requested a review from iccub May 26, 2020 19:43

Remove code that deletes all Domain's regardless of Bookmark association

070d693

kylehickinson added the needs security review label May 27, 2020

iccub approved these changes May 28, 2020

View reviewed changes

kylehickinson added 6 commits May 28, 2020 11:39

Fix monogram appearing on top of folders when scrolling in Bookmarks

52ddef0

Ensure monogram favicons are cleared before resetting image

190260f

Fully-clear monogram favicons at start of favicon load

b5a0d62

Merge development into new-tab-page-refactor

39e855c

Fix favorites bleeding under safe area (and padding on bg buttons)

994c3cc

Fix issue where fresh install would not show favourites overflow

dfaa3b5

jumde reviewed Jun 4, 2020

View reviewed changes

Data/models/Domain.swift Show resolved Hide resolved

jumde reviewed Jun 5, 2020

View reviewed changes

Client/Frontend/Browser/Favicons/FaviconFetcher.swift Show resolved Hide resolved

jumde reviewed Jun 5, 2020

View reviewed changes

Also pull precomposed apple-touch-icon's during metadata parse

b388de3

Fix issue where unsized icon would be used instead of sized ones

kylehickinson force-pushed the new-tab-page-refactor branch from 6740bd8 to b388de3 Compare June 5, 2020 17:52

brave deleted a comment from kylehickinson Jun 5, 2020

kylehickinson merged commit f2f97e5 into development Jun 8, 2020

kylehickinson deleted the new-tab-page-refactor branch June 8, 2020 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Tab Page & Favicons Refactor #2490

New Tab Page & Favicons Refactor #2490

kylehickinson commented Apr 28, 2020 •

edited

Loading

iccub left a comment

iccub Apr 29, 2020

iccub Apr 29, 2020

kylehickinson Apr 29, 2020 •

edited

Loading

iccub Apr 29, 2020

kylehickinson Apr 29, 2020

iccub Apr 29, 2020

iccub Apr 29, 2020

kylehickinson Apr 29, 2020

iccub Apr 29, 2020

iccub Apr 29, 2020

iccub Apr 29, 2020

kylehickinson Apr 29, 2020

iccub Apr 29, 2020

kylehickinson Apr 29, 2020

iccub May 26, 2020

kylehickinson May 26, 2020

iccub May 26, 2020

kylehickinson May 26, 2020 •

edited

Loading

iccub May 26, 2020

iccub May 26, 2020

iccub left a comment

jumde Jun 5, 2020

kylehickinson Jun 5, 2020

kylehickinson commented Jun 8, 2020

		@@ -0,0 +1,30 @@
		// This Source Code Form is subject to the terms of the Mozilla Public


		private let notifications: NewTabPageNotifications

		private let sponsorButton = SpringButton()

New Tab Page & Favicons Refactor #2490

New Tab Page & Favicons Refactor #2490

Conversation

kylehickinson commented Apr 28, 2020 • edited Loading

Summary of Changes

Blanket Refactor Issues

NTP Issues

Keyboard Issues

Favicon Issues

Submitter Checklist:

Test Plan:

Screenshots:

Reviewer Checklist:

iccub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylehickinson Apr 29, 2020 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylehickinson May 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iccub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylehickinson commented Jun 8, 2020

kylehickinson commented Apr 28, 2020 •

edited

Loading

kylehickinson Apr 29, 2020 •

edited

Loading

kylehickinson May 26, 2020 •

edited

Loading