1345199 - Long pressing a link in reader view displays clobbered link information

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect, P3)

Description

[Kaartic Sivaraam]

When a user long presses a link to another part of the page in reader view, the URL of the reader article is shown instead of the "title" portion (or) "href" portion of the link (I guess that's what would be displayed). For example, if there is a link in the page like the one below in a page found at "http://example.com/page.html"

<a href="#title1" title="A Link"> Hyperlink </a>

When a user long presses the "Hyperlink" when in reader view, the following is displayed in the menu instead of the "href" or the "title" potion of the text.

about:reader?url=http%3A%2F%2Fexample.com%2Ftitle.html...

It would be better if the title or href portion of the link were displayed instead of this. Moreover it exposes the user to the workings of Firefox (about:reader?). 

Steps to reproduce:
1. Visit any page that has links to other parts of the same page (e.g.) https://en.wikipedia.org/wiki/Generic_programming
2. Enable Reader mode for that page
3. Long press the first reference ([1]) found in the main section of the page

Expected results:
The "title" or "href" portion of the link is displayed

Actual results:
URL starting with about:reader? is displayed

Comment 1

[Kaartic Sivaraam]

Attachment: "Showing offline version" message shown for pages loaded from local storage

Comment 3

[Roman Polyanovsky]

Hello,

I'd like to work on this as a first bug. I've been able to reproduce it just now in Firefox 58.0.1

Comment 4

[Roman Polyanovsky]

I've investigated this some and there are two places that seem like it can be related to:

It can be easily checked for (even generically for "about:*" with a "url" parameter) in android/chrome/content/browser.js in NativeWindow.contextmenus._getLinkURL

or I can try to chase it in

netwerk/base/nsIOService::NewURI(...) where whatever is handling it seems to be setting displaySpec incorrectly.

I'd appreciate any guidance here, but I'll keep looking for now.

I also haven't been assigned so if there's anything I need to do on that front, please let me know.

Comment 5

[Roman Polyanovsky]

Attachment: 1345199-reader-v0.patch

I've made an initial attempt at fixing this, although I'm skeptical that this is the right approach. I would really appreciate some feedback and pointers to where to go from here.

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8957609 [details] [diff] [review]
1345199-reader-v0.patch

Review of attachment 8957609 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not working on Fennec now. But I'd personally want this fix on Android side.
Hi Michael 
Could you please help me redirect to someone to review this patch?

Thank you!

Comment 7

[Nevin Chen(Not active on Bugzilla)]

btw, please use "need info" and choose someone so (s)he can get notified. Thanks for the patch!

Comment 8

[Kaartic Sivaraam]

(In reply to Nevin Chen [:nechen] from comment #7)
> btw, please use "need info" and choose someone so (s)he can get notified.

Seems you're pinging the wrong person ? I guess you should have pinged "Roman Polyanovsky"

> Thanks for the patch!

This should go to the right person (the one currently being pinged (needinfo, to be exact)).

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8957609 [details] [diff] [review]
1345199-reader-v0.patch

Review of attachment 8957609 [details] [diff] [review]:
-----------------------------------------------------------------

I generally don't do Fennec reviews: giving to Nick.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8957609 [details] [diff] [review]
1345199-reader-v0.patch

Review of attachment 8957609 [details] [diff] [review]:
-----------------------------------------------------------------

Hi!  This is a good approach, but I see there's a slightly more "official" approach used on desktop -- see https://searchfox.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#1084.  Can you see if using `ReaderMode.getOriginalUrlObjectForDisplay` works for you?

And sorry for the long delay reviewing -- we're short-staffed for Fennec right now.  Flag me directly (r?nalexander) next time and I'll turn it around more quickly.

Comment 11

[Roman Polyanovsky]

Attachment: Revised attempt to fix the bug

Hi Nick,

Thanks for the feedback. I changed it to use the API you noted. Does this look better?

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8971453 [details] [diff] [review]
Revised attempt to fix the bug

Review of attachment 8971453 [details] [diff] [review]:
-----------------------------------------------------------------

Nifty!  Sorry for the delayed review.  This looks good to me!

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8971453 [details] [diff] [review]
Revised attempt to fix the bug

Review of attachment 8971453 [details] [diff] [review]:
-----------------------------------------------------------------

Nifty!  Sorry for the delayed review.  This looks good to me!

This needs a fresh commit message, perhaps:

Bug 1345199 - Don't show about:reader? in long-press context menu. r=nalexander

Comment 14

[Nick Alexander :nalexander [he/him]]

I'll mark this checkin-needed after the fresh patch is up.  Thanks Roman!

Comment 15

[Roman Polyanovsky]

Attachment: Fixed bug with correct API call and revised commit message

Hi Nick,

I've created a new commit message as specified. I assume you intended the r=nalexander bit to be part of the commit message as well.

Thanks for your help with the process!

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8974383 [details] [diff] [review]
Fixed bug with correct API call and revised commit message

Review of attachment 8974383 [details] [diff] [review]:
-----------------------------------------------------------------

Yup!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57029c5f35da
Don't show "about:reader?" in the long-press context menu. r=nalexander

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/57029c5f35da

Comment 19

[Stefan Deiac]

Verified as fixed on latest Nightly buid(62.0a1 2018-05-31). 
Devices: Sony Xperia Z5 Premium (Android 6.0.1) and Motorola Nexus 6 (Android 7.1.1).
"about:reader?" is not displayed anymore following steps from description.