1263101 - Incorrect functionality when adding bookmarks without internet connection

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Simon Basca [:SimonB]]

Build: 5.0.0(1)

1. Launch Firefox without internet connection
2. Go to goolge.com
3. Bookmark the offline page
4. Connect to the internet  
5. Reload the page by tapping on the "Try again" button
6. The offline page is bookmarked separately from the one reloaded in the bookmark list

Actual results:
- The page can be bookmarked without internet connection and will not join with the one reloaded.

Expected results: 
- The page bookmarked without internet connection should be merged whit the one reloaded with internet connection
- The pages title should be refreshed in the bookmark list

Note:
- Observe that the pages are bookmarked separately in the bookmark list will be displayed as followed "The internet connection appears to be offline".

Comment 1

[Brian Nicholson (:bnicholson)]

Two options here:
1) Prevent bookmarking error pages.
2) Extract the URL of the page being loaded from the error page URL and bookmark that instead.

Comment 2

[Maurya Talisetti]

Brian,

Can you explain option 2? I'm not sure if you mean bookmarking the URL of the error page or the reloaded page.

Comment 3

[Brian Nicholson (:bnicholson)]

I meant the reloaded page. On second thought, though, this option might not work well since we won't have the title or favicon of the page. Also, doing this would require changes to the bookmark detection in the menu to see whether a give page should be starred.

So let's go with the easy choice: option 1!

Comment 4

[Maurya Talisetti]

Sounds right! Sure, I'll go with option 1 :)

Comment 5

[Maurya Talisetti]

Brian,

How can I check if the currently loaded page is an error page?

I believe we can add that check here: https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Widgets/Menu/MenuView.swift#L369

This is where the bookmark animation first happens before the flow moving on to saving the bookmark.

Comment 6

[Brian Nicholson (:bnicholson)]

Hey Maurya, thanks for taking this! You can use ErrorPageHelper.isErrorPageURL() to determine whether a give URL is an error page.

As for where to put this check, that depends on the UX we want. Robin, for pages that shouldn't be bookmarkable, what do we want to show? Should we gray out the bookmarks button? Hide it completely? Something else?

Comment 7

[Robin Andersen [:tecgirl]]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> Hey Maurya, thanks for taking this! You can use
> ErrorPageHelper.isErrorPageURL() to determine whether a give URL is an error
> page.
> 
> As for where to put this check, that depends on the UX we want. Robin, for
> pages that shouldn't be bookmarkable, what do we want to show? Should we
> gray out the bookmarks button? Hide it completely? Something else?

1) Prevent bookmarking error pages – by disabling it in the menu (greying it out, no response on tap)

Strangely, if I tap the bookmarked offline link 'The Internet connection appears to be offline…' - it actually goes to the correct URL.

Comment 8

[Maurya Talisetti]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1877

Hi Brian,

I've made the bookmark icon to not be shown if it's an error page.

There's one thing though. For tabs with nil url:
- Prior to the change, the Bookmark icon was being displayed, though it would not action.
- After the change, the bookmark icon is not being displayed altogether.

Hope this is ok. Just curious though, why would we have tabs with nil urls?

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8759065 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1877

Hey Maurya, thanks for taking this. Rather than removing the bookmark button from the menu, I think we actually just want to disable it/grey it out instead as mentioned in comment 7.

