Ability to tell if a tab is in reader mode and to move and tab in and out of reader mode

RESOLVED FIXED in Firefox 58

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: Andy McKay, Assigned: bsilverberg)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla58
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 months ago
It would be nice if you could tell if a tab is in reader mode or not. We could add that into https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab and also trigger the update event each time the reader mode status changes: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onUpdated
This information should already be in the url and update event already, is there something more to this?
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → WONTFIX
(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> This information should already be in the url and update event already, is
> there something more to this?

I think for compatibility, we shouldn't expose URLs like about:reader?url=foo, we should just expose "foo" and then expose the reader mode bit separately, also because other browsers (e.g. edge) use a different way of expressing reader mode (special read protocol, ie read:foo). Would that make sense?
Flags: needinfo?(mixedpuppy)
A read protocol would be nice.  bsilverberg will be looking at reader mode soon, redirecting.
Flags: needinfo?(mixedpuppy) → needinfo?(bob.silverberg)
(Reporter)

Comment 4

9 months ago
It might be nice to work in conjunction with bug 1371793. Because you can't just take the about:reader? URL from tabs.query and pass it through to tabs.create at this time. Re-opening based on the last two comments.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Comment 5

9 months ago
Thanks for the feedback, Shane. I had mentioned this in my "intent to implement" email as well. I feel like transitioning to a read: protocol is outside the scope of creating this API, but if we do implement this feature then a change from about:reader to a read: protocol won't have any effect on consumers of the API.
Flags: needinfo?(bob.silverberg)
(Assignee)

Updated

9 months ago
Assignee: nobody → bob.silverberg
(Assignee)

Comment 6

9 months ago
I am adding the ability to put a tab into reader mode and to take a tab out of reader mode into the scope of this bug.
Summary: Ability to tell if a tab is in reader mode → Ability to tell if a tab is in reader mode and to move and tab in and out of reader mode
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1371801
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I'm thinking about how this can be done without loading ReaderMode.jsm into the extension content since it's already loaded into tab-content.  This is what I am thinking.

in tab-content.js add the ability to query the info we need.  In AboutReaderListener, listen for Reader:IsReadable which would respond with something like:

sendAsyncMessage("Reader:Readable", { isArticle: ReaderMode.isProbablyReaderable(content.document) });

isArticle is following the convention for other messages there.

I think we can live with just a toggle, since we have isInReaderMode as well.  tab-content already supports a toggle message, so we can just send that.

This should remove any need to modify ExtensionContent.jsm or extension-process-script.js

What do you think?
Flags: needinfo?(bob.silverberg)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8906693 - Attachment is obsolete: true
Attachment #8906693 - Flags: review?(mixedpuppy)

Comment 12

9 months ago
mozreview-review
Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review183972

::: browser/base/content/tab-content.js:290
(Diff revision 2)
> +
> +      case "Reader:IsAboutReader":
> +        sendAsyncMessage(
> +          "Reader:aboutReader",
> +          { isAboutReader: this.isAboutReader });
> +        break;

I'm thinking we just combine these into one.

"Reader:ReaderState":

if isAboutReader
  return {isArticle:true,isAboutReader:true}
else if isProbablyReaderable:
  return {isArticle:true,isAboutReader:false}
return {isArticle:false,isAboutReader:false}
Comment hidden (mozreview-request)

Comment 14

9 months ago
mozreview-review
Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review184016

::: browser/base/content/tab-content.js:283
(Diff revision 3)
> +
> +      case "Reader:GetReaderState":
> +        let response = { isInReaderMode: this.isAboutReader };
> +        response.isArticle = response.isInReaderMode
> +          ? true
> +          : ReaderMode.isProbablyReaderable(content.document);

|| instead of ternary
Attachment #8906692 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 16

9 months ago
mozreview-review-reply
Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review184016

> || instead of ternary

Nice. :)

Comment 17

9 months ago
mozreview-review
Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review184080

I apologize for the driveby, but I think we can do this without adding an extra message to tab-content etc.

Note also that this doesn't seem to implement the same API on android. Do we intend to support it there, too?

::: browser/base/content/tab-content.js:281
(Diff revision 4)
> +        response.isArticle = response.isInReaderMode ||
> +          ReaderMode.isProbablyReaderable(content.document);

`isProbablyreaderable` isn't free, and equally doesn't give the right result if the DOM hasn't been loaded yet.

