"about:reader" URL in toolbar isn't user-friendly

VERIFIED FIXED in Firefox 37

Status

()

Firefox for Android
Reader View
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: Margaret, Assigned: mcomella)

Tracking

(Blocks: 1 bug)

35 Branch
Firefox 38
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 unaffected, firefox37+ verified, firefox38+ verified, fennec37+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
After landing bug 1111729, we now default to showing the URL in the toolbar, and for reader mode, this looks pretty ugly.

This is also an issue that affects tablets and desktop, since we always show the URL there as well. I'll file a bug about that as well.

It might be worthwhile to try to avoid users ever seeing an "about:reader" URL, keeping it as more of an implementation detail than anything else. However, it's also a pain to try to special-case a certain type of URL.
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1134274
(Reporter)

Comment 2

3 years ago
Shoot, we probably want this to track 37, since that's where bug 1111729 landed.

I fixed this for desktop in bug 1123910 by just showing the original URL, not the about:reader URL. For consistency, I think we should do that here as well.
Assignee: nobody → margaret.leibovic
tracking-fennec: --- → ?
(Reporter)

Comment 3

3 years ago
Mike, I have a lot on my plate right now, would you be willing to take a look at this? I think it will mostly involve making a check on the Java side to show the original URL in the urlbar instead of the about:reader URL (we already have some helper methods that parse the URL out for things like the two line page row items in the reading list panel).
tracking-fennec: ? → 37+
Flags: needinfo?(michael.l.comella)
Assignee: margaret.leibovic → michael.l.comella
Flags: needinfo?(michael.l.comella)
Created attachment 8566846 [details]
MozReview Request: bz://1123904/mcomella

/r/4059 - Bug 1123904 - Display original title in toolbar in reader mode. r=margaret
/r/4061 - Bug 1123904 - Provide url highlighting in reader mode. r=margaret

Pull down these commits:

hg pull review -r df3b84ce791fc11f623cbb548bfed2e4a8fb948c
Attachment #8566846 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 5

3 years ago
https://reviewboard.mozilla.org/r/4061/#review3249

::: mobile/android/base/Tab.java
(Diff revision 1)
> +    /**

Thanks for documenting this.

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> +    // From ReaderParent._getOriginalUrl (browser/modules/ReaderParent.jsm).

I wonder if it would be worth pulling this into a shared helper method in ReaderMode.jsm. But this is fine for right now.
(Reporter)

Comment 7

3 years ago
Comment on attachment 8566846 [details]
MozReview Request: bz://1123904/mcomella

https://reviewboard.mozilla.org/r/4057/#review3253

Ship It!
Attachment #8566846 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8566846 [details]
MozReview Request: bz://1123904/mcomella

Approval Request Comment
[Feature/regressing bug #]: Default to show url, bug 1111729

[User impact if declined]:
  URLs in the toolbar in reader mode will look like `about:reader?url=...`, which looks bad to users (i.e. polish).

[Describe test coverage new/current, TreeHerder]:
  None, filed bug 1135111 as a mentor bug.

[Risks and why]: 
  Worst case, we mangle the url text or highlighting. However, we use some already tested methods so this is unlikely. Relatively low risk.

[String/UUID change made/needed]: None
Attachment #8566846 - Flags: approval-mozilla-aurora?
status-firefox36: --- → unaffected
status-firefox37: --- → affected
status-firefox38: --- → affected
tracking-firefox37: --- → +
tracking-firefox38: --- → +
I'd like to take this fix before the uplift to Beta but given that the fix doesn't look trivial and there is no test coverage I want to see the fix verified on Nightly first. Assuming the fix hits m-c today, that should be doable over the weekend.
Technically this affects 36 too, but it's much more noticeable after bug 1111729 landed in 37.
https://hg.mozilla.org/mozilla-central/rev/5e5861f36aaf
https://hg.mozilla.org/mozilla-central/rev/9a4e71f96af9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Created attachment 8567808 [details]
Screenshot_2015-02-23-10-12-40.png

The original URL is displayed, not the about:reader URL
Tested with:
Device: Nexus 4 (Android 4.4)
Build: Firefox for Android 38.0a1 (2015-02-22)
status-firefox38: fixed → verified
Reminder on uplift (re comment 10).
Flags: needinfo?(lmandel)
I was waiting for this to have a couple of days on m-c first. This will be uplifted post merge in time for Beta 1.
Flags: needinfo?(lmandel)
Comment on attachment 8566846 [details]
MozReview Request: bz://1123904/mcomella

I verified that the original page URL is now shown in Nightly 38.0a1 (2015-02-23) on a Nexus 5. 37 is now Beta so this is now a Beta uplift request. I have cleared the Aurora request and am approving for Beta.

Beta+
Attachment #8566846 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+

Comment 18

3 years ago
Verified as fixed in Firefox 37 Beta 1;
Device: Samsung Galaxy R (Android 2.3.4).
Status: RESOLVED → VERIFIED
status-firefox37: fixed → verified
Comment on attachment 8566846 [details]
MozReview Request: bz://1123904/mcomella
Attachment #8566846 - Attachment is obsolete: true
Attachment #8619180 - Flags: review+
Attachment #8619181 - Flags: review+
Created attachment 8619180 [details]
MozReview Request: Bug 1123904 - Display original title in toolbar in reader mode. r=margaret
Created attachment 8619181 [details]
MozReview Request: Bug 1123904 - Provide url highlighting in reader mode. r=margaret
You need to log in before you can comment on or make changes to this bug.