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)
SeaMonkey
UI Design
Tracking
(seamonkey2.5 wontfix, seamonkey2.6 fixed, seamonkey2.7 fixed, seamonkey2.8 fixed)
RESOLVED
FIXED
seamonkey2.8
People
(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)
References
Details
Attachments
(1 file, 2 obsolete files)
5.04 KB,
patch
|
InvisibleSmiley
:
review+
iannbugzilla
:
approval-comm-aurora+
iannbugzilla
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Comment 2•13 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•13 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•13 years ago
|
||
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•13 years ago
|
Attachment #574163 -
Flags: review? → review?(neil)
Comment 5•13 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•13 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•13 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•13 years ago
|
||
(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•13 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•13 years ago
|
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+
Assignee | ||
Comment 10•13 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•13 years ago
|
status-seamonkey2.5:
--- → wontfix
status-seamonkey2.6:
--- → fixed
status-seamonkey2.7:
--- → fixed
status-seamonkey2.8:
--- → fixed
Comment 11•12 years ago
|
||
Update of test will happen in bug 712871.
Blocks: 712871
Flags: in-testsuite-
Comment 12•12 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.
Description
•