Bookmarklets don't work in Fennec Native

VERIFIED FIXED in Firefox 13

Status

()

defect
P2
normal
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: djst, Assigned: bnicholson)

Tracking

Trunk
Firefox 13
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 verified, fennec13+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Bookmarklets don't work in Fennec anymore. 

Steps to reproduce:

1. Save a bookmarklet from e.g. "invert lightness" from this page https://www.squarefree.com/bookmarklets/color.html. I couldn't find a way to do this from Fennec because it won't show the context menu when click-holding on a button, so the easiest way is to do this on desktop Firefox. Simply drag a bookmarklet to the bookmarks toolbar.
2. Use Firefox sync to get this new bookmark on Fennec.
3. Load a plain web site in Fennec.
4. Select the newly bookmarklet from the awesome bar's Bookmarks section.

Expected results: the colors of the website should be inverted.
Actual results: nothing happens.

This works fine in the classic/XUL version of Fennec.
Richard, are we supporting bookmarklets in sync for native?
Component: General → Android Sync
Product: Fennec Native → Mozilla Services
QA Contact: general → android-sync
Version: Trunk → unspecified
If the bookmark gets to Fennec -- which apparently it does -- and you can see the snippet of JavaScript in the bookmark UI under the title, where the URL should be, then Sync has done all it can.

Sync doesn't give one jot about what's in the title and URI fields.
Component: Android Sync → General
Product: Mozilla Services → Fennec Native
QA Contact: android-sync → general
Version: unspecified → Trunk
This is likely a regression caused by bug 703378.
tracking-fennec: --- → ?
tracking-fennec: ? → 13+
Priority: -- → P2
Assignee: nobody → bnicholson
Posted patch patch (obsolete) — Splinter Review
Tested by syncing a bookmarklet from desktop. Since there's no way to add a bookmarklet with the existing UI, I'm not sure how to make a test case for this.
Attachment #597637 - Flags: review?(mark.finkle)
(In reply to Brian Nicholson (:bnicholson) from comment #4)

> Tested by syncing a bookmarklet from desktop. Since there's no way to add a
> bookmarklet with the existing UI, I'm not sure how to make a test case for
> this.

Can you use the LocalDB API in robocop tests? If so, you could probably just call addBookmark with javascript: url, right? testBookmark selects a bookmark (although that's broken right now with the UI changes going on), but maybe we could do something similar for this. We'd have to somehow check to make sure the js executed though, maybe we can be clever with our bookmarklet code. (I assume you were talking about how to add an automated test ;)
Comment on attachment 597637 [details] [diff] [review]
patch


>+    static final String INPUT_KEY = "input";

Use INPUT_KEY -> USERENTED_KEY
Use "input" -> "userEntered"

>-    private void openUrlAndFinish(String url) {
>+    private void openUrlAndFinish(String url, boolean input) {

Lets add openUserEnteredAndFinish

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

use "userEntered" in the JSON

>       let params = {
>         selected: true,
>         parentId: ("parentId" in data) ? data.parentId : -1,
>-        flags: Ci.nsIWebNavigation.LOAD_FLAGS_DISALLOW_INHERIT_OWNER
>+        flags: (data.input && Ci.nsIWebNavigation.LOAD_FLAGS_DISALLOW_INHERIT_OWNER)
>              | Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP

For simplicity, lets use a local variable for "flags" and use a simple if-check:

let flags =  Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
if (data.userEntered)
  Ci.nsIWebNavigation.LOAD_FLAGS_DISALLOW_INHERIT_OWNER;

let params = {
...
  flags: flags
}

r+, but throw up a new patch so we can see the changes
Attachment #597637 - Flags: review?(mark.finkle) → review+
Posted patch patch v2Splinter Review
updated with changes
Attachment #597637 - Attachment is obsolete: true
Attachment #599346 - Flags: review+
test utility that waits for a given condition to be true before continuing
Attachment #599348 - Flags: review?(mark.finkle)
Posted patch test caseSplinter Review
Attachment #599350 - Flags: review?(mark.finkle)
Comment on attachment 599348 [details] [diff] [review]
timed test utility

Moving this to Joel since it is a utility. It looks OK to me, but I wonder if Robocop already has this kind of feature.
Attachment #599348 - Flags: review?(mark.finkle) → review?(jmaher)
Comment on attachment 599350 [details] [diff] [review]
test case

You should probably make sure the bookmark is removed from the database before moving on. Use another direct DB cursor to clear the DB.

Also want Geoff to take a look too, in case I missed something.
Attachment #599350 - Flags: review?(mark.finkle)
Attachment #599350 - Flags: review?(gbrown)
Attachment #599350 - Flags: review+
Comment on attachment 599350 [details] [diff] [review]
test case

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

Looks great.
Attachment #599350 - Flags: review?(gbrown) → review+
Comment on attachment 599348 [details] [diff] [review]
timed test utility

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

There are two base classes: BaseTest and PixelTest.  This might be better served in FennecNativeActions, but how it is used seems just fine in BaseTest for now.

::: mobile/android/base/tests/BaseTest.java.in
@@ +5,5 @@
>  import @ANDROID_PACKAGE_NAME@.*;
>  
>  import android.app.Activity;
>  import android.app.Instrumentation;
> +import android.content.Intent;

do we need to import Intent?
Attachment #599348 - Flags: review?(jmaher) → review+
Blocks: 725483
(In reply to Joel Maher (:jmaher) from comment #13)
> Comment on attachment 599348 [details] [diff] [review]
> ::: mobile/android/base/tests/BaseTest.java.in
> @@ +5,5 @@
> >  import @ANDROID_PACKAGE_NAME@.*;
> >  
> >  import android.app.Activity;
> >  import android.app.Instrumentation;
> > +import android.content.Intent;
> 
> do we need to import Intent?

Yes - it's used in setUp(). Note that I just moved that line, not added it (I changed the imports to be in alphabetical order).
Verified fixed on:

Firefox 13.0a1 (2012-03-06)
20120306031101
http://hg.mozilla.org/mozilla-central/rev/7d0d1108a14e

--
Device: HTC Desire
OS: Android 2.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.