Closed
Bug 1123904
Opened 10 years ago
Closed 10 years ago
"about:reader" URL in toolbar isn't user-friendly
Categories
(Firefox for Android Graveyard :: Reader View, defect)
Tracking
(firefox36 unaffected, firefox37+ verified, firefox38+ verified, fennec37+)
VERIFIED
FIXED
Firefox 38
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.
Reporter | ||
Comment 2•10 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•10 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 | ||
Updated•10 years ago
|
Assignee: margaret.leibovic → michael.l.comella
Flags: needinfo?(michael.l.comella)
Assignee | ||
Comment 4•10 years ago
|
||
/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•10 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 6•10 years ago
|
||
Reporter | ||
Comment 7•10 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+
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
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?
Updated•10 years ago
|
status-firefox36:
--- → unaffected
status-firefox37:
--- → affected
status-firefox38:
--- → affected
tracking-firefox37:
--- → +
tracking-firefox38:
--- → +
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Comment 13•10 years ago
|
||
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)
Updated•10 years ago
|
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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 17•10 years ago
|
||
Comment 18•10 years ago
|
||
Verified as fixed in Firefox 37 Beta 1;
Device: Samsung Galaxy R (Android 2.3.4).
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 19•9 years ago
|
||
Attachment #8566846 -
Attachment is obsolete: true
Attachment #8619180 -
Flags: review+
Attachment #8619181 -
Flags: review+
Assignee | ||
Comment 20•9 years ago
|
||
Assignee | ||
Comment 21•9 years ago
|
||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•