1160578 - Odd empty door hanger panel when you're on an internal Firefox page

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Peter deHaan [:pdehaan]]

Attachment: Nightly_Start_Page.png

### Steps to reproduce:
I was on a random "Nightly Start Page" using the latest integration build and clicked the Pocket button in the toolbar. I noticed that an awkward, empty panel pops up.

### Actual results:
Empty door hanger. See attached screenshot.

### Expected results:
Button isn't enabled for non-HTTP[S] pages or about: pages or something.

Comment 1

[Peter deHaan [:pdehaan]]

This also happens when you're in reader view and try adding the page to Pocket. Attaching a new screenshot.

Comment 2

[Peter deHaan [:pdehaan]]

Attachment: Prosecutors_Charge_6_Baltimore_Officers_in_Freddie_Gray_Death_and_IRCCloud.png

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This disables the button for any page that isn't HTTP, HTTPS, or about:reader.

Note that the panel will still be blank for about:reader pages until the add-on code is updated to retrieve the correct URL for Reader mode pages.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

https://github.com/Pocket/Firefox/pull/22 tracks what needs to be changed within the add-on for reader mode pages.

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Created attachment 8601113 [details] [diff] [review]
> Patch
> 
> This disables the button for any page that isn't HTTP, HTTPS, or
> about:reader.

Can we only disable the "saving button" in these cases?

Meaning, if a person isn't logged in or doesn't have an account and clicks on the button from about:home I think the button should work and show the logged out version of the panel.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Only disables the button if it can be found in the DOM, and also uses .schemeIs to check the scheme instead of doing string comparison.

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 8601774 [details] [diff] [review]
Patch v2

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

::: browser/components/pocket/Pocket.jsm
@@ +139,5 @@
> +    let widget = CustomizableUI.getWidget("pocket-button");
> +    if (widget && widget.node) {
> +      let win = browser.ownerDocument.defaultView;
> +      widget.node.disabled = win.pktApi.isUserLoggedIn() &&
> +                             !locationURI.schemeIs("http") &&

The onLocationChange() code in browser.js seems to imply aLocationURI can be null, so should probably check for that here.

@@ +141,5 @@
> +      let win = browser.ownerDocument.defaultView;
> +      widget.node.disabled = win.pktApi.isUserLoggedIn() &&
> +                             !locationURI.schemeIs("http") &&
> +                             !locationURI.schemeIs("https") &&
> +                             !(locationURI.schemeIs("about") && locationURI.spec.toLower.startsWith("about:reader?url=");

Does that last need grouped with the previous term?

Double negatives make my head hurt, not sure. Whatever works, I guess.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2ebcaa4dd5fc

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2ebcaa4dd5fc

Comment 12

[Andrei Vaida [:avaida]]

Peter, if you don't mind, I'm taking this since it affects Reader View pages as well.

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 8601774 [details] [diff] [review]
Patch v2

a+ for uplift in service of 38.0.5 spring launch campaign.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8601774 [details] [diff] [review]
Patch v2

[Triage Comment]
Fix the branch

Comment 15

[Andrei Vaida [:avaida]]

Confirmed fixed as of Nightly 40.0a1 (2015-05-06), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Tests were performed on most about:* pages. The toolbar button is disabled there and the context menu option is suppressed. This does NOT apply to about:reader URLs.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4e2cc9bbd04

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-release/rev/77ec9aee0263

Comment 18

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using Firefox 38.0.5 Beta 1 (buildID: 20150511143336).

Comment 19

[Vasilica Tamas, QA [:vtamas]]

Verified this issue on Firefox 39 Beta 1 (20150523155636) using Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5 and here is what I have noticed:

- Pocket icon is not disabled in toolbar for about:* pages 
- "Page not saved. Only links can be saved." message is displayed for about:* pages when pocket button from toolbar is selected
- “Saving...” message is displayed for about:* pages when the pocket button from the panel menu is selected for the second time -> issue tracked in Bug 1168351

Based on the above mentions I am marking this bug as verified on Firefox 39 Beta 1.