1419520 - Reader mode can't be opened if tab is not considered an article

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[fkohrt]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20171114221957

Steps to reproduce:

On Firefox 58 Developer Edition (add-on debugging turned on):
1. Open a new  website that is not considered as an article by Firefox
 - example site: https://www.nextplatform.com/2016/03/22/decade-container-control-google/
2. Use `tabs.toggleReaderMode()` to open the reader mode for that tab
 - for example using this add-on: https://addons.mozilla.org/firefox/addon/activate-reader-view/


Actual results:

Debug console says:
"Error: The specified tab cannot be placed into reader mode."


Expected results:

The tab should have been placed in reader mode.

As already explained [1], I expect the API to not care about if the website is an article or not. This is because the heuristic for "pages that work for the reader mode" isn't perfect and will always lead to false negatives (see [2] for an old, but still unfixed example). Currently, `tabs.toggleReaderMode(tabId)` does something like this:

> if (!tab.isInReaderMode && !tab.isArticle) {
>   throw new ExtensionError("The specified tab cannot be placed into reader mode.");
> }

I believe that when a user tries to use some extra feature (the reader mode) in the regular way (by clicking the button next to the address bar), the platform (Firefox) should check if the thing the user wants will work - if that makes life easier for him. But in case he has certain reasons to believe the platform gets it wrong and makes life actually harder, such preemptive checks (i.e. `!tab.isArticle`) must be circumventable - so add-ons can become the "checking authority" for users who want them to.

Links
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1286387#c16
[2] https://github.com/mozilla/readability/issues/284

Comment 1

[Bob Silverberg [:bsilverberg]]

I did some testing and it seems like trying to toggle reader mode for a non-readerable tab, via sendAsyncMessage("Reader:ToggleReaderMode") doesn't cause any serious problems. I just see a message in the browser console from tab-content.js, line 266: "TypeError: ReaderMode.parseDocument(...) is null". So perhaps it's fine to remove the check we have in toggleReaderMode which ensures that a tab has isArticle: true.

What do you think, Gijs? Are there any issues with an extension trying to toggle reader mode on non-readerable pages?

Comment 2

[:Gijs (he/him)]

(In reply to Bob Silverberg [:bsilverberg] from comment #1)
> I did some testing and it seems like trying to toggle reader mode for a
> non-readerable tab, via sendAsyncMessage("Reader:ToggleReaderMode") doesn't
> cause any serious problems. I just see a message in the browser console from
> tab-content.js, line 266: "TypeError: ReaderMode.parseDocument(...) is
> null". So perhaps it's fine to remove the check we have in toggleReaderMode
> which ensures that a tab has isArticle: true.
> 
> What do you think, Gijs? Are there any issues with an extension trying to
> toggle reader mode on non-readerable pages?

I think it would be OK as long as we either:

1) provided an error response to the extension if it failed (probably preferable)
or
2) showed the dreaded 'couldn't find an article on this page' error in content

Would that make sense?

Comment 3

[Bob Silverberg [:bsilverberg]]

(In reply to :Gijs from comment #2)
> 
> I think it would be OK as long as we either:
> 
> 1) provided an error response to the extension if it failed (probably
> preferable)
> or
> 2) showed the dreaded 'couldn't find an article on this page' error in
> content
> 
> Would that make sense?

Thanks Gijs. I agree that #1 would be preferable and makes sense to do. Looking at the code, we're currently using tab.linkedBrowser.messageManager.sendAsyncMessage() to send the message to the content process. How would we go about being informed if there was an error during the processing of the message in tab-content.js? Right now it's just output via Cu.reportError, so I gather something different would need to happen there, but I'm not sure how the extension process can know about what happens.

Comment 4

[:Gijs (he/him)]

