Closed Bug 827608 Opened 7 years ago Closed 7 years ago

Use HTML context menus in about pages

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21

People

(Reporter: wesj, Assigned: wesj)

References

Details

Attachments

(4 files)

We should use HTML5 context menus in our about:pages and remove the dependency on digging into browser.js there.
This fixes about:apps.
Attachment #698957 - Flags: review?(mark.finkle)
This was more complex and exposed a problem in the implementation where a page can't hide context menu items in the popupshowing or contextmenu events (as we've already build the menu at that point while trying to make sure we had something to show).
Attachment #698958 - Flags: review?(mark.finkle)
About:addons actually didn't have context menu handlers. This makes it so it does with options for "Enable/Disable" and "Uninstall". I also fixed row highlighting when you tap on them and some inappropriate ("Save image" type) context menu items that appear when you long tap on the icons.
Attachment #698962 - Flags: review?(mark.finkle)
Comment on attachment 698957 [details] [diff] [review]
Patch 1/3 - About:apps

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

>+var ContextMenus = {
>+  target: null,

Can we (Should we) null out target in the context event handlers (addToHomescreen and uninstall) as a safety check? Maybe it's overkill?

>+  handleEvent: function(event) {
>+    // store the target of context menu events so that we know which app to act on
>+    this.target = event.target;
>+    while(!this.target.hasAttribute('contextmenu')) {

nit 1: while (
nit 2: use " not '
Attachment #698957 - Flags: review?(mark.finkle) → review+
Comment on attachment 698958 [details] [diff] [review]
Patch 2/3 - About:downloads

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

>+  handleEvent: function(event) {

>+    while(!this.target.hasAttribute('contextmenu')) {

Same nits here

>+    for (var i = 0; i < this.items.length; i++) {
>+      var item = this.items[i];

var? Are you a webdev? :)


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

>           getValue: function(aElt) {
>+            if (item.hasAttribute("hidden"))
>+              return null;
>             return {

nit: Add a blank line before the second "return"

>+        var val = this.menuitems[i].getValue(aTarget);

var?

>+        if (val)
>+          itemArray.push(val);

nit: Add a comment (and a blank line) before this check letting us know that hidden menus will be null

>diff --git a/mobile/android/locales/en-US/chrome/aboutDownloads.dtd b/mobile/android/locales/en-US/chrome/aboutDownloads.dtd

> <!ENTITY aboutDownloads.title                      "Downloads">
> <!ENTITY aboutDownloads.header                     "Your Downloads">
> <!ENTITY aboutDownloads.empty                      "No Downloads">

>+<!ENTITY downloadAction.open                       "Open">
>+<!ENTITY downloadAction.remove                     "Delete">
>+<!ENTITY downloadAction.removeAll                  "Delete All">
>+<!ENTITY downloadAction.pause                      "Pause">
>+<!ENTITY downloadAction.resume                     "Resume">
>+<!ENTITY downloadAction.cancel                     "Cancel">
>+<!ENTITY downloadAction.retry                      "Retry">

In the last patch you change the entities to match the other strings in the DTD file. Here you keep the same entities as were in the properties file. L10N might also like the entities to change.

r+, but I want Biran to take a look at the heart of the aboutDownload.js changes
Attachment #698958 - Flags: review?(mark.finkle)
Attachment #698958 - Flags: review?(bnicholson)
Attachment #698958 - Flags: review+
Comment on attachment 698962 [details] [diff] [review]
Patch 3/3 - About:addons

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

>+  handleEvent: function(event) {

>+    while(!this.target.hasAttribute('contextmenu')) {

nit: Same

>+    var addon = this.target.addon;

nit: var?
Attachment #698962 - Flags: review?(mark.finkle) → review+
Comment on attachment 698958 [details] [diff] [review]
Patch 2/3 - About:downloads

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

Looks good to me; just noted a few other nits.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +39,5 @@
> +var ContextMenus = {
> +  target: null,
> +
> +  init: function() {
> +    document.addEventListener("contextmenu", ContextMenus, false);

Nit: s/ContextMenus/this/ ?

@@ +56,5 @@
> +    // store the target of context menu events so that we know which app to act on
> +    this.target = event.target;
> +    while(!this.target.hasAttribute('contextmenu')) {
> +      this.target = this.target.parentNode;
> +    }

If the user clicks outside of a download entry, won't this climb up to the root node and eventually throw an error from a null this.target? If so, you should add a null check inside the loop so it can return early.

@@ +73,5 @@
> +  // Open shown only for downloads that completed successfully
> +  open: function(event) {
> +    Downloads.openDownload(this.target);
> +  },
> +  

Nit: Remove the trailing whitespace here and below lines
Attachment #698958 - Flags: review?(bnicholson) → review+
This part of this patch should go to Aurora.
Attachment #699987 - Flags: review+
Comment on attachment 699987 [details] [diff] [review]
Patch 2/3 for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 736321
User impact if declined: pages can't dynamically hide (although they can disable) context menu items
Testing completed (on m-c, etc.): landed on mc today
Risk to taking this patch (and alternatives if risky): pretty low risk. Not a widely used feature on the web, but useful. Required for desktop parity.
String or UUID changes made by this patch: None.
Attachment #699987 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/2a1d1669d813
https://hg.mozilla.org/mozilla-central/rev/fcc86fcb89ae
https://hg.mozilla.org/mozilla-central/rev/a411f59040a1
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Comment on attachment 699987 [details] [diff] [review]
Patch 2/3 for Aurora

We don't think this is a particularly critical issue for our users or web developers. We think this can ride the trains.
Attachment #699987 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Depends on: 830404
Duplicate of this bug: 839090
You need to log in before you can comment on or make changes to this bug.