Closed Bug 1123904 Opened 9 years ago Closed 9 years ago

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

Categories

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

35 Branch
All
Android
defect
Not set
normal

Tracking

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

VERIFIED FIXED
Firefox 38
Tracking Status
firefox36 --- unaffected
firefox37 + verified
firefox38 + verified
fennec 37+ ---

People

(Reporter: Margaret, Assigned: mcomella)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
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: --- → ?
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)
/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)
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.
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?
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.
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)
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+
Verified as fixed in Firefox 37 Beta 1;
Device: Samsung Galaxy R (Android 2.3.4).
Status: RESOLVED → VERIFIED
Attachment #8566846 - Attachment is obsolete: true
Attachment #8619180 - Flags: review+
Attachment #8619181 - Flags: review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: