If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Clicking the reader mode button in an app tab opens reader mode in a new tab

VERIFIED FIXED in Firefox 38

Status

()

Firefox
General
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Margaret, Assigned: mathu, Mentored)

Tracking

Trunk
Firefox 39
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox38 verified, firefox39 verified)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
An edge case, but one we'll need to fix.

Updated

3 years ago
Flags: firefox-backlog+
(Reporter)

Comment 1

3 years ago
Fixing this will involve updating this call to openUILinkIn here:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/ReaderParent.jsm#116

I see an "allowPinnedTabHostChange" property here that looks like exactly what we want:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#189
Mentor: margaret.leibovic@gmail.com
Whiteboard: [lang=js][good first bug]
Mentor: jaws@mozilla.com
Assignee: nobody → hammerly.matt
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8565809 [details] [diff] [review]
bug1124271_readermodenewtab.diff

Hopefully passing json inline to a function like this isn't against any style guidelines.

Patch changes two calls to openUILinkIn() by adding an object with one attribute: { "allowPinnedTabHostChange": true }
The second changed call allows a user to activate reader mode without spawning a new tab, while the first changed call allows them to deactivate it and return to the page.
Attachment #8565809 - Flags: review?(jaws)
Comment on attachment 8565809 [details] [diff] [review]
bug1124271_readermodenewtab.diff

Review of attachment 8565809 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Would you like to add a new test (or update the current one)? There is a test at /browser/base/content/test/general/browser_readerMode.js that does a few things with reader mode.

To run the test, type `mach test browser/base/content/test/general/browser_readerMode.js` from the root directory of your source tree.
Attachment #8565809 - Flags: review?(jaws) → review+
Comment on attachment 8565809 [details] [diff] [review]
bug1124271_readermodenewtab.diff

Review of attachment 8565809 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/ReaderParent.jsm
@@ +127,2 @@
>      } else {
> +      win.openUILinkIn("about:reader?url=" + encodeURIComponent(url), "current", { "allowPinnedTabHostChange": true });

We could also refactor this code to remove some of the duplication:

> let expectedURL = url.startsWith("about:reader") ?
>                     this._getOriginalUrl(url) :
>                     "about:reader?url=" + encodeURIComponent(url);
> win.openUILinkIn(expectedURL, "current", {"allowPinnedTabHostChange": true});
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][do not land yet]
(Assignee)

Comment 5

3 years ago
Created attachment 8566334 [details] [diff] [review]
bug1124271_readermodenewtab.diff

You gave the last one r+, but suggested another approach with less repetition. So here it is. What do I do after r+?
Attachment #8566334 - Flags: review?(jaws)
Attachment #8565809 - Attachment is obsolete: true
Comment on attachment 8566334 [details] [diff] [review]
bug1124271_readermodenewtab.diff

Review of attachment 8566334 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Matt Hammerly [:mathu] from comment #5)
> Created attachment 8566334 [details] [diff] [review]
> bug1124271_readermodenewtab.diff
> 
> You gave the last one r+, but suggested another approach with less
> repetition. So here it is. What do I do after r+?

Normally after r+ the patch will land, but it would be nice to include a test here. I put some tips in comment #3 that you can use as a guide.
Attachment #8566334 - Flags: review?(jaws) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8567425 [details] [diff] [review]
bug1124271_readermodenewtab.diff

Conflicting code was merged; this new patch shouldn't conflict
Attachment #8567425 - Flags: review?(jaws)
(Assignee)

Comment 8

3 years ago
Created attachment 8567431 [details] [diff] [review]
browser_bug1124271_readermodenewtab.diff

Adding a test for this fix
Attachment #8566334 - Attachment is obsolete: true
Attachment #8567431 - Flags: review?(jaws)
Attachment #8567425 - Flags: review?(jaws) → review+
Comment on attachment 8567431 [details] [diff] [review]
browser_bug1124271_readermodenewtab.diff

Review of attachment 8567431 [details] [diff] [review]:
-----------------------------------------------------------------

Reading through the code it looks good, but you'll need to update browser.ini to include a reference to this test.
Attachment #8567431 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 10

3 years ago
Created attachment 8567512 [details] [diff] [review]
browser_bug1124271_readermodenewtab.diff

Added test to browser.ini in this patch

I think I can stop spamming Jared with review requests now
Attachment #8567431 - Attachment is obsolete: true
Attachment #8567512 - Flags: review?(jaws)
Attachment #8567512 - Flags: review?(jaws) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21dedd807757
Whiteboard: [lang=js][good first bug][do not land yet] → [lang=js][good first bug]
Flags: qe-verify+
QA Contact: andrei.vaida
The try run linked above is all green, except a build failure on OSX optimized builds but that looks like an infrastructure issue.

This looks ready to land now. Nice job Matt :)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/b03801dea264
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b03801dea264
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → Firefox 39

Updated

3 years ago
Iteration: --- → 38.3 - 23 Feb
Verified fixed on Nightly 39.0a1 (2015-02-25) using Windows 7 (x64), Ubuntu 14.04 (x64), Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
status-firefox39: fixed → verified
Created attachment 8583401 [details] [diff] [review]
Patch for 38

Approval Request Comment
[Feature/regressing bug #]: reading list/reader mode
[User impact if declined]: 38 will have partial reading list/reader mode support
[Describe test coverage new/current, TreeHerder]: automated tests and manual tests
[Risks and why]: none expected
[String/UUID change made/needed]: none
Attachment #8583401 - Flags: approval-mozilla-aurora?
Comment on attachment 8583401 [details] [diff] [review]
Patch for 38

It looks like reading list/reader mode has broad approval and I can just use a=readinglist

https://hg.mozilla.org/releases/mozilla-aurora/rev/fa8645257f11 for reference
Attachment #8583401 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/releases/mozilla-aurora/rev/581298c4704e
status-firefox38: --- → fixed
Verified fixed on Aurora 38.0a2 (2015-03-29) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.
status-firefox38: fixed → verified
You need to log in before you can comment on or make changes to this bug.