(In reply to Maurya Talisetti from comment #8)
> There's one thing though. For tabs with nil url:
> - Prior to the change, the Bookmark icon was being displayed, though it
> would not action.
> - After the change, the bookmark icon is not being displayed altogether.

You're correct that we should treat nil URLs the same as error page URLs (that is, we should disable the bookmark button).

> Just curious though, why would we have tabs with nil urls?

Internally, WKWebView can dispose of its content whenever there's a low memory event. WKWebViews can consume a lot of memory, especially if the user has many tabs open, so this is how WKWebViews prevent apps from running out of memory and crashing. When this happens, the WKWebView URL will be nil until the WKWebView restores its content, which usually happens when it's visible again.

Comment 10

[Maurya Talisetti]

Hi Brian,

I'm trying to disable the bookmark button and need your help with it.

In BrowserViewController.observeValueForKeyPath(keyPath:object:change:context), I added an error page check that determines whether the bookmark button should be disabled.

(https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Browser/BrowserViewController.swift#L873),

        case KVOLoading:
            guard let loading = change?[NSKeyValueChangeNewKey] as? Bool else { break }
            toolbar?.updateReloadStatus(loading)
            urlBar.updateReloadStatus(loading)
            if (!loading) { /* added this if condition */
                if let validUrl = tabManager.selectedTab?.url where ErrorPageHelper.isErrorPageURL(validUrl) {
                    navigationToolbar.bookmarkButton.enabled = false
                }
                
                runScriptsOnWebView(webView)
            }


However, this is not resulting in disabling the actions performed by the bookmark button. I'm not worried about greying it yet (I might have to do a button.alpha = 0.66 as per an answer on SO).

I don't see any line of code that changes the bookmarkButton.enabled property to "true" after the KVOLoading switch case is executed. The code execution (when the page is an error page), passes the if condition.

Any suggestions what I might be missing?

Comment 11

[Brian Nicholson (:bnicholson)]

Hi Maurya -- sorry again for the very late feedback. The company was all in London last week, so there weren't many opportunities to catch up with bugs. I promise to be much more responsive from now on!

The bookmark button has been moved to the menu, so you'll want to make your changes in the menu code. Whenever the menu is shown, we create an AppState instance [1] that describes the state of the application, and AppMenuConfiguration then reads that state to build the menu items [2].

To make the menu show a gray bookmark icon for unsupported URLs, you'll want to change the menu bookmark code at [3] to also check whether the current URL is an error page, then return a DisabledBookmarkMenuItem (which you'll have to create yourself) if it is.

Hopes this makes sense. Let me know if anything is unclear!

[1] https://github.com/mozilla/firefox-ios/blob/e95abdc911a4fe683438ace30b081e543bc4e592/Client/Frontend/Browser/BrowserViewController.swift#L1233
[2] https://github.com/mozilla/firefox-ios/blob/e95abdc911a4fe683438ace30b081e543bc4e592/Client/Frontend/Menu/AppMenuConfiguration.swift#L93
[3] https://github.com/mozilla/firefox-ios/blob/e95abdc911a4fe683438ace30b081e543bc4e592/Client/Frontend/Menu/AppMenuConfiguration.swift#L113

Comment 12

[Maurya Talisetti]

Np Brian! I was on a small vacation myself :) Thanks for your suggestions, I'll look thru them.

Comment 13

[Maurya Talisetti]

Brian,

There's one problem with creating a DisabledBookmarkMenuItem. After adding the change, on clicking the disabled icon, the menu disappears immediately [1]. This can cause the user to not realize that the site wasn't bookmarked and also not realize that the action is disabled. Is that ok? I would have rather hoped that clicking on the bookmark button would do nothing AND also keep the menu still visible.

Since a 'nil' action [3] is not allowed as per the MenuItem protocol [2], a MenuAction needs to be created:

    private static var BookmarkDisabledMenuItem: MenuItem {
        return AppMenuItem(title: BookmarkDisabledTitleString, action:  MenuAction(action: AppMenuAction.BookmarkDisabled.rawValue), icon: "menu-Bookmark", privateModeIcon: "menu-Bookmark-pbm")
    }

Making the action property an optional will solve the problem as we will never call the performMenuAction() method [4]. However, I wonder if this is good design for iOS. Making a button not work might make the user think that the touch screen is not responding :) 

Or, do you think it might be better to display an alert saying that bookmarking of error pages is disabled? And then quickly dismiss the menu at the same time.

[1] https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Widgets/Menu/MenuViewController.swift#L185
[2] https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Widgets/Menu/Items/MenuItem.swift
[3] https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Menu/AppMenuConfiguration.swift#L179
[4] https://github.com/mozilla/firefox-ios/blob/e95abdc911a4fe683438ace30b081e543bc4e592/Client/Frontend/Widgets/Menu/MenuViewController.swift#L206

Comment 14

[Brian Nicholson (:bnicholson)]

Robin (our resident UX expert) suggested we disable the button and not have it respond to taps in comment 7, so let's try to follow that. Making the action optional seems reasonable to me -- I can imagine other scenarios where we'd want to disable certain menu buttons, too.

CC'ing Emily since she owns the menu code and might have other thoughts.

Comment 15

[Emily Toop (:fluffyemily)]

My preferred solution would be:

Add a 'disabled' flag to the `MenuItem` and toggle that depending on whether or not the we are offline. Then check the enabled value on each menu item on cell selection and disallow.

To check on disabled, add an implementation for `collectionView(collectionView:shouldSelectItemAtIndexPath indexPath:)` to `MenuView` inside `UICollectionViewDelegateFlowLayout` and an extra protocol method onto `MenuItemDelegate` which will be called from this implementation. Then in `MenuViewController`, implement the new delegate method and return true or false depending on whether the menu item is disabled or not.

Comment 16

[Maurya Talisetti]

Hi Brian,

In the solution proposed in Comment 15 , is there a way of initializing a computed type property by supplying an argument?