Instead, can we use ReaderParent.jsm, and just add a method on there that takes a tab/browser and returns `browser.isArticle` OR'd with whether the tab is in reader mode (has an about:reader URI) ?
(Reporter)

Updated

8 months ago
status-firefox57: --- → fix-optional
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Comment 19

8 months ago
Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Requesting review on the changes to tab-content.js and browser_readerMode.js, please and thank you.
Flags: needinfo?(bob.silverberg)
Attachment #8906692 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 20

8 months ago
Oops, I added that r? before even seeing your comment, Gijs. I will take a look at it and see what I can do.
(Assignee)

Updated

8 months ago
Attachment #8906692 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #17)
> I apologize for the driveby, but I think we can do this without adding an
> extra message to tab-content etc.

Why?  It was useful.

> Note also that this doesn't seem to implement the same API on android. Do we
> intend to support it there, too?

We'll need a followup bug for android.

> Instead, can we use ReaderParent.jsm, and just add a method on there that
> takes a tab/browser and returns `browser.isArticle` OR'd with whether the
> tab is in reader mode (has an about:reader URI) ?

We'd need to modify the toggle function (or add an alternate arg to it) so it takes a browser rather than event object.  Since we could actually just directly check browser.isArticle and look at about:reader, I'm thinking we can just directly send the message rather than use ReaderParent.  OTOH using ReaderParent may avoid issues with any future changes.  Do you have any strong opinion on that Gijs?
Flags: needinfo?(gijskruitbosch+bugs)
Gijs: 

Bob and I have been chatting about isArticle and the fact that we don't have a clean way to know when isArticle is actually determined since a false is only sent on navigation.  That makes it hard for an addon to know when they can rely on the value being accurate unless it is ==true.  note that tab.status=="complete" can happen prior to 

So my thought is: 

- in tab-content.js always send Reader:UpdateReaderButton, do away with the forceNonArticle stuff, probably also makes the onLocationChange handling unnecessary.
- we listen on that message rather than relying on browser.isArticle.
- browser.tabs.Tab gains an isArticle flag, which defaults to undefined.
- we call the tabs.onUpdated event listener for extensions with the value isArticle (also setting isArticle on the tabs.Tab instance)

This way, an extension has a deterministic way to know when the article value is valid.

I thought about just sending Reader:PushState, but if that happens too early (an article check is pending) it is ignored, so it also doesn't provide us with what we need.
We used to always send up the result of the reader mode message ("even" false) prior to the fix from bug 1147487, but then enabling reader mode caused performance regressions. Reverting the fix (ie always sending isArticle, "even" false) doesn't seem desirable - it'll likely cause the same perf regressions. Note also that we have extant security bugs about Firefox responding poorly to abusive sites that aggressively history.pushState(), and I would like not to add to that problem.

I don't really understand why an add-on would need an update from "unknown" to "false" state. Presumably it would only care if:

a) reader mode becomes available
b) reader mode stops being available

for any given tab. It seems fine to only send updates in that case, and to expose "not readerable" in all other cases. If we send updates, the add-on will still know a tab is readerable at the first possibility. If you wanted to write an add-on to automatically enter reader mode whenever it was available, say, you would simply listen to tab update messages that send "yep, reader mode is available now" and then enter it.

