Last Comment Bug 702019 - Port UI parts of |Bug 617528 - implement the HTML5 "context menu" feature (contextmenu attribute)|
: Port UI parts of |Bug 617528 - implement the HTML5 "context menu" feature (co...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.8
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on: 617528
Blocks: 712871 717111
  Show dependency treegraph
 
Reported: 2011-11-12 08:13 PST by Jens Hatlak (:InvisibleSmiley)
Modified: 2012-01-10 17:13 PST (History)
1 user (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
fixed
fixed
fixed


Attachments
patch (6.35 KB, patch)
2011-11-12 14:46 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
patch v1a (6.38 KB, patch)
2011-11-13 10:18 PST, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Review
patch v2 [Checkin: comments 9 and 10] (5.04 KB, patch)
2011-11-26 06:13 PST, Jens Hatlak (:InvisibleSmiley)
jh: review+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Review

Description Jens Hatlak (:InvisibleSmiley) 2011-11-12 08:13:35 PST
From the base bug:

In the HTML5 spec, section 4.11.4.3, "Context menus", specifies a mechanism for allowing author-developers to define a custom context menu for a particular element. The mechanism enables author-developers to define the context menu declaratively, through markup, by using a "contextmenu" attribute whose value is the ID of a particular menu element in the same document.

http://dev.w3.org/html5/spec/interactive-elements.html#context-menus


This is already in FF 8 (which corresponds with SM 2.5) and doesn't affect l10n so we could in theory try and backport this to Beta to have it as early as SM 2.6 instead of only SM 2.8 (current trunk) or SM 2.7 (Aurora).

Demo:
http://people.mozilla.com/~prouget/bugs/context-menu-test/test.html

m-c changesets:
http://hg.mozilla.org/mozilla-central/rev/561821863607
http://hg.mozilla.org/mozilla-central/rev/cbb901789b3b
Comment 1 Jens Hatlak (:InvisibleSmiley) 2011-11-12 14:46:35 PST
Created attachment 574093 [details] [diff] [review]
patch
Comment 2 neil@parkwaycc.co.uk 2011-11-12 16:20:18 PST
Comment on attachment 574093 [details] [diff] [review]
patch

Won't this break as soon as you try to open the context window in a non-browser window?
Comment 3 Jens Hatlak (:InvisibleSmiley) 2011-11-12 16:26:15 PST
(In reply to neil@parkwaycc.co.uk from comment #2)
> Won't this break as soon as you try to open the context window in a
> non-browser window?

Oops, yes. Two ways out:
a) check for availability of PageMenu and do nothing if it's missing
b) add the PageMenu include to more places (at least MailNews and Composer)
Comment 4 Jens Hatlak (:InvisibleSmiley) 2011-11-13 10:18:19 PST
Created attachment 574163 [details] [diff] [review]
patch v1a

I decided to go with option a. We don't want PageMenu in Composer, so we need to have the PageMenu check anyway. In MailNews it would only make sense for feeds, but the context menu there is already different from the browser one so if anyone really wants to have it there, it can easily be done in a follow-up (with analysis of impact etc.).
Comment 5 neil@parkwaycc.co.uk 2011-11-20 16:42:54 PST
Comment on attachment 574163 [details] [diff] [review]
patch v1a

>+  XPCOMUtils.defineLazyGetter(this, "PageMenu", function() {
>+    let tmp = {};
>+    Components.utils.import("resource://gre/modules/PageMenu.jsm", tmp);
>+    return new tmp.PageMenu();
>+  }); 
Nit: this can now be rewritten as follows:
XPCOMUtils.defineLazyModuleGetter(this, "PageMenu",
                                  "resource://gre/modules/PageMenu.jsm");
Comment 6 Jens Hatlak (:InvisibleSmiley) 2011-11-21 09:55:51 PST
(In reply to neil@parkwaycc.co.uk from comment #5)
> >+  XPCOMUtils.defineLazyGetter(this, "PageMenu", function() {
> >+    let tmp = {};
> >+    Components.utils.import("resource://gre/modules/PageMenu.jsm", tmp);
> >+    return new tmp.PageMenu();
> >+  }); 
> Nit: this can now be rewritten as follows:
> XPCOMUtils.defineLazyModuleGetter(this, "PageMenu",
>                                   "resource://gre/modules/PageMenu.jsm");

Doesn't seem to work. I guess
  tmp["PageMenu"]
is not the same as
  new tmp.PageMenu()
Comment 7 neil@parkwaycc.co.uk 2011-11-21 16:22:02 PST
Comment on attachment 574163 [details] [diff] [review]
patch v1a

>+    return new tmp.PageMenu();
[Bah, there's no reason for this, they were just being awkward...]

>+               pagemenu="start"
Unfortunately you only get to choose start or end. So it makes no sense to put the menuseparator in the middle. r=me with that fixed.

>+    if (!aIsShift && typeof PageMenu !== 'undefined')
[How about using aXulMenu.hasAttribute("pagemenu")? Then you could put the PageMenu lazy getter in nsContextMenu.js too.]
Comment 8 Jens Hatlak (:InvisibleSmiley) 2011-11-26 06:13:45 PST
Created attachment 577052 [details] [diff] [review]
patch v2 [Checkin: comments 9 and 10]

(In reply to neil@parkwaycc.co.uk from comment #7)
> >+               pagemenu="start"
> Unfortunately you only get to choose start or end. So it makes no sense to
> put the menuseparator in the middle. r=me with that fixed.

Sorry, my fault. FF has it as the first one, but I only looked at context that followed.

> >+    if (!aIsShift && typeof PageMenu !== 'undefined')
> [How about using aXulMenu.hasAttribute("pagemenu")? Then you could put the
> PageMenu lazy getter in nsContextMenu.js too.]

Nice, done.
Comment 9 Jens Hatlak (:InvisibleSmiley) 2011-11-26 06:16:43 PST
Comment on attachment 577052 [details] [diff] [review]
patch v2 [Checkin: comments 9 and 10]

http://hg.mozilla.org/comm-central/rev/36004cdd06bb

Requesting Aurora and Beta approval for something that should really have been fixed in SM 2.5 already to match FF 8 that introduced it. No l10n impact, should be safe.
Comment 11 Serge Gautherie (:sgautherie) 2011-12-22 03:15:57 PST
Update of test will happen in bug 712871.
Comment 12 Ian Neal 2012-01-10 17:13:02 PST
Hmmm, this broke debugQATextEditorShell.xul which does not have gBrowser defined

Note You need to log in before you can comment on or make changes to this bug.