Long pressing a link in reader view displays clobbered link information

VERIFIED FIXED in Firefox 62

Status

()

P3
normal
VERIFIED FIXED
2 years ago
3 months ago

People

(Reporter: kaartic, Assigned: rpolyano)

Tracking

({good-first-bug})

54 Branch
Firefox 62
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox62 verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
Created attachment 8844563 [details]
"Showing offline version" message shown for pages loaded from local storage
Duplicate of this bug: 1345193
Component: General → Reader View
Keywords: good-first-bug
Priority: -- → P3
(Assignee)

Comment 3

8 months ago
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
(Assignee)

Comment 4

8 months ago
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.
(Assignee)

Comment 5

6 months ago
Created attachment 8957609 [details] [diff] [review]
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.
Flags: needinfo?(cnevinchen)
Assignee: nobody → rpolyano
Flags: needinfo?(cnevinchen)
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!
Attachment #8957609 - Flags: review?(michael.l.comella)
btw, please use "need info" and choose someone so (s)he can get notified. Thanks for the patch!
Flags: needinfo?(kaarticsivaraam91196)
(Reporter)

Comment 8

6 months ago
(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)).
Flags: needinfo?(kaarticsivaraam91196) → needinfo?(rpolyano)
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.
Attachment #8957609 - Flags: review?(michael.l.comella) → review?(nalexander)
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.
Attachment #8957609 - Flags: review?(nalexander) → feedback+
(Assignee)

Comment 11

5 months ago
Created attachment 8971453 [details] [diff] [review]
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?
Flags: needinfo?(rpolyano) → needinfo?(nalexander)
Attachment #8971453 - Flags: feedback?(nalexander)
(Assignee)

Updated

5 months ago
Attachment #8971453 - Flags: feedback?(nalexander) → review?(nalexander)
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!
Attachment #8971453 - Flags: review?(nalexander) → review+
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
I'll mark this checkin-needed after the fresh patch is up.  Thanks Roman!
Flags: needinfo?(nalexander) → needinfo?(rpolyano)
(Assignee)

Comment 15

4 months ago
Created attachment 8974383 [details] [diff] [review]
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!
Attachment #8957609 - Attachment is obsolete: true
Attachment #8971453 - Attachment is obsolete: true
Flags: needinfo?(rpolyano) → needinfo?(nalexander)
Attachment #8974383 - Flags: review?(nalexander)
(Assignee)

Updated

4 months ago
Attachment #8974383 - Attachment description: Revised commit message → Fixed bug with correct API call and revised commit message
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!
Attachment #8974383 - Flags: review?(nalexander) → review+
Flags: needinfo?(nalexander)
Keywords: checkin-needed

Comment 17

4 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57029c5f35da
Don't show "about:reader?" in the long-press context menu. r=nalexander
Keywords: checkin-needed

Comment 18

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/57029c5f35da
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62

Comment 19

4 months ago
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.
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
You need to log in before you can comment on or make changes to this bug.