In AppMenuConfiguration.swift:
menuItemsForAppState(_:) {
 // ...
             if let validUrl = tabState.url where !ErrorPageHelper.isErrorPageURL(validUrl) {
                menuItems.append(tabState.isBookmarked ? AppMenuConfiguration.RemoveBookmarkMenuItem : AppMenuConfiguration.AddBookmarkMenuItem)
            } else {
                menuItems.append(AppMenuConfiguration.DisabledAddBookmarkMenuItem)
            }
 // ...
}

    private static var DisabledAddBookmarkMenuItem: MenuItem {
        return AppMenuItem(title: AddBookmarkTitleString, action:  MenuAction(action: AppMenuAction.ToggleBookmarkStatus.rawValue), icon: "menu-Bookmark", privateModeIcon: "menu-Bookmark-pbm", isDisabled: true)
    }

Instead of creating another MenuItem for AddBookmark (AddBookmarkMenuItem and DisabledAddBookmarkMenuItem), do you know of any way to may be send an argument while initializing a computed type property?

If that's not possible, I'm wondering if the solution proposed in Comment 14 might be more suitable. Ideally, disabling MenuItem selection would be good. But, if we have to create MenuItems for both enabled and disabled scenarios, we might do enough by just making the MenuItem.action variable an optional.

Comment 17

[Brian Nicholson (:bnicholson)]

(In reply to Maurya Talisetti from comment #16)
> Instead of creating another MenuItem for AddBookmark (AddBookmarkMenuItem
> and DisabledAddBookmarkMenuItem), do you know of any way to may be send an
> argument while initializing a computed type property?

Not during initialization, but you could create a `var disabled: Bool` that you set after creating the item:

    let menuItem = tabState.isBookmarked ? AppMenuConfiguration.RemoveBookmarkMenuItem : AppMenuConfiguration.AddBookmarkMenuItem
    if let validUrl = tabState.url where ErrorPageHelper.isErrorPageURL(validUrl) {
        menuItem.disabled = true
    }
    menuItems.append(menuItem)


With this approach, you'll also want to set a grey color filter on the icons in AppMenuItem.swift if disabled is true.

Comment 18

[Maurya Talisetti]

Thanks Brian! Had some trouble understanding computed properties. I had to use 'var menuItem' instead of 'let menuItem' because it is a struct underneath.

Comment 19

[Maurya Talisetti]

Hi Brian, any suggestions on what we can do to apply color filters? I tried the options in UIImage+ImageEffects.h [1], but the tint/blur is taking the form of a box rather than the shape of the icon (star shape for the bookmark in our case).

[1] https://github.com/mozilla/firefox-ios/blob/f83a82eb3da6e8f9fb04f80cb0c46a8b1e9c4895/ThirdParty/Apple/UIImage+ImageEffects.h#L113

Comment 20

[Brian Nicholson (:bnicholson)]

I think you'll want to use a graphics context to recolor the UIImage. See https://stackoverflow.com/a/19275079, for example.

Comment 21

[Maurya Talisetti]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1984

Comment 22

[Brian Nicholson (:bnicholson)]

Comment on attachment 8767472 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1984

Looking good! Left some comments in the PR.

Comment 23

[Maurya Talisetti]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1984

Hi Brian,

Made changes as per your suggestions.

Comment 24

[Brian Nicholson (:bnicholson)]

Comment on attachment 8774037 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1984

Nice job! One suggestion about tweaking the check to be a little more generic, but other than that, this should be good to go.

Comment 25

[Maurya Talisetti]

Needinfo ping: See https://github.com/mozilla/firefox-ios/pull/1984

Comment 26

[Brian Nicholson (:bnicholson)]

Sorry, missed your question. Thanks for the ping!

Comment 27

[Maurya Talisetti]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1984

Comment 28

[Maurya Talisetti]

Robin, I want to highlight one thing about the solution:
While all else is as expected, in the case when we try to load a bookmarked webpage without an internet connection, for example http://www.google.com, on clicking the menu button on the toolbar, the icon "Remove Bookmark" (disabled) is displayed.

Basically, I was wondering how elegant it is to display "Remove Bookmark" when the user actually can't perform that action because it is disabled. The behavior happens because on every page load we

Comment 29

[Brian Nicholson (:bnicholson)]

Comment on attachment 8792321 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1984

Looks good to me. Thanks again!

Comment 30

[Maurya Talisetti]

Hi Brian,

Could you merge the PR?
Do you think there's a concern with the issue in Comment 28, or should we check with Robin?

Comment 31

[Maurya Talisetti]

Attachment: Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/1984

Updated PR to remove merge conflicts.

Comment 32

[Brian Nicholson (:bnicholson)]

Comment on attachment 8795776 [details] [review]
Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/1984

I think this is fine for now, but I'll leave the NEEDINFO open for Robin in case she gets a chance to look.

Looks good, and thanks again for the PR!

Comment 33

[Brian Nicholson (:bnicholson)]

https://github.com/mozilla-mobile/firefox-ios/commit/a747be6f892a1bc81d41d13f7d8f34dc5bab66cb