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

RESOLVED FIXED in seamonkey2.8

Status

SeaMonkey
UI Design
--
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

Tracking

Trunk
seamonkey2.8
Dependency tree / graph
Bug Flags:
in-testsuite -

SeaMonkey Tracking Flags

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

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
Created attachment 574093 [details] [diff] [review]
patch
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #574093 - Flags: review?(neil)

Comment 2

6 years ago
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?
(Assignee)

Comment 3

6 years ago
(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)
(Assignee)

Comment 4

6 years ago
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.).
Attachment #574093 - Attachment is obsolete: true
Attachment #574093 - Flags: review?(neil)
Attachment #574163 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #574163 - Flags: review? → review?(neil)

Comment 5

6 years ago
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");
(Assignee)

Comment 6

6 years ago
(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

6 years ago
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+
(Assignee)

Comment 8

6 years ago
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.
Attachment #574163 - Attachment is obsolete: true
Attachment #577052 - Flags: review+
(Assignee)

Comment 9

6 years ago
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?
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.8

Updated

6 years ago
Attachment #577052 - Flags: approval-comm-beta?
Attachment #577052 - Flags: approval-comm-beta+
Attachment #577052 - Flags: approval-comm-aurora?
Attachment #577052 - Flags: approval-comm-aurora+
(Assignee)

Comment 10

6 years ago
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]
(Assignee)

Updated

6 years ago
status-seamonkey2.5: --- → wontfix
status-seamonkey2.6: --- → fixed
status-seamonkey2.7: --- → fixed
status-seamonkey2.8: --- → fixed
Depends on: 617528
Update of test will happen in bug 712871.
Blocks: 712871
Flags: in-testsuite-

Updated

6 years ago
Blocks: 717111

Comment 12

6 years ago
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.