Closed Bug 702019 Opened 13 years ago Closed 13 years ago

Port UI parts of |Bug 617528 - implement the HTML5 "context menu" feature (contextmenu attribute)|

Categories

(SeaMonkey :: UI Design, enhancement)

enhancement
Not set
normal

Tracking

(seamonkey2.5 wontfix, seamonkey2.6 fixed, seamonkey2.7 fixed, seamonkey2.8 fixed)

RESOLVED FIXED
seamonkey2.8
Tracking Status
seamonkey2.5 --- wontfix
seamonkey2.6 --- fixed
seamonkey2.7 --- fixed
seamonkey2.8 --- fixed

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #574093 - Flags: review?(neil)
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?
(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)
Attached patch patch v1a (obsolete) — Splinter Review
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.).
Attachment #574093 - Attachment is obsolete: true
Attachment #574093 - Flags: review?(neil)
Attachment #574163 - Flags: review?
Attachment #574163 - Flags: review? → review?(neil)
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");
(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 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.]
Attachment #574163 - Flags: review?(neil) → review+
(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.
Attachment #574163 - Attachment is obsolete: true
Attachment #577052 - Flags: review+
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.
Attachment #577052 - Attachment description: patch v2 → patch v2 [Checkin: comment 9]
Attachment #577052 - Flags: approval-comm-beta?
Attachment #577052 - Flags: approval-comm-aurora?
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.8
Attachment #577052 - Flags: approval-comm-beta?
Attachment #577052 - Flags: approval-comm-beta+
Attachment #577052 - Flags: approval-comm-aurora?
Attachment #577052 - Flags: approval-comm-aurora+
Comment on attachment 577052 [details] [diff] [review]
patch v2 [Checkin: comments 9 and 10]

http://hg.mozilla.org/releases/comm-aurora/rev/602f4250cca6
http://hg.mozilla.org/releases/comm-beta/rev/a03506c2ab9f
Attachment #577052 - Attachment description: patch v2 [Checkin: comment 9] → patch v2 [Checkin: comments 9 and 10]
Depends on: 617528
Update of test will happen in bug 712871.
Blocks: 712871
Flags: in-testsuite-
Blocks: 717111
Hmmm, this broke debugQATextEditorShell.xul which does not have gBrowser defined
You need to log in before you can comment on or make changes to this bug.