Closed Bug 1160578 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Pocket, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 40
Tracking Status
firefox38.0.5 --- verified
firefox39 --- verified
firefox40 --- verified

People

(Reporter: pdehaan, Assigned: jaws)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached image 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.
This also happens when you're in reader view and try adding the page to Pocket. Attaching a new screenshot.
Duplicate of this bug: 1161182
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8601113 - Flags: review?(dolske)
https://github.com/Pocket/Firefox/pull/22 tracks what needs to be changed within the add-on for reader mode pages.
(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.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8601113 - Attachment is obsolete: true
Attachment #8601113 - Flags: review?(dolske)
Attachment #8601685 - Flags: review?(dolske)
Attached patch Patch v2Splinter Review
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.
Attachment #8601685 - Attachment is obsolete: true
Attachment #8601685 - Flags: review?(dolske)
Attachment #8601774 - Flags: review?(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.
Attachment #8601774 - Flags: review?(dolske) → review+
https://hg.mozilla.org/mozilla-central/rev/2ebcaa4dd5fc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Peter, if you don't mind, I'm taking this since it affects Reader View pages as well.
Flags: qe-verify+
QA Contact: andrei.vaida
Comment on attachment 8601774 [details] [diff] [review]
Patch v2

a+ for uplift in service of 38.0.5 spring launch campaign.
Attachment #8601774 - Flags: approval-mozilla-beta+
Attachment #8601774 - Flags: approval-mozilla-aurora+
Comment on attachment 8601774 [details] [diff] [review]
Patch v2

[Triage Comment]
Fix the branch
Attachment #8601774 - Flags: approval-mozilla-beta+ → approval-mozilla-release+
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.
Status: RESOLVED → VERIFIED
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).
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.
You need to log in before you can comment on or make changes to this bug.