Closed Bug 1128757 Opened 5 years ago Closed 5 years ago

Reader mode button appears but redirects out when there is a fragment in the URL

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
fennec 38+ ---

People

(Reporter: bgstandaert, Assigned: Margaret)

References

()

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150202030232

Steps to reproduce:

Open a webpage this affects - example is https://bugzilla.mozilla.org/enter_bug.cgi#h=dupes|Firefox. Click on the reader mode button in the address bar.


Actual results:

There is no actual reader view. The about:reader page just redirects back to the non-reader mode page.


Expected results:

The button should not display when there is no reader mode available for the page. Either the about:reader page should show an actual reader mode, or the reader mode button should not appear at all.
Component: Untriaged → Reader Mode
Product: Firefox → Toolkit
Version: Firefox 38 → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
I can reproduce this. I suspect the problem here probably has to do with creating the url parameter in the about:reader URL. What's likely happening is that there *is* a reader mode article, but that when we load about:reader, we run into some issue with the URL.

I can investigate.
Assignee: nobody → margaret.leibovic
I can reproduce this with any URL containing a fragment e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1128757#c0
Hardware: x86 → All
Summary: reader mode button appearing when no reader view → Reader mode button appears but redirects out when there is a fragment in the URL
Oh, maybe this is the same issue as bug 1135084.

Sorry I haven't had time to dig into this yet :(
Duplicate of this bug: 1135084
What's happening here is that we drop the ref part of the article url:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm?force=1#223

So when we do a comparison of the current url to the saved article's url to make sure we're loading the correct thing, we find they are not equal, and we think we didn't find an article.

I have a patch that will fix this issue as we see it now, but this actually exposes a bigger problem for articles like the one I saw in bug 1135084:
http://m.thestar.com/#/article/news/gta/2015/02/19/horror-hope-and-heartbreak-for-our-lost-child-elijah-keenan.html

In this case, the ref actually influences the content of the page (/me shakes fist at web developers), and what's extra unfortunate is that we're using this stripped URL for caching content:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm?force=1#156

This makes me think that the real fix here is to get rid of any .specIgnoringRef calls, and just pass around complete URLs as we see them.

The unfortunate thing here would be that https://bugzilla.mozilla.org/show_bug.cgi?id=1128757#c0 and https://bugzilla.mozilla.org/show_bug.cgi?id=1128757#c1 would be cached as different articles, but we only actually use this cache when you save URLs to it, and if you save both of those URLs, I'm fine with us just caching that content twice.

rnewman/bnicholson, what do you two think we should do here?
Flags: needinfo?(rnewman)
Flags: needinfo?(bnicholson)
OS: Mac OS X → All
/r/4391 - Bug 1128757 - Trim original url before trying to get article for url

Pull down this commit:

hg pull review -r cde1276a9f38fe14e33a35e6eefa556cb8d5f254
(In reply to :Margaret Leibovic from comment #6)
> Created attachment 8570157 [details]
> MozReview Request: bz://1128757/margaret
> 
> /r/4391 - Bug 1128757 - Trim original url before trying to get article for
> url
> 
> Pull down this commit:
> 
> hg pull review -r cde1276a9f38fe14e33a35e6eefa556cb8d5f254

This is my WIP that fixes toggling reader view from the toolbar button, but we should think about whether or not this is the correct approach.
(In reply to :Margaret Leibovic from comment #5)

> This makes me think that the real fix here is to get rid of any
> .specIgnoringRef calls, and just pass around complete URLs as we see them.
...
> rnewman/bnicholson, what do you two think we should do here?

We have the DOM, so in theory we can determine whether the fragment identifier refers to an identifier that exists in the page.

I'd err on the side of keeping the entire URL unchanged, but if we ever discard it, that knowledge might help.
Flags: needinfo?(rnewman)
I agree, I'd prefer potentially duplicate saved articles over broken ones.
Flags: needinfo?(bnicholson)
Comment on attachment 8570157 [details]
MozReview Request: bz://1128757/margaret

/r/4391 - Bug 1128757 - Do not trim fragments from URLs loaded in reader view. r=bnicholson

Pull down this commit:

hg pull review -r 9b50a3cb5b00aefe875935e5cad136879b6f8271
Attachment #8570157 - Flags: review?(bnicholson)
This change depends on bug 1134443.
Depends on: 1134443
tracking-fennec: --- → 38+
Comment on attachment 8570157 [details]
MozReview Request: bz://1128757/margaret

https://reviewboard.mozilla.org/r/4389/#review3749

Ship It!
Attachment #8570157 - Flags: review?(bnicholson) → review+
One thing to consider here is that we may now end up with an inconsistency between article URLs in the reading list and URLs for articles stored in the cache... However, I *think* it will be okay because we were stripping the fragment from the article URL before sending the article over to be stored in Java, so in theory what is stored in the cache will match the article that is stored in the content provider.

I'll think about this a bit more. In the worst case, we wouldn't find an article in the cache for an article, and we may end up with orphan articles in the cache. But hopefully bug 1126244 can help deal with that.
https://hg.mozilla.org/mozilla-central/rev/70ba3108e0fa
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Duplicate of this bug: 1135197
Attachment #8570157 - Attachment is obsolete: true
Attachment #8619308 - Flags: review+
You need to log in before you can comment on or make changes to this bug.