Closed
Bug 827608
Opened 11 years ago
Closed 11 years ago
Use HTML context menus in about pages
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 21
People
(Reporter: wesj, Assigned: wesj)
References
Details
Attachments
(4 files)
8.27 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
15.98 KB,
patch
|
mfinkle
:
review+
bnicholson
:
review+
|
Details | Diff | Splinter Review |
5.84 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
2.24 KB,
patch
|
wesj
:
review+
akeybl
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
We should use HTML5 context menus in our about:pages and remove the dependency on digging into browser.js there.
Assignee | ||
Comment 1•11 years ago
|
||
This fixes about:apps.
Attachment #698957 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
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 4•11 years ago
|
||
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 5•11 years ago
|
||
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 6•11 years ago
|
||
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 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1d1669d813 https://hg.mozilla.org/integration/mozilla-inbound/rev/fcc86fcb89ae https://hg.mozilla.org/integration/mozilla-inbound/rev/a411f59040a1
Assignee | ||
Comment 9•11 years ago
|
||
This part of this patch should go to Aurora.
Attachment #699987 -
Flags: review+
Assignee | ||
Comment 10•11 years ago
|
||
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?
Comment 11•11 years ago
|
||
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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Comment 12•11 years ago
|
||
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-
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•