(In reply to Bob Silverberg [:bsilverberg] from comment #3)
> (In reply to :Gijs from comment #2)
> > 1) provided an error response to the extension if it failed (probably
> > preferable)
> 
> Thanks Gijs. I agree that #1 would be preferable and makes sense to do.
> Looking at the code, we're currently using
> tab.linkedBrowser.messageManager.sendAsyncMessage() to send the message to
> the content process. How would we go about being informed if there was an
> error during the processing of the message in tab-content.js? Right now it's
> just output via Cu.reportError, so I gather something different would need
> to happen there, but I'm not sure how the extension process can know about
> what happens.

I think that really, most of this stuff needs to move to the content process. However, you'd still have the same issue in that you'd need to get a result from the content process after toggling reader mode in the parent process. So I guess we'd need to add a new message manager message that reports failure/success back from the content process? Success is kinda already detectable because the tab will navigate to about:reader (so I guess the add-on could do something like wait 10s for navigation and then go "uh, maybe it didn't work"), but that's not very ergonomic for the consumer of the WE API.

Comment 5

[Andy McKay]

We've had a history of extensions papering over bugs in Firefox by using extensions to fix things. It seems like we are devoting time to add a feature where we should be in spending the time to fix readability. If an extension wants to do it's own thing with the page, then the extension can do that (this is how Chrome add-ons implement reader mode).

I'd vote for won't fixing this and instead asking for issue 284 to be worked on.

Comment 6

[:Gijs (he/him)]

(In reply to Andy McKay [:andym] from comment #5)
> We've had a history of extensions papering over bugs in Firefox by using
> extensions to fix things. It seems like we are devoting time to add a
> feature where we should be in spending the time to fix readability.

I disagree on 2 counts:

1) Fixing readability is not as easy as all that, unfortunately. The check "is this readerable" happens by analyzing the DOM when the page is rendered. We pretty much want an answer when the page shows up to avoid a situation where the button isn't there when users look for it (but appears "later"). That means doing it on page load. That takes time. The more elaborate the detection, the more time it takes. That time impacts page load times (a lot, actually, if run on every page load). We ran into this when we initially implemented reader mode in Firefox - it regressed talos pageload times.

So the algorithm we use to check "can we run reader mode on this" is deliberately very lightweight, because we obviously don't want to spend 50ms every page load working out if we can run reader mode on it and get a reasonable result (never mind that, if you're not careful, the 50ms scales with the size of the DOM, so imagine what happens if you load the whatwg single-page spec...).

Which isn't to say that I would say no to a trivial patch to fix https://github.com/mozilla/readability/issues/284 - quite the opposite! - but I'm not convinced it would be trivial to fix without regressing perf, because pretty much all added complexity in that thing regresses perf.

2) I don't think that would /really/ address this bug. The way I read comment #0, this particular website is just an example. There are add-ons that are deliberately designed to "auto-trigger" reader mode. People running some of these add-ons basically render anything that renders in reader mode, in reader mode, and (apparently) don't care about the performance impact this has (or the page usability decreasing, when e.g. transforming pages on BMO or youtube into reader mode - or perhaps the add-on implements its own list of sites where it doesn't activate reader mode, I dunno). Either way, I think there are legitimate usecases for wanting an add-on that does this, that aren't necessarily easily addressed by "just fixing readability" (without regressing things for the majority of users who don't frequently use reader mode).



Alternatively, someone can design a system where we can easily run the entirety of 'toggle reader mode' post-pageload on a separate thread (which involves stringifying the DOM and sending it across... :-( ) so we "mostly" don't interfere with the performance of websites, and then we can do that instead of the cheaper approximation we do now. But that, too, is not trivial...

Comment 7

[Bob Silverberg [:bsilverberg]]

I tend to agree with Gijs that this is something we should allow WebExtensions to do, and it doesn't sound like a terribly complicated fix (to introduce a message from the content process to the extension process). I'm going to add this to the API triage so we can discuss it with the team.

Comment 8

[Caitlin Neiman [:caitmuenster]]

Hi fkohrt and Gijs, this has been added to the agenda for the December 5 Webextensions APIs triage meeting. Would you be able to join us? 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/WebExtensions/Triage#Next_Meeting
* Meeting agenda: https://docs.google.com/document/d/1vH4wqJJZt1jk-cpx5NOq67b1UNekeQog6bz2HFOHe5E/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision

Comment 9

[fkohrt]

Sure, I can join via IRC.

Comment 10

[:Gijs (he/him)]

I likely can't make it at that specific time. If I can, it will be only for the first 20-ish minutes. Andy, can you or Mike or whoever else would be suitable perhaps pre-empt at least some of the questions you would want answered? Happy to reply via email or on the bug.

Comment 11

[Andy McKay]

I'm good with the comments so far. It seems weird to knowingly provide the option to go into reader mode without knowing that it will work, but if that's people want to do, sure - not a biggie.

Comment 12

[Bob Silverberg [:bsilverberg]]

This has been approved, so I will start working on it sometime soon. We will return information from the call to tabs.toggleReaderMode() which will let the extension know whether something went wrong during the process. I think we can simply return either true (it worked) or false (it didn't).

Comment 13

[Wladimir Palant]

Quite frankly, the current API is inconsistent. One can call tabs.create({openInReaderMode: true}) and the tab will open in reader mode no matter what. And sure enough, in some cases this means seeing "Failed to load article from page" error but that's something for extension's author to worry about. I would expect tabs.update() to also take openInReaderMode parameter and allow replacing a tab by the reader view of either the same or a different URL - obviously, without additional checks, consistent with how tabs.create() works. Instead, there is tabs.toggleReaderMode() method which appears to do extra checks. That's just unnecessary.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.