(In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> (In reply to :Gijs from comment #17)
> > I apologize for the driveby, but I think we can do this without adding an
> > extra message to tab-content etc.
> 
> Why?  It was useful.

I don't really understand why. Webextension code (that then communicates to the oop webextension, maybe) in the parent needs to know whether the tab is in reader mode, right, not webextension code in the content process? And the parent process already has this information. So you shouldn't need to talk to the content process.

If you need to send updates to the add-on (which is implied in comment #22) then you could add listeners in the parent process  for UpdateReaderButton.

Does that make sense? :-)
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mixedpuppy)
(In reply to :Gijs from comment #23)

> I don't really understand why an add-on would need an update from "unknown"
> to "false" state. Presumably it would only care if:

It's the indeterminate behavior problem...when do you look to see if it's true?  And if it's false, is it really false or do I wait longer?  

However, I think just adding the update notification is probably enough.

> (In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> > (In reply to :Gijs from comment #17)
> > > I apologize for the driveby, but I think we can do this without adding an
> > > extra message to tab-content etc.
> > 
> > Why?  It was useful.
> 
> I don't really understand why. Webextension code (that then communicates to

I think you misread me.  I was saying "Why Apologize, your input was useful".  I should have shortened the quoted bit more.
Flags: needinfo?(mixedpuppy)
(In reply to Shane Caraveo (:mixedpuppy) from comment #24)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> > > (In reply to :Gijs from comment #17)
> > > > I apologize for the driveby, but I think we can do this without adding an
> > > > extra message to tab-content etc.
> > > 
> > > Why?  It was useful.
> > 
> > I don't really understand why. Webextension code (that then communicates to
> 
> I think you misread me.  I was saying "Why Apologize, your input was
> useful".  I should have shortened the quoted bit more.

Ahhhh. This makes sense.

It probably doesn't matter much, but for whatever it's worth - I apologized because unsolicited feedback (especially of the negative "don't do this" variety) can be a bit demoralizing / "stop energy", and I try not to demoralize people... :-)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 28

8 months ago
Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Gijs, do you mind taking another look at the latest version of the code?
Attachment #8906692 - Flags: review?(mixedpuppy)
Attachment #8906692 - Flags: review?(gijskruitbosch+bugs)
Attachment #8906692 - Flags: review+

Comment 29

8 months ago
mozreview-review
Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review185196

The approach to getting reader mode article / "in reader mode" data looks great, as does the approach to toggling reader mode. I have a few questions / suggestions below, but as this addresses my main concerns with the previous approach, I think Shane or someone else who knows webextensions better than me would be best off reviewing the rest.

::: browser/components/extensions/ext-tabs.js:297
(Diff revision 7)
> +          let readerModeChangeListener = (eventName, event) => {
> +            let {gBrowser} = event.nativeTab.ownerGlobal;
> +            let tab = tabManager.wrapTab(
> +              gBrowser.getTabForBrowser(event.nativeTab));
> +
> +            fireForTab(tab, {readerMode: event.isArticle});

I'm sure this is me missing something, but it looks like this sends the add-on an object with a 'reader mode' property, with a bool value, but the bool indicates whether or not reader mode is available, not whether the tab is in reader mode.

I don't see any specific code to notify add-ons when the tab enters/leaves reader mode, is that right? Based on the test, it seems like we're assuming that the `isArticle` bool will change when entering / leaving reader mode. I'm not sure that is always (going to be) the case.

I wonder if we should change the property name we use for this update event to be clearer, and if we should have a separate update event for entering/leaving reader mode.

::: browser/components/extensions/ext-tabs.js:931
(Diff revision 7)
>          },
> +
> +        async toggleReaderMode(tabId) {
> +          let tab = await promiseTabWhenReady(tabId);
> +          if (!tab.isInReaderMode && !tab.isArticle) {
> +            return;

Should the add-on get some kind of error back?

::: browser/components/extensions/test/browser/browser_ext_tabs_readerMode.js:70
(Diff revision 7)
> +  tabs = await Promise.all([
> +    extension.awaitMessage("tabUpdated"),
> +    extension.awaitMessage("readerMode:false"),
> +  ]);
> +  ok(tabs[0].url.startsWith(READER_MODE_PREFIX), "Tab url indicates reader mode.");
> +  ok(!tabs[1].isArticle, "Tab is not readerable.");

Is it always the case that a tab that's in reader mode has `isArticle` set to false? If so, I expect it's a side-effect of us displaying the contents after the initial page load, or something... not necessarily reliable. So I'm not sure if it makes sense to assert it - but maybe I'm missing something? :-)
Attachment #8906692 - Flags: review?(gijskruitbosch+bugs)

Comment 30

8 months ago
mozreview-review
Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review185214

Just some small changes and a try, I think we're there.

::: browser/components/extensions/ext-browser.js:546
(Diff revision 7)
> +   *        Whether the document in the tab can be rendered in reader mode.
> +   * @private
> +   */
> +  emitReaderModeUpdated(nativeTab, isArticle) {
> +    let tabId = this.getId(nativeTab);
> +    this.emit("tab-reader-mode-updated", {nativeTab, tabId, isArticle});

I don't think this is updating the mode, but is updating the article flag.  How about just tab-isarticle.  Probably same for function name

::: browser/components/extensions/ext-tabs.js:297
(Diff revision 7)
> +          let readerModeChangeListener = (eventName, event) => {
> +            let {gBrowser} = event.nativeTab.ownerGlobal;
> +            let tab = tabManager.wrapTab(
> +              gBrowser.getTabForBrowser(event.nativeTab));
> +
> +            fireForTab(tab, {readerMode: event.isArticle});

lets call that isArticle rather than readerMode since this isn't really a notification on readerMode.

::: browser/components/extensions/ext-tabs.js:931
(Diff revision 7)
>          },
> +
> +        async toggleReaderMode(tabId) {
> +          let tab = await promiseTabWhenReady(tabId);
> +          if (!tab.isInReaderMode && !tab.isArticle) {
> +            return;

yeah, toss an extensionerror
Attachment #8906692 - Flags: review?(mixedpuppy)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 33

8 months ago
mozreview-review-reply
Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review185196

> I'm sure this is me missing something, but it looks like this sends the add-on an object with a 'reader mode' property, with a bool value, but the bool indicates whether or not reader mode is available, not whether the tab is in reader mode.
> 
> I don't see any specific code to notify add-ons when the tab enters/leaves reader mode, is that right? Based on the test, it seems like we're assuming that the `isArticle` bool will change when entering / leaving reader mode. I'm not sure that is always (going to be) the case.
> 
> I wonder if we should change the property name we use for this update event to be clearer, and if we should have a separate update event for entering/leaving reader mode.

I have changed the names because it was misleading. As for having an event for entering/leaving reader mode, that's a good idea, but I think I'll file a follow-up bug for that.

> Is it always the case that a tab that's in reader mode has `isArticle` set to false? If so, I expect it's a side-effect of us displaying the contents after the initial page load, or something... not necessarily reliable. So I'm not sure if it makes sense to assert it - but maybe I'm missing something? :-)

It was something I noticed in my manual testing, but you're right, perhaps it's not intentional and therefore I shouldn't assert it.
Comment hidden (mozreview-request)

Comment 35

8 months ago
mozreview-review
Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review185480
Attachment #8906692 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 45

8 months ago
Thanks for the review, and the help, Shane. The last try was pristine. I need to hold off landing this until after the merge, so I'll just let it sit for a bit.
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
status-firefox57: fix-optional → wontfix
Keywords: dev-doc-needed

Comment 47

8 months ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e174306e0b0
Add some reader mode support to the tabs API, r=mixedpuppy
Backed out for e.g. failing chrome's mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html on Android:

https://hg.mozilla.org/integration/autoland/rev/a97948d297e2bbd8503ec3955f89772834f609e8

Push which ran failing tests: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7eb30eea17798e0e46481f381d91e2b63e50f180&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133131613&repo=autoland

[task 2017-09-25T15:43:35.742Z] 15:43:35     INFO -  85 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html
[task 2017-09-25T15:49:06.982Z] 15:49:06     INFO -  Buffered messages logged at 15:43:37
[task 2017-09-25T15:49:06.983Z] 15:49:06     INFO -  86 INFO SpawnTask.js | Entering test test_activeTab_pageAction
[task 2017-09-25T15:49:06.983Z] 15:49:06     INFO -  87 INFO Extension loaded
[task 2017-09-25T15:49:06.983Z] 15:49:06     INFO -  Buffered messages finished
[task 2017-09-25T15:49:06.983Z] 15:49:06     INFO -  88 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | Test timed out.
[task 2017-09-25T15:49:06.983Z] 15:49:06     INFO -  reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:121:7
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:142:7
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  89 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | Extension left running at test shutdown
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  ExtensionTestUtils.loadExtension/<@chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:109:7
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  executeCleanupFunction@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1224:19
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  SimpleTest.finish@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1237:5
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  killTest@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:130:7
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  delayedKillTest@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:157:47
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  90 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | message queue is empty - got "[\"page-loaded\"]", expected "[]"
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  ExtensionTestUtils.loadExtension/<@chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:27:7
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  executeCleanupFunction@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1224:19
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  91 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | no tasks awaiting on messages - got "[\"background_page.ready\"]", expected "[]"
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  ExtensionTestUtils.loadExtension/<@chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:31:7
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  executeCleanupFunction@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1224:19
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  92 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | Test left extra windows or tabs: {"extraWindows":[],"extraTabs":["http://example.com/#test_activeTab_pageAction"]}
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  @chrome://mochitests/content/chrome/mobile/android/components/extensions/test/mochitest/head.js:27:7
[task 2017-09-25T15:49:17.385Z] 15:49:17     INFO -  93 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | took 332284ms

And more failures in other chrome tests.
Flags: needinfo?(bob.silverberg)
(Assignee)

Updated

8 months ago
Blocks: 1402921
(Assignee)

Updated

8 months ago
Blocks: 1402924
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 51

8 months ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2c18cfef4e3
Add some reader mode support to the tabs API, r=mixedpuppy

Comment 52

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d2c18cfef4e3
Status: REOPENED → RESOLVED
Last Resolved: 10 months ago8 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Updated

8 months ago
Flags: needinfo?(bob.silverberg)
Duplicate of this bug: 1411526
I've written:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/toggleReaderMode

...and updated:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create (for openInReaderMode)
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab (for isArticle and isInReaderMode)

One thing I thought: for the example (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/toggleReaderMode#Examples) I have some code that automatically switches every page into Reader Mode (if it is eligible).

It works, sort of, but breaks the Back button, because the original page is still in the nav history. So suppose:

(1) you load crookedtimber.org -> it's not an article, so nothing happens
(2) you navigate to http://crookedtimber.org/2017/10/30/the-week-after-open-access-week/ -> this is an article page, so you get readerified.

I think the session history is now like:

* (current) about:reader?http://crookedtimber.org/2017/10/30/the-week-after-open-access-week/
* http://crookedtimber.org/2017/10/30/the-week-after-open-access-week/
* http://crookedtimber.org

So if I go back, I go back to the article, then immediately get readerified again. So I wonder, should there be a "loadReplace" option for this API, like for https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/update? Or is there a better way to do this?
Ni for Bob for comment #54 because I know that the "make it readerize everything" option is often requested so I'd like to make it work, but I don't know enough about webextensions to answer the actual question... :-)
Flags: needinfo?(bob.silverberg)
(Assignee)

Comment 56

7 months ago
Thanks for the ping, Gijs. In our current implementation we simply send the "Reader:ToggleReaderMode" message in order to toggle reader mode, so our code doesn't really understand that it may be loading a new URL. If we are to keep the existing implementation (which I prefer, so our API code doesn't have to understand the implementation of reader mode) then it sounds like this is something that would need to be addressed in the reader mode code itself, probably in ReaderMode.enterReaderMode.

Does that make sense to do?
Flags: needinfo?(bob.silverberg) → needinfo?(gijskruitbosch+bugs)
(In reply to Bob Silverberg [:bsilverberg] from comment #56)
> Thanks for the ping, Gijs. In our current implementation we simply send the
> "Reader:ToggleReaderMode" message in order to toggle reader mode, so our
> code doesn't really understand that it may be loading a new URL. If we are
> to keep the existing implementation (which I prefer, so our API code doesn't
> have to understand the implementation of reader mode) then it sounds like
> this is something that would need to be addressed in the reader mode code
> itself, probably in ReaderMode.enterReaderMode.
> 
> Does that make sense to do?

Maybe, but it'd have to be optional and supplied by the add-on, and we'd need to make sure the 'exit reader mode' functionality continues to work in that case (I believe right now it'll use history.back() so we can use bfcache if available, but maybe it's already gated on a URI check, I don't remember off-hand).

We don't want to always do this because in normal usage:

1. open page
2. click reader mode icon
3. click 'back'

the user would expect to go back to the non-reader-ized page, not go back to whatever was open before the page. That's different in the case of the add-on that's being suggested, but of course that's not the only possible usecase of reader mode (even for add-ons).

Will, can you file a followup bug? Thanks for raising this!
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(wbamberg)
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1414026. I wasn't sure if the summary was too prescriptive of the implementation, so feel free to fix it. Thanks!
Flags: needinfo?(wbamberg)
NI for Bob to review the docs. Sorry I just realised I forgot to do that.

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/toggleReaderMode
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create (for openInReaderMode)
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab (for isArticle and isInReaderMode)
Flags: needinfo?(bob.silverberg)
(Assignee)

Comment 60

7 months ago
(In reply to Will Bamberg [:wbamberg] from comment #59)
> NI for Bob to review the docs. Sorry I just realised I forgot to do that.
> 
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/
> toggleReaderMode
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create
> (for openInReaderMode)
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab (for
> isArticle and isInReaderMode)

Looks great, thanks Will.
Flags: needinfo?(bob.silverberg)
Keywords: dev-doc-needed → dev-doc-complete

Comment 61

6 months ago
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe verify-“ flag.

Thanks!
Flags: needinfo?(bob.silverberg)
(Assignee)

Updated

6 months ago
Flags: needinfo?(bob.silverberg) → qe-verify-
You need to log in before you can comment on or make changes to this bug.