1322117 - Reader Mode tab shows up in Recently Closed Tabs

Status: VERIFIED FIXED

(Firefox for iOS :: General, defect, P1)

Description

[Maurya Talisetti]

Attachment: Recently Closed Tabs

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161129173726

Steps to reproduce:

1. Navigate to any website that can be viewed in Reader Mode (e.g., wikipedia article)
2. View it in reader mode
3. Close the tab
4. Now check Recently Closed Tabs under History


Actual results:

The localhost url to the page that was viewed in reader mode is displayed among the recently closed tabs. On tapping that item, a blank page is shown. See screenshots.


Expected results:

The actual url (not the localhost url) should be shown among the recently closed tabs. On tapping, it should navigate to that page (not to a blank page).

Comment 1

[Maurya Talisetti]

Correction: Tapping on the item takes us to the reader view page (not  blank page). But, we want it to take us to the actual page (without the reader view applied).

Comment 2

[Maurya Talisetti]

I've noticed that even other "localhost" pages are also being displayed in the Recently Closed Tabs panel. For instance if a page timed out, an entry shows up in the panel with the title "The request timed out." and with a "localhost" url. I believe we don't want to show localhost urls anywhere.

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: PR: https://github.com/mozilla-mobile/firefox-ios/pull/2283

This patch does the following:

* It moves helper functions from `AboutUtils`, `ReaderModeUtils` and `ErrorPageHelper` into `NSURLExtensions`.
* It introduces a `NSURL.displayURL` and `NSURL.displayString` which correctly transform about, error and reader mode URLs to the page that they represent.
* It fixes bug 1322117 by calling `url.displayString()` instead of `url.absoluteString()` in the `RecentlyClosedTabsPanel`.

I understand this turned out to be a rather big refactor. We can also move this to 6.1 or 7.0 if we have a bad feeling about a change this large.

Comment 4

[Farhan Patel [:farhan]]

Comment on attachment 8819004 [details] [review]
PR: https://github.com/mozilla-mobile/firefox-ios/pull/2283

I'm cool with this going into 6.0.

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8819004 [details] [review]
PR: https://github.com/mozilla-mobile/firefox-ios/pull/2283

Left some comments in the PR, but nothing too big. Would be nice if these extension functions were computed properties instead.

Comment 6

[Farhan Patel [:farhan]]

I added a lot more changes. If you could do another look that would be awesome :)

Comment 7

[Brian Nicholson (:bnicholson)]

Mostly LGTM -- left a few comments in the PR.

Comment 8

[Farhan Patel [:farhan]]

master https://github.com/mozilla-mobile/firefox-ios/commit/1326872c53591075723896437e8a88658e10e94f

Comment 9

[Farhan Patel [:farhan]]

make sure to also uplift the folowup please

b28cdb50962ecfc35b22c833cfa6b1506a165a0e

Comment 10

[Farhan Patel [:farhan]]

v6.x 182fe46a9b060c40c43c7ab3871c3d7e8e5d1d54

Comment 11

[Catalin Suciu [:csuciu]]

On latest master, the URL is show correctly in the 'Recently Closed' panel, but tapping on it takes you to the reader view page. 
Per comment #1, tapping in the link should take you to the actual page.

Farhan, what's the expected behavior in this case?

Comment 12

[Farhan Patel [:farhan]]

This was the behavior I was going for. It is also similar to how the back/forward list behaves. In the future it would be a good way to show that the URL is a ReaderMode url.

Comment 13

[Catalin Suciu [:csuciu]]

Verifying as fixed on 6.0b9