Last Comment Bug 617528 - implement the HTML5 "context menu" feature (contextmenu attribute)
: implement the HTML5 "context menu" feature (contextmenu attribute)
Status: RESOLVED FIXED
: dev-doc-needed, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- enhancement with 7 votes (vote)
: mozilla8
Assigned To: Jan Varga [:janv]
:
Mentors:
http://whatwg.org/html/#context-menus
Depends on: 295285 676236 699709 703998 704428 705292 692139 701328 702125 870388
Blocks: html5 702560 746087 897102 702019 721614
  Show dependency treegraph
 
Reported: 2010-12-07 22:12 PST by Michael[tm] Smith
Modified: 2014-02-24 05:37 PST (History)
59 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
patch for event dispatcher (2.03 KB, patch)
2011-06-02 00:49 PDT, Jan Varga [:janv]
bugs: review-
Details | Diff | Splinter Review
patch for event dispatcher v2 (2.52 KB, patch)
2011-06-02 12:34 PDT, Jan Varga [:janv]
bugs: review+
Details | Diff | Splinter Review
test for event dispatcher (4.27 KB, patch)
2011-06-05 22:00 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
patch v1 (112.82 KB, patch)
2011-06-08 14:12 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
browser/ part (4.25 KB, patch)
2011-06-10 09:33 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
patch v2 (112.96 KB, patch)
2011-06-12 13:58 PDT, Jan Varga [:janv]
bugs: review-
Details | Diff | Splinter Review
patch for event dispatcher v3 (ready to be pushed) (8.74 KB, patch)
2011-06-14 11:47 PDT, Jan Varga [:janv]
mounir: review-
Details | Diff | Splinter Review
Part 1 - Dispatch shift right click (contextmenu event) only to chrome (ready to be pushed) (9.06 KB, patch)
2011-06-15 08:00 PDT, Jan Varga [:janv]
mounir: checkin+
Details | Diff | Splinter Review
Part 2 - Add support for HTML5 Commands (93.28 KB, patch)
2011-06-21 09:59 PDT, Jan Varga [:janv]
mounir: review-
Details | Diff | Splinter Review
Part2 - Core implementation (99.62 KB, patch)
2011-07-06 13:00 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
Part 2 - Core implementation v2 (117.32 KB, patch)
2011-07-08 02:43 PDT, Jan Varga [:janv]
bugs: review-
Details | Diff | Splinter Review
Part 2 - Core implementation v3 (118.01 KB, patch)
2011-07-15 12:51 PDT, Jan Varga [:janv]
dao+bmo: review-
Details | Diff | Splinter Review
Part 2 - Core implementation v4 (128.19 KB, patch)
2011-07-20 06:47 PDT, Jan Varga [:janv]
dao+bmo: review+
Details | Diff | Splinter Review
Part 2 - Core implementation v5 (145.94 KB, patch)
2011-07-25 04:11 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
Part 2 - Core implementation v6 (145.69 KB, patch)
2011-07-25 05:59 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
interdiff v4 - v5 (31.23 KB, patch)
2011-07-25 07:16 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
interdiff v5 - v6 (6.43 KB, patch)
2011-07-25 07:17 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
comments (5.24 KB, text/plain)
2011-07-27 06:42 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details
Part 2 - Core implementation v7 (144.75 KB, patch)
2011-07-27 08:44 PDT, Jan Varga [:janv]
bugs: review+
Details | Diff | Splinter Review
Part 2 - Core implementation v8 (145.73 KB, patch)
2011-07-27 12:56 PDT, Jan Varga [:janv]
bugs: review+
Details | Diff | Splinter Review
Part 2 - Core implementation v9 (147.35 KB, patch)
2011-08-08 06:08 PDT, Jan Varga [:janv]
bugs: review+
enndeakin: review+
Details | Diff | Splinter Review
Changes between v8 and v9 (85.92 KB, patch)
2011-08-08 06:09 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
A followup fix (14.30 KB, patch)
2011-08-15 06:06 PDT, Jan Varga [:janv]
enndeakin: review+
Details | Diff | Splinter Review
updated demo (1.13 KB, text/html)
2011-08-18 22:50 PDT, Jan Varga [:janv]
no flags Details

Description Michael[tm] Smith 2010-12-07 22:12:51 PST
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b8pre) Gecko/20101124 Firefox/4.0b8pre
Build Identifier: 4.0b8pre

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

Reproducible: Always
Comment 1 Paul Rouget [:paul] 2011-02-20 10:48:38 PST
Experimenting with <menu type="context"> :

http://people.mozilla.com/~prouget/bugs/context-menu-test/nativemenu.xpi

Try it with this web page: http://people.mozilla.com/~prouget/bugs/context-menu-test/test.html

My main concern is security. How can we make it clear that these menu-items are not from Firefox?
Comment 2 steve faulkner 2011-02-21 01:22:55 PST
keep in mind context menus must be operable using the keyboard, for example in windows the shift+f10 keys will open the context menu for the currently focused element (if focusable) or the element adjacent/containing the caret when caret browsing is enabled.
Comment 3 Robert O'Rourke 2011-02-21 05:09:24 PST
I like this a lot but I think it would be best to keep the regular context menu looking like part of the OS and then allow for styling one area within the context menu that contains site specific elements - basically sandbox the site specific features somehow. It would go some way towards alleviating the potential security problems you mention and standard browser usability would be unaffected, it would also be a better indication to the user that it's new stuff/ site specific stuff.
Comment 4 Mounir Lamouri (:mounir) 2011-02-22 15:21:36 PST
Paul, is that implemented by any UA?
Comment 5 Antoine Turmel [:GeekShadow] 2011-02-22 16:17:34 PST
(In reply to comment #1)
> Experimenting with <menu type="context"> :
> 
> http://people.mozilla.com/~prouget/bugs/context-menu-test/nativemenu.xpi
> 
> Try it with this web page:
> http://people.mozilla.com/~prouget/bugs/context-menu-test/test.html
> 
> My main concern is security. How can we make it clear that these menu-items are
> not from Firefox?

A separator plus a warning seems good. Replace it by "Web Menu" maybe ?
Comment 6 Paul Rouget [:paul] 2011-02-23 02:27:08 PST
(In reply to comment #4)
> Paul, is that implemented by any UA?

No.
Comment 7 Paul Rouget [:paul] 2011-02-23 02:28:25 PST
This feature could be only available for App Tabs (I think we can consider these pages as trusted by the user).
Comment 8 Mounir Lamouri (:mounir) 2011-02-23 02:43:06 PST
(In reply to comment #7)
> This feature could be only available for App Tabs (I think we can consider
> these pages as trusted by the user).

A sub-menu in the context menu seems like a cautious way to make sure the user understand the items are from the website. Something like:
[ Copy ]
[ Paste ]
[ Anything ]
[ Menu provided by the website > ]
   [ Foo ]
   [ Bar ]

Anyhow, I'm pretty sure simulating a context menu in javascript isn't really hard nowadays and a website might already be able to fool a user that way. If that's true, that would limit the security regression for users with javascript disabled, right?
Comment 9 Paul Rouget [:paul] 2011-02-23 02:48:51 PST
Web pages want to enhance native UI to make controls easier to reach. Hiding the menu in a submenu makes this feature less useful.

Also, I agree about the security regression.

I think allowing this feature only for AppTabs, with no warning, is a good tradeoff.
Comment 10 Mounir Lamouri (:mounir) 2011-02-23 02:54:38 PST
Actually, with <menu>, the website can run a javascript method when the user click a menu item but how is that more harmful than running a javascript method at any other moment? The user might click on the menu item thinking that's Firefox menu but how would that change anything? I mean, I can't think of any scenario where it makes a difference.
Comment 11 Paul Rouget [:paul] 2011-02-23 03:01:41 PST
(In reply to comment #10)
> Actually, with <menu>, the website can run a javascript method when the user
> click a menu item but how is that more harmful than running a javascript method
> at any other moment? The user might click on the menu item thinking that's
> Firefox menu but how would that change anything? I mean, I can't think of any
> scenario where it makes a difference.

I understand that. But I don't think a web-side action should be executed when the user think he is executing a UA-side action. I think this should be clear, somehow.
Comment 12 Christopher Blizzard (:blizzard) 2011-02-24 09:46:12 PST
The value of this is that a right click menu will look native, instead of being generated by the web site.  That's about it.

Sites are already controlling the right click experience because they can.  Right click on a photo on flickr, or right click on a flash or html5 video on youtube.  I did my taxes the other day and it turns out that the turbotax site overrides the right click as well.

So I'd rather just make it possible for web sites to control the context menu and get an event.  If there are items that always have to be there (can sites generate copy/paste events?) they should be near the bottom.

Does the API allow you to control what goes into the menu when they click or does it require that the items be declared ahead of time?
Comment 13 Paul Rouget [:paul] 2011-02-24 10:22:07 PST
(In reply to comment #12)
> Does the API allow you to control what goes into the menu when they click or
> does it require that the items be declared ahead of time?

It's a declarative mechanism, like:

<menu type=context>
  <command label="rotate" onclick="rotate()">
  <command label="resize" onclick="resize()">
  <menu label="share">
    <command label="twitter" onclick="alert('foo')">
    <command label="facebook" onclick="alert('bar')">
  </menu>
</menu>

But the HTML can be changed anytime (on the right click event for example).

Would prefer a Javascript API?
Comment 14 Mounir Lamouri (:mounir) 2011-05-26 06:28:25 PDT
Jan, could you explain how bug 556743 blocks this bug?
Comment 15 Jan Varga [:janv] 2011-05-26 07:00:09 PDT
Sure

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

4.11.4.2 Building menus and toolbars

A menu (or toolbar) consists of a list of zero or more of the following components:
Commands, which can be marked as default commands
Separators
Other menus (which allows the list to be nested)

So I need to use the "command API" to build a context menu.

http://dev.w3.org/html5/spec/commands.html#concept-command

4.11.5.3 Using the input element to define a command
...
Otherwise, the Type is "radio" or "checkbox". If the element is a labeled control, the textContent of the first label element in tree order whose labeled control is the element in question is the Label (in DOM terms, this is the string given by element.labels[0].textContent). Otherwise, the value of the value attribute, if present, is the Label. Otherwise, the Label is the empty string.

So I need |element.labels[0].textContent|
Using your WIP patch, it looks quite simple in C++

if (mLabelList.Length()) {
  nsIContent* labelContent = mLabelList.GetNodeAt(0);
  nsContentUtils::GetNodeTextContent(labelContent, PR_TRUE, aLabel);
}
else {
  GetAttr(kNameSpaceID_None, nsGkAtoms::value, aLabel);
}

I'm actually trying to add full support for "command API" along with HTML5 context menus.
http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#elements

I don't know, maybe there's a separate bug for "command API" too
Comment 16 Marco Zehe (:MarcoZ) 2011-05-26 07:26:42 PDT
We also need to take into account exposure to accessibility APIs. e. g. the XUL generated context menu mixed in with the HTML5 stuff.
Comment 17 Mounir Lamouri (:mounir) 2011-05-26 09:16:49 PDT
(In reply to comment #15)
> So I need |element.labels[0].textContent|
> Using your WIP patch, it looks quite simple in C++
> 
> if (mLabelList.Length()) {
>   nsIContent* labelContent = mLabelList.GetNodeAt(0);
>   nsContentUtils::GetNodeTextContent(labelContent, PR_TRUE, aLabel);
> }
> else {
>   GetAttr(kNameSpaceID_None, nsGkAtoms::value, aLabel);
> }

My WIP patch is old and wrong. I don't like how this is spec'ed and it's far from being an author priority.
Though, doing a helper to get the labels shouldn't be hard. Getting the first label in tree order labeling an element neither.

> I'm actually trying to add full support for "command API" along with HTML5
> context menus.
> http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.
> html#elements
> 
> I don't know, maybe there's a separate bug for "command API" too

I think that would be better to do this in a separate patch/bug.
Comment 18 Jan Varga [:janv] 2011-05-27 07:03:28 PDT
Ok, I'll implement a helper to get the labels.
Comment 19 Jan Varga [:janv] 2011-06-01 11:22:52 PDT
There might be an accessibility problem when a web page defines too many commands (menu items). Should we set a limit for number of items in the root menu ?
Comment 20 Jan Varga [:janv] 2011-06-02 00:49:09 PDT
Created attachment 536833 [details] [diff] [review]
patch for event dispatcher

Per http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#context-menus

"User agents may provide means for bypassing the context menu processing model, ensuring that the user can always access the UA's default context menus. For example, the user agent could handle right-clicks that have the Shift key depressed in such a way that it does not fire the contextmenu event and instead always shows the default context menu."

The attached patch adds a flag NS_EVENT_FLAG_ONLY_CHROME_DISPATCH when the shift key is depressed (only for context menu event).

However it seems there's a bug in the event dispatcher when handling the flag.
event.target can contain an anonymous element.
For example, when I right click on an input element in a web page, event.target returns the internal <div> of <input> element.

I'm attaching it as a separate patch and I will request a review for this from smaug
Comment 21 Jan Varga [:janv] 2011-06-02 00:58:41 PDT
Btw, a side effect of this patch is the possibility to access default context menu, even when a web page calls event.preventDefault()

Without switching a pref.
Comment 22 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-06-02 10:30:14 PDT
Comment on attachment 536833 [details] [diff] [review]
patch for event dispatcher

This is hackish. I don't want magical re-targeting in case
of native anonymous, but which wouldn't work with other anonymous content.

We could add yet another flag for this case.
NS_EVENT_RETARGET_TO_NON_NATIVE_ANONYMOUS
And then in PresShell you would set both
NS_EVENT_FLAG_ONLY_CHROME_DISPATCH and NS_EVENT_RETARGET_TO_NON_NATIVE_ANONYMOUS

And this needs automatic tests.
Comment 23 Jan Varga [:janv] 2011-06-02 12:34:57 PDT
Created attachment 536957 [details] [diff] [review]
patch for event dispatcher v2
Comment 24 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-06-02 12:47:40 PDT
Comment on attachment 536957 [details] [diff] [review]
patch for event dispatcher v2

>diff -r 9a6c139a4e58 content/events/src/nsEventDispatcher.cpp
>--- a/content/events/src/nsEventDispatcher.cpp	Thu Jun 02 10:27:51 2011 +0800
>+++ b/content/events/src/nsEventDispatcher.cpp	Thu Jun 02 21:25:31 2011 +0200
>@@ -489,6 +489,21 @@
> 
>   nsCOMPtr<nsPIDOMEventTarget> target = do_QueryInterface(aTarget);
> 
>+  PRBool retargeted = PR_FALSE;
>+
>+  if (aEvent->flags & NS_EVENT_RETARGET_TO_NON_NATIVE_ANONYMOUS) {
>+    nsCOMPtr<nsIContent> content = do_QueryInterface(target);
>+    if (content && content->IsInNativeAnonymousSubtree()) {
>+      nsCOMPtr<nsPIDOMEventTarget> newTarget =
>+        do_QueryInterface(content->FindFirstNonNativeAnonymous());
NS_ENSURE_STATE(newTarget);
And then no need for if (newTarget)

And the test?

r=me for the patch, but a test is needed before landing this.
Comment 25 Jan Varga [:janv] 2011-06-02 12:54:52 PDT
(In reply to comment #24)
> >+      nsCOMPtr<nsPIDOMEventTarget> newTarget =
> >+        do_QueryInterface(content->FindFirstNonNativeAnonymous());
> NS_ENSURE_STATE(newTarget);
> And then no need for if (newTarget)
> 
> And the test?

working on it

> 
> r=me for the patch, but a test is needed before landing this.

thanks
Comment 26 Jan Varga [:janv] 2011-06-05 02:33:20 PDT
It seems it's not possible to automatically test the NS_EVENT_RETARGET_TO_NON_NATIVE_ANONYMOUS flag. I couldn't find a way to dispatch events to anonymous content from a script.
Comment 27 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-06-05 09:50:15 PDT
If you use DOMMouseUtils::sendMouseEvent and use the coordinates of some
textarea element for example. Send the events to the center of the textarea
(if it has text), then the event should be dispatched to native anonymous
<div>
Comment 28 Jan Varga [:janv] 2011-06-05 10:28:51 PDT
(In reply to comment #27)
> If you use DOMMouseUtils::sendMouseEvent and use the coordinates of some
> textarea element for example. Send the events to the center of the textarea
> (if it has text), then the event should be dispatched to native anonymous
> <div>

yeah, I'm experimenting with it right now ...
anyway, for some reason when the event is dispatched, event.target is HTMLHTMLElement
Comment 29 Jan Varga [:janv] 2011-06-05 10:42:43 PDT
Also, a synthetic context menu event with the shift key set is not dispatched via pres shell, so those flags (NS_EVENT_RETARGET_TO_NON_NATIVE_ANONYMOUS and NS_EVENT_FLAG_ONLY_CHROME_DISPATCH) are not set in this case :(
Comment 30 Jan Varga [:janv] 2011-06-05 10:52:03 PDT
utils.dispatchDOMEventViaPresShell() works ok, but then I can't test the retargeting ...
Comment 31 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-06-05 12:02:05 PDT
(In reply to comment #29)
> Also, a synthetic context menu event with the shift key set is not
> dispatched via pres shell,
Sure it is. sendMouseEvent uses either viewobserver (which is the preshell) or
widget which will then forward the call the viewobserver.
Comment 32 Jan Varga [:janv] 2011-06-05 12:05:17 PDT
Never mind, I figured it out ...
DOMMouseUtils::sendMouseEvent works as expected

It's the element.dispatchEvent() that probably doesn't go through the pres shell.
Comment 33 Jan Varga [:janv] 2011-06-05 22:00:54 PDT
Created attachment 537498 [details] [diff] [review]
test for event dispatcher
Comment 34 Jan Varga [:janv] 2011-06-08 14:12:35 PDT
Created attachment 538114 [details] [diff] [review]
patch v1
Comment 35 Jan Varga [:janv] 2011-06-08 15:03:47 PDT
Here are some notes about the implementation:
- the implementation follows sections 4.11.4.2 and 4.11.4.3 of the spec, including all the details like postprocessing and the shift right click
- the page menu (native context menu built from HTML menu) works on top of all XUL elements in theory, <browser> and <iframe> are obvious ones
- it even works in mixed documents (XUL documents with HTML elements)
- menu items of the page menu can be placed almost anywhere in the native context menu, pagemenupopup="true" and pagemenupos="start|end" are new XUL attributes for specifying the position (so it's possible to place it in the top or bottom of the menu or even in a submenu)
- if there's no pagemenupopup attribute then the feature is disabled
- implementation includes support for a special attribute "nodefault" on HTML menu element, if the attribute is set and document is in an app tab then default context menu items are not shown
- default context menu is always accessible with the shift key depressed
Comment 36 :Ms2ger (⌚ UTC+1/+2) 2011-06-08 15:08:21 PDT
Comment on attachment 538114 [details] [diff] [review]
patch v1

>--- a/content/html/content/src/nsGenericHTMLElement.cpp
>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>+nsGenericHTMLElement::GetDescendantURIAttr(nsIAtom* aTag, nsIAtom* aAttr) const
>+{
>+  PRUint32 count = GetChildCount();
>+  for (PRUint32 i = 0; i < count; i++) {
>+    nsIContent* child = GetChildAt(i);

I think we're trying to move away from indexed access.

>+    if (!child->NodeInfo()->Equals(aTag, kNameSpaceID_XHTML))

!child->IsHTML(aTag)

>+      continue;
>+
>+    nsGenericHTMLElement* element = FromContent(child);
>+    if (element) {

How can this fail?

>+nsGenericHTMLElement::GetContextMenu(nsIDOMHTMLMenuElement** aContextMenu)
>+  nsIDocument* doc = GetCurrentDoc();

GetOwnerDoc?

>+  if (doc) {
>+    Element* element = doc->GetElementById(value);
>+    if (element && element->NodeInfo()->Equals(nsGkAtoms::menu,
>+                                               kNameSpaceID_XHTML))

element->IsHTML(nsGkAtoms::menu)

>+nsGenericHTMLElement::SetContextMenu(nsIDOMHTMLMenuElement* aContextMenu)
>+{
>+  NS_ENSURE_ARG_POINTER(aContextMenu);
>+
>+  nsresult rv;
>+  nsCOMPtr<nsIDOMHTMLElement> element = do_QueryInterface(aContextMenu, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsAutoString id;
>+  element->GetId(id);

You want to QI to nsIContent and GetAttr, I think. nsIDOM* stuff tends to be slow.

>+
>+  if (id.IsEmpty())
>+    return NS_ERROR_FAILURE;

The spec doesn't throw here. (Also, I like {}s.)

>+NS_IMETHODIMP
>+nsGenericHTMLElement::GetLabel(nsAString& aLabel)
>+{
>+  if (DoGetCmdType() == CMD_TYPE_NONE && HasFlag(NODE_HAS_ACCESSKEY)) {
>+    // XXXvarga check .labels here once it's implemented
>+    nsContentUtils::GetNodeTextContent(this, PR_TRUE, aLabel);
>+  }
>+  else
>+    SetDOMStringToNull(aLabel);

if () {

} else {

}

>--- a/content/html/content/src/nsGenericHTMLElement.h
>+++ b/content/html/content/src/nsGenericHTMLElement.h
>+  enum CmdType
>+  {
>+    CMD_TYPE_NONE               = 0,
>+    CMD_TYPE_RADIO              = 1,
>+    CMD_TYPE_CHECKBOX           = 2,
>+    CMD_TYPE_COMMAND            = 3,
>+  };
>+
>+  PRUint32 GetCmdType();
>+  virtual PRUint32 DoGetCmdType();

Return CmdType?

>--- a/content/html/content/src/nsHTMLAnchorElement.cpp
>+++ b/content/html/content/src/nsHTMLAnchorElement.cpp
>+nsHTMLAnchorElement::GetIcon(nsAString& aIcon)
>+    }
>+    else {

} else {

>--- /dev/null
>+++ b/content/html/content/src/nsHTMLCommandElement.cpp
>+class nsHTMLCommandElement : public nsGenericHTMLElement,
>+                             public nsIDOMHTMLCommandElement

I'd appreciate it if this was in a .h

>+NS_IMETHODIMP
>+nsHTMLCommandElement::Click()
>+{
>+  if (mType == CMD_TYPE_CHECKBOX) {
>+    if (HasAttr(kNameSpaceID_None, nsGkAtoms::checked))
>+      UnsetAttr(kNameSpaceID_None, nsGkAtoms::checked, PR_TRUE);
>+    else
>+      SetAttr(kNameSpaceID_None, nsGkAtoms::checked,
>+              NS_LITERAL_STRING("checked"), PR_TRUE);

I think you want to set it to EmptyString() here.

>+    PRUint32 count = parent->GetChildCount();
>+    for (PRUint32 i = 0; i < count; i++) {
>+      nsCOMPtr<nsIContent> child = parent->GetChildAt(i);

As above.

>+      if (child->NodeInfo()->Equals(nsGkAtoms::command, kNameSpaceID_XHTML)) {

As above.

>+    SetAttr(kNameSpaceID_None, nsGkAtoms::checked,
>+            NS_LITERAL_STRING("checked"), PR_TRUE);

.

>+nsHTMLCommandElement::ParseAttribute(PRInt32 aNamespaceID,
>+      PRBool success

This tends to be called ok.

>--- a/content/html/content/src/nsHTMLInputElement.cpp
>+++ b/content/html/content/src/nsHTMLInputElement.cpp
>+nsHTMLInputElement::DoGetCmdType()

Does this want a switch?

>+    // XXXvarga check .labels here once it's implemented

There's a bug for that, right? Please mention the bug number. (Also above.)

>--- a/content/html/content/src/nsHTMLLabelElement.cpp
>+++ b/content/html/content/src/nsHTMLLabelElement.cpp
>+nsHTMLLabelElement::GetCommandElement()
>+{
>+  Element* element = GetLabeledElement();
>+
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(element);

No need for this, Element inherits from nsIContent.

>--- a/content/html/content/src/nsHTMLLabelElement.h
>+++ b/content/html/content/src/nsHTMLLabelElement.h
>+#include "nsIDOMNSHTMLElement.h"

Can't be forward declared?

>--- a/content/html/content/src/nsHTMLLegendElement.cpp
>+++ b/content/html/content/src/nsHTMLLegendElement.cpp
>+Element*
>+nsHTMLLegendElement::GetCommandElement(nsIContent* aParent,
>+                                       nsIContent* aIgnoreContent)
>+{
>+  PRUint32 count = aParent->GetChildCount();
>+  for (PRUint32 i = 0; i < count; i++) {
>+    nsIContent* child = aParent->GetChildAt(i);
>+
>+    if (!child->IsElement())
>+      continue;
>+
>+    if (aIgnoreContent && child == aIgnoreContent)
>+      continue;
>+
>+    if (!child->NodeInfo()->Equals(nsGkAtoms::label, kNameSpaceID_XHTML) &&
>+        !child->NodeInfo()->Equals(nsGkAtoms::legend, kNameSpaceID_XHTML)) {
>+
>+      nsGenericHTMLElement* htmlElement =
>+        nsGenericHTMLElement::FromContent(child);
>+
>+      if (htmlElement && htmlElement->GetCmdType() != CMD_TYPE_NONE)
>+        return child->AsElement();

return htmlElement? I seem to have seen most of this code before, though. Can it be shared somehow?

>+nsHTMLLegendElement::DoGetCmdType(nsIDOMNSHTMLElement** aElement)

Is there a particular reason not to return dom::Element here, btw?

>--- a/content/html/content/src/nsHTMLOptionElement.cpp
>+++ b/content/html/content/src/nsHTMLOptionElement.cpp
>+nsHTMLOptionElement::DoGetCmdType()
>+{
>+  nsCOMPtr<nsIDOMHTMLSelectElement> selectElement = do_QueryInterface(
>+    static_cast<nsIContent*>(GetSelect()));

Er, what? You can just assign GetSelect()'s result here.

>--- a/content/xul/content/src/nsXULPopupListener.cpp
>+++ b/content/xul/content/src/nsXULPopupListener.cpp
>+nsXULPopupListener::HandleContent(nsIContent* aTriggerContent,
>+  CallQueryInterface(pageMenuContent, aPageMenu);
>+
>+  return NS_OK;

return CallQueryInterface(pageMenuContent, aPageMenu)?

>+nsXULPopupListener::GenerateContent(nsIContent* aPageMenuContent,
>+    mPageMenuTable = new PageMenuTable;
>+    if (!mPageMenuTable || !mPageMenuTable->Init()) {

Remove the !mPageMenuTable check.

This functions seems to want less nsIDOM*, I think.

>diff --git a/content/xul/content/src/nsXULPopupListener.h b/content/xul/content/src/nsXULPopupListener.h
>--- a/content/xul/content/src/nsXULPopupListener.h
>+++ b/content/xul/content/src/nsXULPopupListener.h
>+#include "nsIDOMHTMLMenuElement.h"

Looks like you can do without?

>--- a/dom/interfaces/html/nsIDOMHTMLDocument.idl
>+++ b/dom/interfaces/html/nsIDOMHTMLDocument.idl
>-

I added that line on purpose, you know :)
Comment 37 Jan Varga [:janv] 2011-06-08 16:47:08 PDT
(In reply to comment #36)
> Comment on attachment 538114 [details] [diff] [review] [review]
> patch v1
> 
> >--- a/content/html/content/src/nsGenericHTMLElement.cpp
> >+++ b/content/html/content/src/nsGenericHTMLElement.cpp
> >+nsGenericHTMLElement::GetDescendantURIAttr(nsIAtom* aTag, nsIAtom* aAttr) const
> >+{
> >+  PRUint32 count = GetChildCount();
> >+  for (PRUint32 i = 0; i < count; i++) {
> >+    nsIContent* child = GetChildAt(i);
> 
> I think we're trying to move away from indexed access.
> 

ok, makes sense

> >+    if (!child->NodeInfo()->Equals(aTag, kNameSpaceID_XHTML))
> 
> !child->IsHTML(aTag)

ok

> 
> >+      continue;
> >+
> >+    nsGenericHTMLElement* element = FromContent(child);
> >+    if (element) {
> 
> How can this fail?
> 

I see, no need for that

> >+nsGenericHTMLElement::GetContextMenu(nsIDOMHTMLMenuElement** aContextMenu)
> >+  nsIDocument* doc = GetCurrentDoc();
> 
> GetOwnerDoc?

why ?

> 
> >+  if (doc) {
> >+    Element* element = doc->GetElementById(value);
> >+    if (element && element->NodeInfo()->Equals(nsGkAtoms::menu,
> >+                                               kNameSpaceID_XHTML))
> 
> element->IsHTML(nsGkAtoms::menu)

ok

> 
> >+nsGenericHTMLElement::SetContextMenu(nsIDOMHTMLMenuElement* aContextMenu)
> >+{
> >+  NS_ENSURE_ARG_POINTER(aContextMenu);
> >+
> >+  nsresult rv;
> >+  nsCOMPtr<nsIDOMHTMLElement> element = do_QueryInterface(aContextMenu, &rv);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  nsAutoString id;
> >+  element->GetId(id);
> 
> You want to QI to nsIContent and GetAttr, I think. nsIDOM* stuff tends to be
> slow.

nsGenericHTMLElement::GetId(nsAString& aId) is actually pretty efficient here
it returns just GetAttr(kNameSpaceID_None, nsGkAtoms::id, aId);

so it doesn't matter IMHO


> 
> >+
> >+  if (id.IsEmpty())
> >+    return NS_ERROR_FAILURE;
> 
> The spec doesn't throw here. (Also, I like {}s.)
> 

ok

> >+NS_IMETHODIMP
> >+nsGenericHTMLElement::GetLabel(nsAString& aLabel)
> >+{
> >+  if (DoGetCmdType() == CMD_TYPE_NONE && HasFlag(NODE_HAS_ACCESSKEY)) {
> >+    // XXXvarga check .labels here once it's implemented
> >+    nsContentUtils::GetNodeTextContent(this, PR_TRUE, aLabel);
> >+  }
> >+  else
> >+    SetDOMStringToNull(aLabel);
> 
> if () {
> 
> } else {
> 
> }
> 

ok

> >--- a/content/html/content/src/nsGenericHTMLElement.h
> >+++ b/content/html/content/src/nsGenericHTMLElement.h
> >+  enum CmdType
> >+  {
> >+    CMD_TYPE_NONE               = 0,
> >+    CMD_TYPE_RADIO              = 1,
> >+    CMD_TYPE_CHECKBOX           = 2,
> >+    CMD_TYPE_COMMAND            = 3,
> >+  };
> >+
> >+  PRUint32 GetCmdType();
> >+  virtual PRUint32 DoGetCmdType();
> 
> Return CmdType?

no, the code looks uglier with it, it requires extra casting and all DoGeCmdType() methods in derived classes would have to be prefixed with nsGenericHTMLElement::CmdType

nsHTMLInputElement does the same, InputElementTypes is an enum and mType is PRUint8, GetType() returns PRUint32

> 
> >--- a/content/html/content/src/nsHTMLAnchorElement.cpp
> >+++ b/content/html/content/src/nsHTMLAnchorElement.cpp
> >+nsHTMLAnchorElement::GetIcon(nsAString& aIcon)
> >+    }
> >+    else {
> 
> } else {

ok

> 
> >--- /dev/null
> >+++ b/content/html/content/src/nsHTMLCommandElement.cpp
> >+class nsHTMLCommandElement : public nsGenericHTMLElement,
> >+                             public nsIDOMHTMLCommandElement
> 
> I'd appreciate it if this was in a .h

I can do that

> 
> >+NS_IMETHODIMP
> >+nsHTMLCommandElement::Click()
> >+{
> >+  if (mType == CMD_TYPE_CHECKBOX) {
> >+    if (HasAttr(kNameSpaceID_None, nsGkAtoms::checked))
> >+      UnsetAttr(kNameSpaceID_None, nsGkAtoms::checked, PR_TRUE);
> >+    else
> >+      SetAttr(kNameSpaceID_None, nsGkAtoms::checked,
> >+              NS_LITERAL_STRING("checked"), PR_TRUE);
> 
> I think you want to set it to EmptyString() here.

the spec states:
"the UA must add a checked attribute, with the literal value checked"

> 
> >+    PRUint32 count = parent->GetChildCount();
> >+    for (PRUint32 i = 0; i < count; i++) {
> >+      nsCOMPtr<nsIContent> child = parent->GetChildAt(i);
> 
> As above.

ok

> 
> >+      if (child->NodeInfo()->Equals(nsGkAtoms::command, kNameSpaceID_XHTML)) {
> 
> As above.

ok

> 
> >+    SetAttr(kNameSpaceID_None, nsGkAtoms::checked,
> >+            NS_LITERAL_STRING("checked"), PR_TRUE);
> 
> .
> 
> >+nsHTMLCommandElement::ParseAttribute(PRInt32 aNamespaceID,
> >+      PRBool success
> 
> This tends to be called ok.

nsHTMLInputElement and nsHTMLButtonElement both call it success :)

> 
> >--- a/content/html/content/src/nsHTMLInputElement.cpp
> >+++ b/content/html/content/src/nsHTMLInputElement.cpp
> >+nsHTMLInputElement::DoGetCmdType()
> 
> Does this want a switch?

if you prefer :)

> 
> >+    // XXXvarga check .labels here once it's implemented
> 
> There's a bug for that, right? Please mention the bug number. (Also above.)

ok

> 
> >--- a/content/html/content/src/nsHTMLLabelElement.cpp
> >+++ b/content/html/content/src/nsHTMLLabelElement.cpp
> >+nsHTMLLabelElement::GetCommandElement()
> >+{
> >+  Element* element = GetLabeledElement();
> >+
> >+  nsCOMPtr<nsIContent> content = do_QueryInterface(element);
> 
> No need for this, Element inherits from nsIContent.

ok

> 
> >--- a/content/html/content/src/nsHTMLLabelElement.h
> >+++ b/content/html/content/src/nsHTMLLabelElement.h
> >+#include "nsIDOMNSHTMLElement.h"
> 
> Can't be forward declared?

it can, ok

> 
> >--- a/content/html/content/src/nsHTMLLegendElement.cpp
> >+++ b/content/html/content/src/nsHTMLLegendElement.cpp
> >+Element*
> >+nsHTMLLegendElement::GetCommandElement(nsIContent* aParent,
> >+                                       nsIContent* aIgnoreContent)
> >+{
> >+  PRUint32 count = aParent->GetChildCount();
> >+  for (PRUint32 i = 0; i < count; i++) {
> >+    nsIContent* child = aParent->GetChildAt(i);
> >+
> >+    if (!child->IsElement())
> >+      continue;
> >+
> >+    if (aIgnoreContent && child == aIgnoreContent)
> >+      continue;
> >+
> >+    if (!child->NodeInfo()->Equals(nsGkAtoms::label, kNameSpaceID_XHTML) &&
> >+        !child->NodeInfo()->Equals(nsGkAtoms::legend, kNameSpaceID_XHTML)) {
> >+
> >+      nsGenericHTMLElement* htmlElement =
> >+        nsGenericHTMLElement::FromContent(child);
> >+
> >+      if (htmlElement && htmlElement->GetCmdType() != CMD_TYPE_NONE)
> >+        return child->AsElement();
> 
> return htmlElement? I seem to have seen most of this code before, though.
> Can it be shared somehow?

I agree with returning just htmlElement
but, what would you like to share here ?

> 
> >+nsHTMLLegendElement::DoGetCmdType(nsIDOMNSHTMLElement** aElement)
> 
> Is there a particular reason not to return dom::Element here, btw?

yes, almost all callers expect nsIDOMNSHTMLElement interface


> 
> >--- a/content/html/content/src/nsHTMLOptionElement.cpp
> >+++ b/content/html/content/src/nsHTMLOptionElement.cpp
> >+nsHTMLOptionElement::DoGetCmdType()
> >+{
> >+  nsCOMPtr<nsIDOMHTMLSelectElement> selectElement = do_QueryInterface(
> >+    static_cast<nsIContent*>(GetSelect()));
> 
> Er, what? You can just assign GetSelect()'s result here.
> 

ok

> >--- a/content/xul/content/src/nsXULPopupListener.cpp
> >+++ b/content/xul/content/src/nsXULPopupListener.cpp
> >+nsXULPopupListener::HandleContent(nsIContent* aTriggerContent,
> >+  CallQueryInterface(pageMenuContent, aPageMenu);
> >+
> >+  return NS_OK;
> 
> return CallQueryInterface(pageMenuContent, aPageMenu)?

ok

> 
> >+nsXULPopupListener::GenerateContent(nsIContent* aPageMenuContent,
> >+    mPageMenuTable = new PageMenuTable;
> >+    if (!mPageMenuTable || !mPageMenuTable->Init()) {
> 
> Remove the !mPageMenuTable check.

ok

> 
> This functions seems to want less nsIDOM*, I think.
> 

No, that's on purpose ...
AppendChildTo() which allows to bypass notifications can't be used here.
It doesn't handle document fragments anyway and actually we do want to send notifications at that (final) point. 

> >diff --git a/content/xul/content/src/nsXULPopupListener.h b/content/xul/content/src/nsXULPopupListener.h
> >--- a/content/xul/content/src/nsXULPopupListener.h
> >+++ b/content/xul/content/src/nsXULPopupListener.h
> >+#include "nsIDOMHTMLMenuElement.h"
> 
> Looks like you can do without?

sure, I'll move it to .cpp

> 
> >--- a/dom/interfaces/html/nsIDOMHTMLDocument.idl
> >+++ b/dom/interfaces/html/nsIDOMHTMLDocument.idl
> >-
> 
> I added that line on purpose, you know :)

sure, I decided to remove ".commands" in the end, because it didn't handle live updates of <label> and <legend> elements with accesskeys

and I overlooked that line somehow
Comment 38 Mounir Lamouri (:mounir) 2011-06-10 07:59:26 PDT
Jan, can the already reviewed patch be pushed alone?
In addition, is it possible to split the patch? is it ready to be reviewed?
Comment 39 Jan Varga [:janv] 2011-06-10 08:37:52 PDT
(In reply to comment #38)
> Jan, can the already reviewed patch be pushed alone?

yes, there's only one small change, see comment 24
that change is included in the big patch
you can also merge it with the attached automatic test


> In addition, is it possible to split the patch? is it ready to be reviewed?
I'm already incorporating suggestions from ms2ger. Some of them were unclear ... bz helped me to solve it on irc

I will attach a new patch soon.

browser/base could be reviewed separately, I'll attach a patch for it in a sec
Comment 40 Jan Varga [:janv] 2011-06-10 09:33:51 PDT
Created attachment 538539 [details] [diff] [review]
browser/ part
Comment 41 Jan Varga [:janv] 2011-06-10 09:50:09 PDT
(In reply to comment #40)
> Created attachment 538539 [details] [diff] [review] [review]
> browser/ part

the current placement of page menu is in the bottom of native context menu
as I mentioned it can be easily changed (to place it in the top or in a submenu)

this patch is mostly about showing/hiding page menu separator and about hiding default context menu items for <menu type="context" nodefault> in app tabs

the nodefault attribute is not in the standard, but I think it can be really useful for web apps
see http://userexperience.evantageconsulting.com/2010/08/detailed-html5-ux-designers/
(Figure 4)
Comment 42 :Ms2ger (⌚ UTC+1/+2) 2011-06-10 09:54:47 PDT
Should be -x-moz-nodefault (or _moz-nodefault), and please send email to whatwg or file a bug.
Comment 43 Ole Dormann 2011-06-10 13:46:41 PDT
If you use a custom context menu with default items, custom items would typically be more important than the standard items, so usability wise, they should be at the top.
Comment 44 Jan Varga [:janv] 2011-06-12 13:58:20 PDT
Created attachment 538778 [details] [diff] [review]
patch v2
Comment 45 Jan Varga [:janv] 2011-06-13 00:11:51 PDT
Neil, could you take a look at XUL related changes ?
especially changes to these classes/interfaces: nsXULElement, nsXULPopupListener and nsIDOMXULElement
Comment 46 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-06-13 04:33:38 PDT
(In reply to comment #43)
> If you use a custom context menu with default items, custom items would
> typically be more important than the standard items, so usability wise, they
> should be at the top.
More important to who? To web developer sure, but I' not at all sure they
would be more important to the user.
But this is a good question and this all needs a ux review.
Comment 47 Jan Varga [:janv] 2011-06-13 04:50:43 PDT
who should do a ux review ?
can you cc him/her ?
Comment 48 Ole Dormann 2011-06-13 06:18:20 PDT
(In reply to comment #46)
> (In reply to comment #43)
> > If you use a custom context menu with default items, custom items would
> > typically be more important than the standard items, so usability wise, they
> > should be at the top.
> More important to who? To web developer sure, but I' not at all sure they
> would be more important to the user.
> But this is a good question and this all needs a ux review.

More important to the enduser. If you add a custom context menu with say a item "save changes" it would properly be a more important item than say "view source" or "save page as..."

Just look a the context menu for pinned task in IE9 when you trigger it from the Windows taskbar - custom item at the top.

But of course a minor detail :-)
Comment 49 Neil Deakin 2011-06-13 06:30:55 PDT
(In reply to comment #45)
> Neil, could you take a look at XUL related changes ?
> especially changes to these classes/interfaces: nsXULElement,
> nsXULPopupListener and nsIDOMXULElement

I'm not clear why this needs to be special cased. A popupshowing event that adds the elements with the listener implemented in content/html should be sufficient no? Or, it seems like it could just be implemented in script in nsContextMenu.js
Comment 50 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-06-13 07:02:24 PDT
Comment on attachment 538778 [details] [diff] [review]
patch v2


> 
>+already_AddRefed<nsIURI>
>+nsGenericHTMLElement::GetDescendantURIAttr(nsIAtom* aTag, nsIAtom* aAttr) const
This returns only uri from a child element, not from (deeper) the descendants..
Icon handling clearly needs tests.


>+nsGenericHTMLElement::GetContextMenu(nsIDOMHTMLMenuElement** aContextMenu)
>+{
>+  NS_ENSURE_ARG_POINTER(aContextMenu);
>+
>+  *aContextMenu = nsnull;
>+
>+  nsAutoString value;
>+  GetAttr(kNameSpaceID_None, nsGkAtoms::contextmenu, value);
>+
>+  if (value.IsEmpty())
>+    return NS_OK;
>+
>+  nsIDocument* doc = GetCurrentDoc();
>+  if (doc) {
>+    Element* element = doc->GetElementById(value);
>+    if (element && element->IsHTML(nsGkAtoms::menu))
>+      return CallQueryInterface(element, aContextMenu);
>+  }
Why not just
if (doc) {
  nsCOMPtr<nsIDOMHTMLMenuElement> menu = do_QueryInterface(doc->GetElementById(value));
  menu.swap(*aContextMenu);
}


>+nsGenericHTMLElement::SetContextMenu(nsIDOMHTMLMenuElement* aContextMenu)
>+{
>+  NS_ENSURE_ARG_POINTER(aContextMenu);
>+
>+  nsAutoString id;
>+  aContextMenu->GetId(id);
>+
>+  return SetAttr(kNameSpaceID_None, nsGkAtoms::contextmenu, id, PR_TRUE);
>+}
Huh, HTML spec is strange again. But I'm not going to complain.


The spec is actually too strange in this case to implement without
arguing getting it to be fixed.

var m = document.createElement("menu");
m.id = "m";
someotherElement.contextMenu = m;
alert(someotherElement.contextMenu == m); // This would be false
alert(someotherElement.getAttribute("contextmenu") == m.getAttribute("id")); // This would be true.



>+PRUint32
>+nsGenericHTMLElement::GetCmdType()
>+{
>+  PRUint32 cmdType = DoGetCmdType();
>+  if (cmdType != CMD_TYPE_NONE)
>+    return cmdType;

if (expr) {
  ...
}
Same also elsewhere.



>+NS_IMETHODIMP
>+nsGenericHTMLElement::GetIcon(nsAString& aIcon)
>+{
>+  SetDOMStringToNull(aIcon);
>+  return NS_OK;
>+}
Need to review this...


> nsresult nsGenericHTMLElement::Click()
> {
>   if (HasFlag(NODE_HANDLING_CLICK))
>     return NS_OK;
> 
>+  if (DoGetCmdType() == CMD_TYPE_NONE && HasFlag(NODE_HAS_ACCESSKEY))
>+    Focus();
>+
This should be after SetFlags(NODE_HANDLING_CLICK);
But, what in the spec says that focus should be called here?
I couldn't see it in 
http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#dom-click
(Perhaps it is mentioned somewhere where "pre-click activation steps" is defined, but couldn't find it now) 


>+nsHTMLButtonElement::GetIcon(nsAString& aIcon)
>+{
>+  nsCOMPtr<nsIURI> uri = GetDescendantURIAttr(nsGkAtoms::img,
>+                                              nsGkAtoms::src);
>+  if (uri) {
>+    nsCAutoString spec;
>+    uri->GetSpec(spec);
>+    CopyUTF8toUTF16(spec, aIcon);
>+  }
>+  else {
>+    SetDOMStringToNull(aIcon);
>+  }
>+  return NS_OK;


if (expr) {
  stmt;
} else {
  stmt;
}
Same also elsewhere

>+nsHTMLCommandElement::Click()
>+{
>+  if (mType == CMD_TYPE_CHECKBOX) {
>+    if (HasAttr(kNameSpaceID_None, nsGkAtoms::checked))
>+      UnsetAttr(kNameSpaceID_None, nsGkAtoms::checked, PR_TRUE);
>+    else
>+      SetAttr(kNameSpaceID_None, nsGkAtoms::checked,
>+              NS_LITERAL_STRING("checked"), PR_TRUE);
>+  }
>+  else if (mType == CMD_TYPE_RADIO) {
>+    nsAutoString radiogroup;
>+    GetRadiogroup(radiogroup);
>+
>+    nsCOMPtr<nsIContent> parent = GetParent();
>+
>+    nsCOMPtr<nsIContent> cur;
>+    for (cur = parent->GetFirstChild(); cur; cur = cur->GetNextSibling()) {
>+      if (cur != this && cur->IsHTML(nsGkAtoms::command)) {
>+        nsAutoString rg;
>+        cur->GetAttr(kNameSpaceID_None, nsGkAtoms::radiogroup, rg);
>+        if (rg.Equals(radiogroup) &&
>+            cur->HasAttr(kNameSpaceID_None, nsGkAtoms::checked))
>+          cur->UnsetAttr(kNameSpaceID_None, nsGkAtoms::checked, PR_TRUE);
>+      }
>+    }
>+
>+
>+    SetAttr(kNameSpaceID_None, nsGkAtoms::checked,
>+            NS_LITERAL_STRING("checked"), PR_TRUE);
>+  }
>+
>+  return nsGenericHTMLElement::Click();
>+}
This looks wrong. Shouldn't this be in PreHandleEvent()/PostHandleEvent()


This needs lots of tests.
Comment 51 :Ms2ger (⌚ UTC+1/+2) 2011-06-13 09:00:37 PDT
(In reply to comment #50)
> Why not just
> if (doc) {
>   nsCOMPtr<nsIDOMHTMLMenuElement> menu =
> do_QueryInterface(doc->GetElementById(value));
>   menu.swap(*aContextMenu);
> }

menu.forget(aContextMenu);, then :)
Comment 52 Jan Varga [:janv] 2011-06-13 11:04:40 PDT
(In reply to comment #49)
> (In reply to comment #45)
> > Neil, could you take a look at XUL related changes ?
> > especially changes to these classes/interfaces: nsXULElement,
> > nsXULPopupListener and nsIDOMXULElement
> 
> I'm not clear why this needs to be special cased. A popupshowing event that
> adds the elements with the listener implemented in content/html should be
> sufficient no? Or, it seems like it could just be implemented in script in
> nsContextMenu.js

Neil, the page menu should be built as the default action of the context menu event.
Anyway, how would I know (in content/html) what xul menupopup to use for building ? and it seems more logical to built xul elements in content/xul by iterating over html elements than building xul elements in content/html.
Regarding nsContextMenu.js ...
it could be built there, but nsContextMenu.js lives in browser/
I think this is a feature that should be natively implemented, so it can be reused in any xul based app. C++ implementation should be faster. I don't know if it is possible to use interface/object hash tables in JS. The current C++ implementation maps xul menu items directly to html elements.
Also, we can avoid comparing most of the strings in C++ like command type etc when building the menu.
Comment 53 Jan Varga [:janv] 2011-06-13 11:19:03 PDT
(In reply to comment #52)
> I think this is a feature that should be natively implemented, so it can be
> reused in any xul based app. C++ implementation should be faster. I don't
> know if it is possible to use interface/object hash tables in JS. The
> current C++ implementation maps xul menu items directly to html elements.
> Also, we can avoid comparing most of the strings in C++ like command type
> etc when building the menu.

There's also a possibility that the logic for building context menus might be reused for toolbar menus in future.
I guess they will have to be implemented in C++
Comment 54 Jan Varga [:janv] 2011-06-13 11:29:51 PDT
(In reply to comment #50)
> >+already_AddRefed<nsIURI>
> >+nsGenericHTMLElement::GetDescendantURIAttr(nsIAtom* aTag, nsIAtom* aAttr) const
> This returns only uri from a child element, not from (deeper) the
> descendants..
> Icon handling clearly needs tests.

It should be ok since the loop is calling GetNextNode() not GetNextSibling()

I'll fix the nits mentioned in other comments and will wait for consensus on stuff that is not so clear (the spec is unclear and even buggy in some cases)

I'll also try to add more automatic tests.
Comment 55 Neil Deakin 2011-06-13 11:37:18 PDT
(In reply to comment #52)
> (In reply to comment #49)

> Neil, the page menu should be built as the default action of the context
> menu event.

The patch here is generating the menu just before ShowPopup is called. The popupshowing event will be fired and listeners called before ShowPopup returns. So it seems like it shouldn't cause any difference in when the menu is generated, as both will happen during the processing of the contextmenu event.

I'll look into the patch in more detail...
Comment 56 Jan Varga [:janv] 2011-06-14 11:47:38 PDT
Created attachment 539268 [details] [diff] [review]
patch for event dispatcher v3 (ready to be pushed)
Comment 57 Neil Deakin 2011-06-14 13:00:41 PDT
You'll want to add the event to nsDOMClassInfo::DefineStaticJSVals and nsEventReceiverSH::ReallyIsEventName as well.


diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
+    this.xulMenu = aXulMenu;
     this.browser = aBrowser;
+
+    if (this.initStandalonePageMenu())
+      return;

Just pass xulMenu to initStandalonePageMenu and don't store it on 'this'.


diff --git a/content/xul/content/src/nsXULElement.cpp b/content/xul/content/src/nsXULElement.cpp
   nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
-  if (!pm)
+  if (!pm || pm->IsPopupOpen(popup))
     return NS_OK;

+  nsCOMPtr<nsIDOMMouseEvent> mouseEvent = do_QueryInterface(aEvent);
+  PRBool shiftKey;
+  mouseEvent->GetShiftKey(&shiftKey);
+  if (!shiftKey) {
+    nsCOMPtr<nsIDOMEventTarget> target;
+    aEvent->GetTarget(getter_AddRefs(target));
+    nsCOMPtr<nsIContent> triggerContent = do_QueryInterface(target);
+    if (triggerContent)
+      HandleContent(triggerContent, popup, getter_AddRefs(mPageMenu));
+  }
+

Should this only happen for context menus?

+nsresult
+nsXULPopupListener::HandleContent(nsIContent* aTriggerContent,
+                                  nsIContent* aPopupContent,
+                                  nsIDOMNode** aPageMenu)
...
+  nsEvent event(PR_TRUE, NS_SHOW_EVENT);
+  event.flags |= NS_EVENT_FLAG_CANT_CANCEL;
+  event.flags |= NS_EVENT_FLAG_CANT_BUBBLE;

Just put this on one line.


+  nsCOMPtr<nsIPresShell> shell = document->GetShell();
+  if (!shell)
+    return NS_ERROR_FAILURE;
+ 
+  nsRefPtr<nsPresContext> presContext = shell->GetPresContext();
+  nsEventStatus status = nsEventStatus_eIgnore;
+  nsEventDispatcher::Dispatch(pageMenuContent, presContext, &event, nsnull,
+                              &status);
+
+  nsresult rv = GenerateContent(pageMenuContent, aPopupContent);
+  if (NS_FAILED(rv)) {
+    if (mPageMenuTable)
+      mPageMenuTable->Clear();
+    return rv;
+  }

Is the content supposed to be generated only when the show event wasn't cancelled?


+nsresult
+nsXULPopupListener::FindPageMenu(nsIContent* aTriggerContent,
...
+  if (!aTriggerContent->IsHTML())
+    return NS_OK;
+
+  nsCOMPtr<nsIDOMHTMLMenuElement> menuElement;

I'd move this right inside the loop where it's used.


+  nsAutoString pagemenupos;
+  insertionPoint->GetAttr(kNameSpaceID_None, nsGkAtoms::pagemenupos,
+                          pagemenupos);

Rather than have two seperate attributes 'pagemenupopup' and 'pagemenupos',
I'd just use one for both uses: pagemenu = "start"


+  if (pagemenupos.IsEmpty() || pagemenupos.EqualsLiteral("start")) {
+    nsCOMPtr<nsIContent> refContent = insertionPoint->GetFirstChild();
+    insertionPoint->InsertBefore(fragmentContent, refContent, &rv);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+  else if (pagemenupos.EqualsLiteral("end")) {
+    insertionPoint->AppendChild(fragmentContent, &rv);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  return NS_OK;

Remove the two NS_ENSURE_SUCCESS lines and just return rv here.


+    else if (child->IsHTML(nsGkAtoms::option)) {
+      nsAutoString text;
+      nsContentUtils::GetNodeTextContent(child, PR_TRUE, text);
+
+      if (child->HasAttr(kNameSpaceID_None, nsGkAtoms::value) &&
+          child->AttrValueIs(kNameSpaceID_None, nsGkAtoms::value,
+                             EmptyString(), eIgnoreCase) &&
+          child->HasAttr(kNameSpaceID_None, nsGkAtoms::disabled) &&
+          CountCharInReadable(text, PRUnichar('-')) == text.Length()) {
+        rv = CreateSeparator(aDocument, child, aPopupContent);
+        NS_ENSURE_SUCCESS(rv, rv);
+      }
+    }

What a bizarre spec requirement.


+nsresult
+nsXULPopupListener::CreateElement(nsIDocument* aDocument,
+                                  nsIAtom* aTag, nsIContent** aResult)
...
+  (*aResult)->SetAttr(kNameSpaceID_None, nsGkAtoms::pagemenuitem,
+                      EmptyString(), PR_FALSE);

Is there a reason to use non-notifying modifications for all the SetAttr calls?


+nsresult
+nsXULPopupListener::CreateSubmenu(nsIDocument* aDocument,
+                                  nsIContent* aHTMLContent,
+                                  nsIContent* aPopupContent)
...
+  rv = CreateMenu(aDocument, aHTMLContent, menuPopup);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  return NS_OK;

Again, as in a few other places, just return rv here.


+nsresult
+nsXULPopupListener::PostprocessGeneratedContent(nsIContent* aPopupContent)
+{

I think it would be better to maintain a flag that indicates if the last node generated was a separator, and
not add duplicates, rather than create them only to remove them again later.


+nsresult
+nsXULPopupListener::RemoveGeneratedContent(nsIContent* aPopupContent,
+                                           PRBool aFlag)

aFlag needs a better name, along with a comment describing what it does.


+{
+  // copied from nsXULContentBuilder::RemoveGeneratedContent()

As an aside, I wonder how feasible a template implementation of this would be?


@@ -100,16 +100,17 @@ interface nsIDOMXULElement : nsIDOMEleme
   
   attribute boolean allowEvents;
 
   readonly attribute nsIRDFCompositeDataSource database;
   readonly attribute nsIXULTemplateBuilder     builder;
   readonly attribute nsIRDFResource            resource;
   readonly attribute nsIControllers            controllers;
   readonly attribute nsIBoxObject              boxObject;
+  readonly attribute nsIDOMNode                pageMenu;

As this property is specific to <browser> (or <iframe> I suppose), it would be better placed on nsIContainerBoxObject.
But as this is only used as a minor performance optimization, I'm not sure it's worth adding at all, and instead just compute it again.
Of course, if the code was implemented in a script, that wouldn't be a concern.

> I think this is a feature that should be natively implemented, so it can be reused in any xul based app.

You could add it as a script in toolkit/content and load it on demand only if the feature was used.


> C++ implementation should be faster. I don't know if it is possible to use interface/object hash tables in JS.
> The current C++ implementation maps xul menu items directly to html elements.

In script, you can just set a property on the menu/menuseparator/menuitem instead.
You only seem to use it to handle the command event and to remove the content later, both of which can
just check the property.
Comment 58 Jan Varga [:janv] 2011-06-14 14:28:10 PDT
(In reply to comment #57)
> You'll want to add the event to nsDOMClassInfo::DefineStaticJSVals and
> nsEventReceiverSH::ReallyIsEventName as well.
> 
> 

ok

> diff --git a/browser/base/content/nsContextMenu.js
> b/browser/base/content/nsContextMenu.js
> +    this.xulMenu = aXulMenu;
>      this.browser = aBrowser;
> +
> +    if (this.initStandalonePageMenu())
> +      return;
> 
> Just pass xulMenu to initStandalonePageMenu and don't store it on 'this'.
> 

ok

> 
> diff --git a/content/xul/content/src/nsXULElement.cpp
> b/content/xul/content/src/nsXULElement.cpp
>    nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
> -  if (!pm)
> +  if (!pm || pm->IsPopupOpen(popup))
>      return NS_OK;
> 
> +  nsCOMPtr<nsIDOMMouseEvent> mouseEvent = do_QueryInterface(aEvent);
> +  PRBool shiftKey;
> +  mouseEvent->GetShiftKey(&shiftKey);
> +  if (!shiftKey) {
> +    nsCOMPtr<nsIDOMEventTarget> target;
> +    aEvent->GetTarget(getter_AddRefs(target));
> +    nsCOMPtr<nsIContent> triggerContent = do_QueryInterface(target);
> +    if (triggerContent)
> +      HandleContent(triggerContent, popup, getter_AddRefs(mPageMenu));
> +  }
> +
> 
> Should this only happen for context menus?


yeah, I'll fix that

> 
> +nsresult
> +nsXULPopupListener::HandleContent(nsIContent* aTriggerContent,
> +                                  nsIContent* aPopupContent,
> +                                  nsIDOMNode** aPageMenu)
> ...
> +  nsEvent event(PR_TRUE, NS_SHOW_EVENT);
> +  event.flags |= NS_EVENT_FLAG_CANT_CANCEL;
> +  event.flags |= NS_EVENT_FLAG_CANT_BUBBLE;
> 
> Just put this on one line.
> 

ok

> 
> +  nsCOMPtr<nsIPresShell> shell = document->GetShell();
> +  if (!shell)
> +    return NS_ERROR_FAILURE;
> + 
> +  nsRefPtr<nsPresContext> presContext = shell->GetPresContext();
> +  nsEventStatus status = nsEventStatus_eIgnore;
> +  nsEventDispatcher::Dispatch(pageMenuContent, presContext, &event, nsnull,
> +                              &status);
> +
> +  nsresult rv = GenerateContent(pageMenuContent, aPopupContent);
> +  if (NS_FAILED(rv)) {
> +    if (mPageMenuTable)
> +      mPageMenuTable->Clear();
> +    return rv;
> +  }
> 
> Is the content supposed to be generated only when the show event wasn't
> cancelled?
> 

Well, one would think so ...
but the event is not cancelable (see NS_EVENT_FLAG_CANT_CANCEL flag above)
The spec says:
"If the element or one of its ancestors does have a context menu assigned, then the user agent must fire a simple event named show at the menu element of the context menu of the nearest ancestor (including the element itself) with one assigned.

The default action of this event is that the user agent must show a context menu built from the menu element."

"Firing a simple event named e means that an event with the name e, which does not bubble (except where otherwise stated) and is not cancelable (except where otherwise stated), and which uses the Event interface, must be created and dispatched at the given target."


> 
> +nsresult
> +nsXULPopupListener::FindPageMenu(nsIContent* aTriggerContent,
> ...
> +  if (!aTriggerContent->IsHTML())
> +    return NS_OK;
> +
> +  nsCOMPtr<nsIDOMHTMLMenuElement> menuElement;
> 
> I'd move this right inside the loop where it's used.
> 

ok

> 
> +  nsAutoString pagemenupos;
> +  insertionPoint->GetAttr(kNameSpaceID_None, nsGkAtoms::pagemenupos,
> +                          pagemenupos);
> 
> Rather than have two seperate attributes 'pagemenupopup' and 'pagemenupos',
> I'd just use one for both uses: pagemenu = "start"
> 

ok

> 
> +  if (pagemenupos.IsEmpty() || pagemenupos.EqualsLiteral("start")) {
> +    nsCOMPtr<nsIContent> refContent = insertionPoint->GetFirstChild();
> +    insertionPoint->InsertBefore(fragmentContent, refContent, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +  else if (pagemenupos.EqualsLiteral("end")) {
> +    insertionPoint->AppendChild(fragmentContent, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  return NS_OK;
> 
> Remove the two NS_ENSURE_SUCCESS lines and just return rv here.
> 

ok

> 
> +    else if (child->IsHTML(nsGkAtoms::option)) {
> +      nsAutoString text;
> +      nsContentUtils::GetNodeTextContent(child, PR_TRUE, text);
> +
> +      if (child->HasAttr(kNameSpaceID_None, nsGkAtoms::value) &&
> +          child->AttrValueIs(kNameSpaceID_None, nsGkAtoms::value,
> +                             EmptyString(), eIgnoreCase) &&
> +          child->HasAttr(kNameSpaceID_None, nsGkAtoms::disabled) &&
> +          CountCharInReadable(text, PRUnichar('-')) == text.Length()) {
> +        rv = CreateSeparator(aDocument, child, aPopupContent);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +      }
> +    }
> 
> What a bizarre spec requirement.

yeah ...
"An option element that has a value attribute set to the empty string, and has a disabled attribute, and whose textContent consists of a string of one or more hyphens (U+002D HYPHEN-MINUS)
"
it seems that the HasAttr() check is useless, I'll remove it

> 
> 
> +nsresult
> +nsXULPopupListener::CreateElement(nsIDocument* aDocument,
> +                                  nsIAtom* aTag, nsIContent** aResult)
> ...
> +  (*aResult)->SetAttr(kNameSpaceID_None, nsGkAtoms::pagemenuitem,
> +                      EmptyString(), PR_FALSE);
> 
> Is there a reason to use non-notifying modifications for all the SetAttr
> calls?

I think it's useless to notify observers when the produced document fragment is not in a document. That's another reason why it should be a little bit faster this way.


> 
> 
> +nsresult
> +nsXULPopupListener::CreateSubmenu(nsIDocument* aDocument,
> +                                  nsIContent* aHTMLContent,
> +                                  nsIContent* aPopupContent)
> ...
> +  rv = CreateMenu(aDocument, aHTMLContent, menuPopup);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return NS_OK;
> 
> Again, as in a few other places, just return rv here.
> 

ok

> 
> +nsresult
> +nsXULPopupListener::PostprocessGeneratedContent(nsIContent* aPopupContent)
> +{
> 
> I think it would be better to maintain a flag that indicates if the last
> node generated was a separator, and
> not add duplicates, rather than create them only to remove them again later.
> 

I'll try to rework it ...

> 
> +nsresult
> +nsXULPopupListener::RemoveGeneratedContent(nsIContent* aPopupContent,
> +                                           PRBool aFlag)
> 
> aFlag needs a better name, along with a comment describing what it does.
> 

ok


> 
> +{
> +  // copied from nsXULContentBuilder::RemoveGeneratedContent()
> 
> As an aside, I wonder how feasible a template implementation of this would
> be?

it should be possible, of course
however, I've had a feeling that it's not preferred anymore in browser/
Instead of building xul elements directly, we would have to build a datasource.
and it would be really nice to produce JSON and then just let it built automatically by template builder :)

> 
> 
> @@ -100,16 +100,17 @@ interface nsIDOMXULElement : nsIDOMEleme
>    
>    attribute boolean allowEvents;
>  
>    readonly attribute nsIRDFCompositeDataSource database;
>    readonly attribute nsIXULTemplateBuilder     builder;
>    readonly attribute nsIRDFResource            resource;
>    readonly attribute nsIControllers            controllers;
>    readonly attribute nsIBoxObject              boxObject;
> +  readonly attribute nsIDOMNode                pageMenu;
> 
> As this property is specific to <browser> (or <iframe> I suppose), it would
> be better placed on nsIContainerBoxObject.

well, it works on any element:
<vbox contextmenu="myPageMenu" flex="1"
      xmlns:html="http://www.w3.org/1999/xhtml">
  <html:div contextmenu="htmlMenu">
    <html:p>Click me</html:p>
  </html:div>
  <html:menu type="context" id="htmlMenu">
    <html:a href="http://www.mozilla.org">Go to mozilla.org</html:a>
  </html:menu>                                                         
</vbox>


> But as this is only used as a minor performance optimization, I'm not sure
> it's worth adding at all, and instead just compute it again.
> Of course, if the code was implemented in a script, that wouldn't be a
> concern.

well, the attribute is non null, only if page menu generation was successful
so it's not just a perf optimization

> 
> > I think this is a feature that should be natively implemented, so it can be reused in any xul based app.
> 
> You could add it as a script in toolkit/content and load it on demand only
> if the feature was used.
> 

right ...
anyway, when I originally designed it, I thought it would be nice to just put an attribute on <menupopup> to enable html5 context menus in XUL

> 
> > C++ implementation should be faster. I don't know if it is possible to use interface/object hash tables in JS.
> > The current C++ implementation maps xul menu items directly to html elements.
> 
> In script, you can just set a property on the menu/menuseparator/menuitem
> instead.
> You only seem to use it to handle the command event and to remove the
> content later, both of which can
> just check the property.

yeah that should work
I know JS is much faster these days, however we can avoid comparing most of the strings in C++ like command type, attribute atoms when building the menu as I already mentioned.
IIRC, nsContextMenu doesn't build any menu items, it just shows/hides them

later, we can add another layer (e.g. JSON) between html and the app, so it can be used in other frontends too
Comment 59 Neil Deakin 2011-06-14 15:42:01 PDT
> Well, one would think so ...
> but the event is not cancelable (see NS_EVENT_FLAG_CANT_CANCEL flag above)
> The spec says:
> "If the element or one of its ancestors does have a context menu assigned,
> then the user agent must fire a simple event named show at the menu element
> of the context menu of the nearest ancestor (including the element itself)
> with one assigned.
> 
> The default action of this event is that the user agent must show a context
> menu built from the menu element."

You might want to clarify if that is intended, since cancelling the event using preventDefault normally means don't do the default action.

> > As this property is specific to <browser> (or <iframe> I suppose), it would
> > be better placed on nsIContainerBoxObject.
> 
> well, it works on any element:
> <vbox contextmenu="myPageMenu" flex="1"
>       xmlns:html="http://www.w3.org/1999/xhtml">
>   <html:div contextmenu="htmlMenu">
>     <html:p>Click me</html:p>
>   </html:div>
>   <html:menu type="context" id="htmlMenu">
>     <html:a href="http://www.mozilla.org">Go to mozilla.org</html:a>
>   </html:menu>                                                         
> </vbox>
> 

Of course, there the right way would be to just use a regular context menu using a <menupopup>.

But even if not, I don't think it's worth adding a property to every XUL element for this very specific case.
Comment 60 Jan Varga [:janv] 2011-06-14 18:18:52 PDT
(In reply to comment #59)
> > Well, one would think so ...
> > but the event is not cancelable (see NS_EVENT_FLAG_CANT_CANCEL flag above)
> > The spec says:
> > "If the element or one of its ancestors does have a context menu assigned,
> > then the user agent must fire a simple event named show at the menu element
> > of the context menu of the nearest ancestor (including the element itself)
> > with one assigned.
> > 
> > The default action of this event is that the user agent must show a context
> > menu built from the menu element."
> 
> You might want to clarify if that is intended, since cancelling the event
> using preventDefault normally means don't do the default action.
> 

ok

> > > As this property is specific to <browser> (or <iframe> I suppose), it would
> > > be better placed on nsIContainerBoxObject.
> > 
> > well, it works on any element:
> > <vbox contextmenu="myPageMenu" flex="1"
> >       xmlns:html="http://www.w3.org/1999/xhtml">
> >   <html:div contextmenu="htmlMenu">
> >     <html:p>Click me</html:p>
> >   </html:div>
> >   <html:menu type="context" id="htmlMenu">
> >     <html:a href="http://www.mozilla.org">Go to mozilla.org</html:a>
> >   </html:menu>                                                         
> > </vbox>
> > 
> 
> Of course, there the right way would be to just use a regular context menu
> using a <menupopup>.
> 
> But even if not, I don't think it's worth adding a property to every XUL
> element for this very specific case.

ok
Comment 61 Mounir Lamouri (:mounir) 2011-06-15 02:22:02 PDT
Comment on attachment 539268 [details] [diff] [review]
patch for event dispatcher v3 (ready to be pushed)

Review of attachment 539268 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately, I think the test should be fixed before I push this.
In addition, could you provide a commit message for this patch, see: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

::: content/events/test/test_bug617528.xul
@@ +23,5 @@
> +    var _window;
> +    var browser;
> +    var event;
> +
> +    function start() {

Instead, you can do: addLoadEvent(function() {
and remove onload=start(). It should have the same behavior but is easier to read.

@@ +68,5 @@
> +
> +      browser.addEventListener("contextmenu", onContextMenu, false);
> +
> +      wu.sendMouseEvent("contextmenu", left, top, 2, 1, 0);
> +      is(event.defaultPrevented, true,

If I understand this test correctly, this is not correct: you send the mouse event and when you get the contextmenu event, you set |event| but you are using |event| just after sending the mouse event. I think it's a very bad practice than can lead to random oranges.
Instead, you should move everything below |sendMouseEvent| inside |onContextMenu|.
Comment 62 Jan Varga [:janv] 2011-06-15 02:46:08 PDT
(In reply to comment #61)
> Comment on attachment 539268 [details] [diff] [review] [review]
> patch for event dispatcher v3 (ready to be pushed)
> 
> Review of attachment 539268 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Unfortunately, I think the test should be fixed before I push this.
> In addition, could you provide a commit message for this patch, see:
> http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
> 
> ::: content/events/test/test_bug617528.xul
> @@ +23,5 @@
> > +    var _window;
> > +    var browser;
> > +    var event;
> > +
> > +    function start() {
> 
> Instead, you can do: addLoadEvent(function() {
> and remove onload=start(). It should have the same behavior but is easier to
> read.

well, I checked other tests in the tree and they use <body|window onload>,
window.onload or addLoadEvent()

no problem, I can change it to addLoadEvent()


> 
> @@ +68,5 @@
> > +
> > +      browser.addEventListener("contextmenu", onContextMenu, false);
> > +
> > +      wu.sendMouseEvent("contextmenu", left, top, 2, 1, 0);
> > +      is(event.defaultPrevented, true,
> 
> If I understand this test correctly, this is not correct: you send the mouse
> event and when you get the contextmenu event, you set |event| but you are
> using |event| just after sending the mouse event. I think it's a very bad
> practice than can lead to random oranges.
> Instead, you should move everything below |sendMouseEvent| inside
> |onContextMenu|.

Again, this is something that works ok (I ran mochitest-chrome and mochitest-plain locally) and I just found out that the same technique is used for example in content/events/test/test_bug545268.html

sendMouseEvent is synchronous so it should block code execution until onContextMenu() is finished

IMO, it would be actually a bug if it finished later
Comment 63 Jan Varga [:janv] 2011-06-15 08:00:59 PDT
Created attachment 539521 [details] [diff] [review]
Part 1 - Dispatch shift right click (contextmenu event) only to chrome (ready to be pushed)
Comment 64 Jan Varga [:janv] 2011-06-19 09:49:05 PDT
http://hg.mozilla.org/mozilla-central/rev/6d2cd42969f9
Comment 65 Jan Varga [:janv] 2011-06-20 01:42:13 PDT
It seems the spec will change: 
http://www.w3.org/TR/html5/elements.html#elements
http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#elements

See "?" after .contextMenu, .commandType, .label, .icon
Comment 66 Jan Varga [:janv] 2011-06-20 03:16:22 PDT
ok, it just means the attribute can return null
Comment 67 Michael[tm] Smith 2011-06-20 04:26:42 PDT
(In reply to comment #66)
> ok, it just means the attribute can return null

Yeah, and fwiw, because it's due to a change that was made to the WebIDL spec, IDLs in other specs that use WebIDL are going to be updated similarly (some already have been)
Comment 68 Jan Varga [:janv] 2011-06-20 10:37:10 PDT
filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=12999
for the nodefault attribute
Comment 69 Jan Varga [:janv] 2011-06-21 09:59:35 PDT
Created attachment 540787 [details] [diff] [review]
Part 2 - Add support for HTML5 Commands
Comment 70 Jan Varga [:janv] 2011-06-21 10:28:05 PDT
I will change those |todo_is| to |is| in the test ...
I talked to bz and he says it is actually correct behavior to return overridden attributes in this case.
Comment 71 Mounir Lamouri (:mounir) 2011-06-23 07:35:29 PDT
Comment on attachment 540787 [details] [diff] [review]
Part 2 - Add support for HTML5 Commands

Review of attachment 540787 [details] [diff] [review]:
-----------------------------------------------------------------

In my opinion, we shouldn't implement this specification in its current state: there are a few minor issues I pointed during the review but also major ones.
First of all, we are adding a lot of IDL attributes to HTMLElement (btw, you did forgot two of them: accessKeyLabel and title). This is polluting the HTMLElement interface for no reason. We could instead add an IDL attribute named 'commandState' (or whatever you want) which would be an object with all these attributes. In addition of de-polluting the HTMLELement interface it will also help identifying if a specific element is a command or not by checking if |.commandState| is null.
In addition, I don't really like how HTMLCommandElement is redifining basically all IDL attributes that already are in HTMLElement (which HTMLCommandElement inherits from). I think it would be nice to find a way to have HTMLCommandElement just inherits from HTMLElement with no addition. The only attribute in HTMLCommandElement that isn't present in HTMLElement is radiogroup but I believe that use cases could be fully moved to <input type='radio'> I actually don't understand why HTMLCommandElement can be in the 'radio' or 'checked' states. Is there something I'm missing here?

For the tests, it's great to have some but I think you could make those easier to read (and to write) by just declaring cases to test and have a loop testing each case. It would be great to update your tests that way. By the way, it's better to name your test instead of putting a bug number when you are actually adding a feature instead of fixing a bug.

Given the specs change I think should be done, I did read less carefully some parts of the code.

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +2395,5 @@
>    *aContentEditable = PR_FALSE;
>    return NS_OK;
>  }
>  
> +nsresult

NS_IMETHODIMP

@@ +2398,5 @@
>  
> +nsresult
> +nsGenericHTMLElement::GetContextMenu(nsIDOMHTMLMenuElement** aContextMenu)
> +{
> +  NS_ENSURE_ARG_POINTER(aContextMenu);

You don't need that. AFAIK, aContextMenu can't be null.

@@ +2410,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsIDocument* doc = GetCurrentDoc();
> +  if (doc) {

if (!doc) {
  return NS_OK;
}

@@ +2419,5 @@
> +
> +  return NS_OK;
> +}
> +
> +nsresult

NS_IMETHODIMP

@@ +2428,5 @@
> +  nsAutoString id;
> +  aContextMenu->GetId(id);
> +
> +  return SetAttr(kNameSpaceID_None, nsGkAtoms::contextmenu, id, PR_TRUE);
> +}

I think we should have element.contextMenu being readonly and have it behave like input.list: you set the content attribute with an ID and read the IDL attribute that returns an element.
Basically, for you, that would mean removing this method and change the IDL to make it readonly. Though, I think it would be better to wait for Olli's opinion before doing so.

@@ +2473,5 @@
> +
> +  return CMD_TYPE_NONE;
> +}
> +
> +nsresult

NS_IMETHODIMP

@@ +2501,5 @@
> +NS_IMETHODIMP
> +nsGenericHTMLElement::GetLabel(nsAString& aLabel)
> +{
> +  if (DoGetCmdType() == CMD_TYPE_NONE && HasFlag(NODE_HAS_ACCESSKEY)) {
> +    // XXXvarga check .labels here once it's implemented (bug 556743)

Don't use XXX but TODO with a bug number.

::: content/html/content/src/nsGenericHTMLElement.h
@@ +156,5 @@
>    // Callback for destructor of of dataset to ensure to null out weak pointer.
>    nsresult ClearDataset();
> +  nsresult GetContextMenu(nsIDOMHTMLMenuElement** aContextMenu);
> +  nsresult SetContextMenu(nsIDOMHTMLMenuElement* aContextMenu);
> +  nsresult GetCommandType(nsAString& aCommandType);

I might be missing something but why not NS_IMETHOD?

@@ +542,5 @@
> +    CMD_TYPE_NONE               = 0,
> +    CMD_TYPE_RADIO              = 1,
> +    CMD_TYPE_CHECKBOX           = 2,
> +    CMD_TYPE_COMMAND            = 3
> +  };

No need to set the values for each type. You can set the first one to 0 if you need it but you don't have to set the others in any case.

@@ +544,5 @@
> +    CMD_TYPE_CHECKBOX           = 2,
> +    CMD_TYPE_COMMAND            = 3
> +  };
> +
> +  PRUint32 GetCmdType();

This method can be const.

@@ +550,2 @@
>  protected:
> +  virtual PRUint32 DoGetCmdType();

This one too could be const I believe.
By the way, maybe you should rename it GetCmdTypeInternal()?

@@ +1518,5 @@
> +  NS_SCRIPTABLE NS_IMETHOD SetDir(const nsAString & aDir) { return _to SetDir(aDir); } \
> +  NS_SCRIPTABLE NS_IMETHOD GetClassName(nsAString & aClassName) { return _to GetClassName(aClassName); } \
> +  NS_SCRIPTABLE NS_IMETHOD SetClassName(const nsAString & aClassName) { return _to SetClassName(aClassName); } \
> +  NS_SCRIPTABLE NS_IMETHOD GetAccessKey(nsAString & aAccessKey) { return _to GetAccessKey(aAccessKey); } \
> +  NS_SCRIPTABLE NS_IMETHOD SetAccessKey(const nsAString & aAccessKey) { return _to SetAccessKey(aAccessKey); } \

There is a change here? If not, could you remove this chunk.

::: content/html/content/src/nsHTMLAnchorElement.cpp
@@ +201,5 @@
> +
> +NS_IMETHODIMP
> +nsHTMLAnchorElement::GetLabel(nsAString& aLabel)
> +{
> +  if (DoGetCmdType() != CMD_TYPE_NONE) {

Maybe you could add a protected inline "bool IsCommand() const" method to nsGenericHTMLElement.h

::: content/html/content/src/nsHTMLCommandElement.cpp
@@ +50,5 @@
> +NS_IMPL_NS_NEW_HTML_ELEMENT(Command)
> +
> +
> +nsHTMLCommandElement::nsHTMLCommandElement(already_AddRefed<nsINodeInfo> aNodeInfo)
> +  : nsGenericHTMLElement(aNodeInfo), mType(CMD_TYPE_COMMAND)

: nsGenericHTMLELement(aNodeInfo)
, mType(kCommandDefaultType->value)

@@ +91,5 @@
> +      UnsetAttr(kNameSpaceID_None, nsGkAtoms::checked, PR_TRUE);
> +    } else {
> +      SetAttr(kNameSpaceID_None, nsGkAtoms::checked,
> +              NS_LITERAL_STRING("checked"), PR_TRUE);
> +    }

I think the specifications should be changed and we shouldn't set "checked". The attribute reflection for a boolean requires us to set the empty string when a boolean attribute is set, not the attribute name.
So, that should be:
SetChecked(!HasAttr());

@@ +100,5 @@
> +    nsCOMPtr<nsIContent> parent = GetParent();
> +
> +    nsCOMPtr<nsIContent> cur;
> +    for (cur = parent->GetFirstChild(); cur; cur = cur->GetNextSibling()) {
> +      if (cur != this && cur->IsHTML(nsGkAtoms::command)) {

Change that to:
if (cur == this || !cur->IsHTML(nsGkAtoms::command)) {
  continue;
}

@@ +112,5 @@
> +    }
> +
> +
> +    SetAttr(kNameSpaceID_None, nsGkAtoms::checked,
> +            NS_LITERAL_STRING("checked"), PR_TRUE);

SetChecked(PR_TRUE);

@@ +138,5 @@
> +  } else {
> +    SetDOMStringToNull(aIcon);
> +  }
> +  return NS_OK;
> +}

Wow... The command API says .icon should return null if there is no icon but HTMLCommand says .icon should reflect the content attribute (thus should return the empty string if the URL is invalid or null).
Then, I believe you should use NS_IMPL_URI_ATTR.

::: content/html/content/src/nsHTMLLegendElement.cpp
@@ +155,5 @@
> +nsHTMLLegendElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aAttribute,
> +                               PRBool aNotify)
> +{
> +  return nsGenericHTMLElement::UnsetAttr(aNameSpaceID, aAttribute, aNotify);
> +}

Did you change something here?

::: content/html/content/src/nsHTMLSharedElement.cpp
@@ +261,5 @@
>  NS_IMPL_BOOL_ATTR(nsHTMLSharedElement, Compact, compact)
>  
>  // nsIDOMHTMLMenuElement
>  //NS_IMPL_BOOL_ATTR(nsHTMLSharedElement, Compact, compact)
> +NS_IMPL_STRING_ATTR(nsHTMLSharedElement, Label, label)

You forgot the type reflection. It's an enumerated attribute and should be reflected as such. Though, I see nsHTMLSharedElement already have a type attribute for HTMLParamElement. In that case, you might want to manage both reflection in one method (thus, not using the macros) or create a nsHTMLMenuElement.cpp.

::: dom/interfaces/html/nsIDOMHTMLCommandElement.idl
@@ +11,5 @@
> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> + * for the specific language governing rights and limitations under the
> + * License.
> + *
> + * The Original Code is mozilla.org code.

Mozilla, not mozilla.org.

::: dom/interfaces/html/nsIDOMHTMLMenuElement.idl
@@ +54,3 @@
>  interface nsIDOMHTMLMenuElement : nsIDOMHTMLElement
>  {
>             attribute boolean          compact;

Do you know why we have this attribute? It looks like it's not in the spec. Maybe you could open a follow-up to remove it?

It's possible that HTMLMenuElement was used for something else before. In that case, maybe you should check that the entry in nsHTMLEditUtils and nsHTMLTags are correct.

::: editor/libeditor/html/nsHTMLEditUtils.cpp
@@ +605,5 @@
>    ELEM(code, PR_TRUE, PR_TRUE, GROUP_PHRASE, GROUP_INLINE_ELEMENT),
>    ELEM(col, PR_FALSE, PR_FALSE, GROUP_TABLE_CONTENT | GROUP_COLGROUP_CONTENT,
>         GROUP_NONE),
>    ELEM(colgroup, PR_TRUE, PR_FALSE, GROUP_NONE, GROUP_COLGROUP_CONTENT),
> +  ELEM(command, PR_FALSE, PR_FALSE, GROUP_NONE, GROUP_NONE),

This change is going to require Ehsan's review I believe.

::: parser/htmlparser/src/nsElementTable.cpp
@@ +448,5 @@
> +    /*autoclose starttags and endtags*/ 0,0,0,0,
> +    /*parent,incl,exclgroups*/          kFlowEntity|kHeadMisc, kNone, kNone,
> +    /*special props, prop-range*/       kNonContainer,kDefaultPropRange,
> +    /*special parents,kids*/            0, 0,
> +  },

You are going to need mrbkap review for this change.
Comment 72 David Zbarsky (:dzbarsky) 2011-06-23 08:07:11 PDT
Note that accessKeyLabel is being implemented in bug 583533.
Comment 73 Jan Varga [:janv] 2011-06-23 10:10:12 PDT
Do we really want to customize the spec ?
Shouldn't we just wait for the final version of the spec and fix the implementation after that ?
Otherwise we will spend a lot of time by changing implementation. The current patch tries to follow the spec as much as possible. I know, the are many unresolved issues in the spec, but does it make sense to change implementation at this point ?
The only consumer of command API is context menu building code. We can make command API private for now if you want, that would be safe.

I think, those people who are writing these specs are waiting for the first implementation so they can test it and make modifications to the spec if it's needed. So let's provide the first/initial implementation which actually follows the spec and move forward.

Just my 2 cents.

P.S. I'll comment on the technical stuff later today
Comment 74 Mounir Lamouri (:mounir) 2011-06-23 10:26:09 PDT
We shouldn't implement something just because the specs say so. Unfortunately, the specs are sometimes insane and the best way to make that clear for editors is to point out the issues and not do a bad implementation. Usually, two options follow: not implementing what we think is wrongly specified or implement including the changes we ask for depending on how confident we are about those changes. For such a big feature, I do now what the best approach is.
I do not think editors are waiting for implementations to review their own specifications. Usually, they consider that if an implementation follows the specs, that means they agree and it leads to situation where an implementor ask for a change and the spec editor points that all other implementations are following what the specifications currently say. Then, depending on your request and how used the feature is, the spec might never change.
Comment 75 Jan Varga [:janv] 2011-06-23 11:13:45 PDT
(In reply to comment #71)
> >  
> > +nsresult
> 
> NS_IMETHODIMP

why ? isn't this an optimization (to avoid a virtual method) ?
did you notice the comment above GetId() ?

> 
> @@ +2398,5 @@
> >  
> > +nsresult
> > +nsGenericHTMLElement::GetContextMenu(nsIDOMHTMLMenuElement** aContextMenu)
> > +{
> > +  NS_ENSURE_ARG_POINTER(aContextMenu);
> 
> You don't need that. AFAIK, aContextMenu can't be null.

how come ?
what about C++ callers, can't they pass null ?

> 
> @@ +2410,5 @@
> > +    return NS_OK;
> > +  }
> > +
> > +  nsIDocument* doc = GetCurrentDoc();
> > +  if (doc) {
> 
> if (!doc) {
>   return NS_OK;
> }
> 

why ?
the indentation is only 2 spaces here


> I might be missing something but why not NS_IMETHOD?

yep, see my comment above

> 
> @@ +542,5 @@
> > +    CMD_TYPE_NONE               = 0,
> > +    CMD_TYPE_RADIO              = 1,
> > +    CMD_TYPE_CHECKBOX           = 2,
> > +    CMD_TYPE_COMMAND            = 3
> > +  };
> 
> No need to set the values for each type. You can set the first one to 0 if
> you need it but you don't have to set the others in any case.
> 

isn't this more readable ?

> 
> @@ +1518,5 @@
> > +  NS_SCRIPTABLE NS_IMETHOD SetDir(const nsAString & aDir) { return _to SetDir(aDir); } \
> > +  NS_SCRIPTABLE NS_IMETHOD GetClassName(nsAString & aClassName) { return _to GetClassName(aClassName); } \
> > +  NS_SCRIPTABLE NS_IMETHOD SetClassName(const nsAString & aClassName) { return _to SetClassName(aClassName); } \
> > +  NS_SCRIPTABLE NS_IMETHOD GetAccessKey(nsAString & aAccessKey) { return _to GetAccessKey(aAccessKey); } \
> > +  NS_SCRIPTABLE NS_IMETHOD SetAccessKey(const nsAString & aAccessKey) { return _to SetAccessKey(aAccessKey); } \
> 
> There is a change here? If not, could you remove this chunk.

there are mixed line endings in this file (win/unix)

> 
> Did you change something here?

mixed line endings and extra white space

and the "title" attribute is already part of nsIDOMHTMLElement
accessKeyLabel is part of another bug
Comment 76 Jan Varga [:janv] 2011-06-23 11:35:27 PDT
(In reply to comment #74)
> I do not think editors are waiting for implementations to review their own
> specifications. Usually, they consider that if an implementation follows the
> specs, that means they agree and it leads to situation where an implementor
> ask for a change and the spec editor points that all other implementations
> are following what the specifications currently say. Then, depending on your
> request and how used the feature is, the spec might never change.

There are already bugs filed for most of the issues, so they know about it.
They know we don't like it in the current state. We can also remove/disable implementation if they are not willing to fix the spec. No problem.

The problem is that we don't know if the proposed changes are correct. This stuff is rather complicated, everything is related to everything.

If the current spec of command API is generally unacceptable, but we need it for context menu feature (it doesn't make sense to rework it just for this feature), let's make it internal API and fix it later when things settle down.
Comment 77 :Ms2ger (⌚ UTC+1/+2) 2011-06-23 13:12:17 PDT
(In reply to comment #71)
> ::: dom/interfaces/html/nsIDOMHTMLMenuElement.idl
> @@ +54,3 @@
> >  interface nsIDOMHTMLMenuElement : nsIDOMHTMLElement
> >  {
> >             attribute boolean          compact;
> 
> Do you know why we have this attribute? It looks like it's not in the spec.
> Maybe you could open a follow-up to remove it?

http://www.whatwg.org/specs/web-apps/current-work/multipage/obsolete.html#dom-menu-compact

(In reply to comment #75)
> > 
> > @@ +2398,5 @@
> > >  
> > > +nsresult
> > > +nsGenericHTMLElement::GetContextMenu(nsIDOMHTMLMenuElement** aContextMenu)
> > > +{
> > > +  NS_ENSURE_ARG_POINTER(aContextMenu);
> > 
> > You don't need that. AFAIK, aContextMenu can't be null.
> 
> how come ?
> what about C++ callers, can't they pass null ?

If they do that, they deserve to crash.

> > 
> > @@ +2410,5 @@
> > > +    return NS_OK;
> > > +  }
> > > +
> > > +  nsIDocument* doc = GetCurrentDoc();
> > > +  if (doc) {
> > 
> > if (!doc) {
> >   return NS_OK;
> > }
> > 
> 
> why ?
> the indentation is only 2 spaces here

It makes the expected code flow clearer.
Comment 78 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-06-23 13:14:23 PDT
(In reply to comment #73)
> Shouldn't we just wait for the final version of the spec and fix the
> implementation after that ?
Final version of the HTML spec, which is defined as "Living Standard"?
;)
Comment 79 Jan Varga [:janv] 2011-06-23 13:42:35 PDT
(In reply to comment #78)
> (In reply to comment #73)
> > Shouldn't we just wait for the final version of the spec and fix the
> > implementation after that ?
> Final version of the HTML spec, which is defined as "Living Standard"?
> ;)

yeah, I take that :)
I actually meant to wait for the command API and related issues to be resolved
Comment 80 Jan Varga [:janv] 2011-06-23 13:51:20 PDT
(In reply to comment #77)
> > > @@ +2410,5 @@
> > > > +    return NS_OK;
> > > > +  }
> > > > +
> > > > +  nsIDocument* doc = GetCurrentDoc();
> > > > +  if (doc) {
> > > 
> > > if (!doc) {
> > >   return NS_OK;
> > > }
> > > 
> > 
> > why ?
> > the indentation is only 2 spaces here
> 
> It makes the expected code flow clearer.

oh man, code of this method is just so simple
and it's the last block before return NS_OK;
I expect that somebody else would say, change it to |if (doc)| :)

it's just a question of taste

I have no problem w/ changing it to |if (!doc)| but it looks to me that this is nit of all nits :)
Comment 81 Mounir Lamouri (:mounir) 2011-06-23 14:43:04 PDT
(In reply to comment #80)
> oh man, code of this method is just so simple
> and it's the last block before return NS_OK;
> I expect that somebody else would say, change it to |if (doc)| :)
> 
> it's just a question of taste
> 
> I have no problem w/ changing it to |if (!doc)| but it looks to me that this
> is nit of all nits :)

Sure, some remarks were only nits, or coding stye guidance. I wouldn't mind if you want to keep |if (doc) {| in that case but generally, it's better to do the opposite to improve readability.

For the specs, I think some issues have been raised but unlikely all of them (like introducing CommandState). Olli, which process would you recommend?
Comment 82 Jan Varga [:janv] 2011-06-23 23:50:39 PDT
Well, it looks to me there's no optimal solution for the command API, so we have to find a good compromise

command API is not only those 5 new attributes, there are others facets like .id, .title, .hidden, etc.

If we move only those 5 attributes to .commandState it will be inconsistent.
Actually, what would we get if we moved it to .commandState ?
Is it a problem to have additional 5 attributes ? performance, memory footprint ?

The real issue is clashing/shadowing attribute names as bz pointed out in http://www.w3.org/Bugs/Public/show_bug.cgi?id=12845

.commandState solves this problem in a way, but as I said it introduces other inconsistency 

However, in my opinion it's not so bad, some attributes are just overriden by elements which inherit from HTMLElement

If you want to know if an element defines a command, just check .commandType for null. You can't check .label for null because that attribute is overriden e.g. by HTMLOptionElement, so it returns a label or the empty string even if it doesn't define a command.
Actually, it has to do it, otherwise it would break existing content.

The semantic of these new attributes for command API is almost the same as semantic of attributes on elements which inherit from HTMLElement.

There are only some cases where it's not perfect (consistent).

We could rename it to e.g. GetCommandLabel(), but it looks ugly IMO and it solves only these small inconsistencies
Comment 83 Hixie (not reading bugmail) 2011-06-24 11:56:21 PDT
The API is being discussed in http://www.w3.org/Bugs/Public/show_bug.cgi?id=12844 — in particular, the shadowing at the IDL level should be able to be removed (though probably still with different definitions of the attributes for different elements, since the whole point here is that some aspects of the command API are read-only on some elements and not on others, and some aspects are implemented differently on different elements — e.g. the label of an <input type=button>, an <input type=radio>, and a <button> are all quite different, but we want them all exposed with the same API so that one doesn't have to check what kind of element it is each time).
Comment 84 Jan Varga [:janv] 2011-06-24 16:57:57 PDT
(In reply to comment #71)
> In addition, I don't really like how HTMLCommandElement is redifining
> basically all IDL attributes that already are in HTMLElement (which
> HTMLCommandElement inherits from). I think it would be nice to find a way to
> have HTMLCommandElement just inherits from HTMLElement with no addition.

These attributes are actually read/write in HTMLCommandElement and readonly in HTMLElement, so they have to be declared that way

> The only attribute in HTMLCommandElement that isn't present in HTMLElement is
> radiogroup but I believe that use cases could be fully moved to <input
> type='radio'> I actually don't understand why HTMLCommandElement can be in
> the 'radio' or 'checked' states. Is there something I'm missing here?
> 

The specification just introduces a new element that can be used to define all 3 types of commands. The checked attribute is automatically updated when the action of command is triggered.

Why do you think it shouldn't support radio and checkbox type ?

<command> can be used to declare menus which don't render in browsers that have no support for it
The scope of the radiogroup is the child list of the parent element.
Comment 85 Jan Varga [:janv] 2011-06-28 05:13:15 PDT
Just for reference, here is some background behind html menus:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2005-October/004883.html
Comment 86 Jan Varga [:janv] 2011-06-30 03:06:34 PDT
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12845#c37

so does it mean that (for example) HTMLOptionElement .label should return null if the element doesn't define a command ?
Comment 87 Hixie (not reading bugmail) 2011-06-30 11:18:26 PDT
No, HTMLOptionElement.label should always return its label.
Comment 88 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-07-01 09:13:08 PDT
I'm having trouble to understand the need for <command> element.
Though, it could be significantly less-messy to create a menu if 
only <command> was accepted as its child. But in that case
other ways to defined a command shouldn't be supported.

The current menu creation is just strange.
Comment 89 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-07-01 09:40:59 PDT
OK, http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2004-August/001655.html
explains some of the awkwardness .
I'm just not sure "don't-require-CSS" is good enough reasons for 
<command>
Comment 90 Jan Varga [:janv] 2011-07-01 10:08:25 PDT
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-May/019579.html

... though support for this in the spec is awaiting implementation experience 
with the command system as a whole ...
Comment 91 Jan Varga [:janv] 2011-07-05 15:37:42 PDT
Comment on attachment 540787 [details] [diff] [review]
Part 2 - Add support for HTML5 Commands

Ok, so it seems <command> is meant to be the primary way of defining a command.
I'm finishing work on a new patch that avoids usage of the "controversial" command API (it uses <command> directly). However, on the other side, the patch is based on a new approach that will provide foundation for toolbar menus, custom context menus and also an easier migration to the multi process architecture.
Comment 92 Hixie (not reading bugmail) 2011-07-06 11:09:55 PDT
So long as you make it possible to use all the elements that create commands, that's ok... it rather defeats the point of the graceful degradation feature though if you only let <command> elements create commands!

As far as the command API is concerned, my understanding is that the changes the spec will be getting here are pretty minimal; there doesn't seem to be much controversy left. (The change is just be to make the getter on HTMLElement do the same as on the derived interface, rather than having two different getters.)
Comment 93 Jan Varga [:janv] 2011-07-06 11:35:58 PDT
Well, let's start with "basic" support first... I'll file a new bug for the command API, we can discuss the remaining issues there.

I'm currently waiting for the try server, will attach a patch soon.
Comment 94 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-07-06 11:45:27 PDT
(In reply to comment #92)
> So long as you make it possible to use all the elements that create
> commands, that's ok... it rather defeats the point of the graceful
> degradation feature though if you only let <command> elements create
> commands!
Well, the whole menu creation using non-<command> elements is strange,
and whoever uses it will probably always have to look at the spec to see which 
elements do what.
I hope the menu creation will be cleaned up significantly - though, as of now
I don't have a proposal how to do that, or what exactly needs to be changed.
Comment 95 Hixie (not reading bugmail) 2011-07-06 12:23:59 PDT
Implementation experience here will definitely help improve the spec, indeed.

The idea behind the current design is that someone can make a legacy fallback menu or toolbar and style it with CSS, but then in modern UAs have it work as a real menu, without having to duplicate everything. I don't think people will have much trouble working out what will work in <menu>, since it's pretty much everything you would expect to have work.
Comment 96 Jan Varga [:janv] 2011-07-06 13:00:47 PDT
Created attachment 544318 [details] [diff] [review]
Part2 - Core implementation

Here's the patch, I'll post more details tomorrow.
Comment 97 Jan Varga [:janv] 2011-07-08 02:43:26 PDT
Created attachment 544758 [details] [diff] [review]
Part 2 - Core implementation v2

Added comments to IDL interfaces and an automatic test.
Comment 98 Jan Varga [:janv] 2011-07-08 03:08:27 PDT
Ok, main differences are:
- general menu building code moved to nsHTMLMenuElement
- real building of content (XUL) is now done in a separate object that implements nsIMenuBuilder interface
- the interface can be used to build custom context menus (e.g. for Fennec) or to build toolbar menus
- nsHTMLMenuFrame will provide a builder for toolbar menus to build anonymous content which will be owned by the frame, a mutation observer will catch any DOM changes and will cause a reframe (I've already tested it a bit)
- only <command> can define a command, however the command API is not used to get the facets
- .contextMenu is readonly
- otherwise the implementation follows the spec
- all needed scripting is in a new JS module PageMenu.jsm so it can be easily reused in other XUL based apps
- once we switch to <browser remote="true">, the document fragment built by the native builder will be serialized and passed to the message manager, chrome will parse it into a document fragment again that will be added to the native context menu
- even XSLT can be used to transform XUL output from the native builder (I tested it)
Comment 99 Jan Varga [:janv] 2011-07-08 22:44:54 PDT
Comment on attachment 544758 [details] [diff] [review]
Part 2 - Core implementation v2

>+    } else if (tag == nsGkAtoms::hr || (tag == nsGkAtoms::option &&
>+               child->AttrValueIs(kNameSpaceID_None, nsGkAtoms::value,
>+                                  EmptyString(), eIgnoreCase) &&
>+               child->HasAttr(kNameSpaceID_None, nsGkAtoms::disabled) &&
>+               CountCharInReadable(optionText, PRUnichar('-')) ==
>+                                   optionText.Length())) {
>+      AddSeparator(aBuilder, aSeparator);
>+    }

I found a bug, |!optionText.IsEmpty() &&| is missing there, I'll fix it.
Comment 100 Hixie (not reading bugmail) 2011-07-11 14:27:09 PDT
I changed the command API to not overload the previously-defined members, which might help this a bit.
Comment 101 Jan Varga [:janv] 2011-07-11 21:43:37 PDT
ok, thanks
I'll file a new bug for the command API implementation.
Comment 102 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-07-12 04:58:20 PDT
Comment on attachment 544758 [details] [diff] [review]
Part 2 - Core implementation v2

>@@ -3273,17 +3299,21 @@ nsresult nsGenericHTMLElement::Click()
> 
>   // Click() is never called from native code, but it may be
>   // called from chrome JS. Mark this event trusted if Click()
>   // is called from chrome code.
>   nsMouseEvent event(nsContentUtils::IsCallerChrome(),
>                      NS_MOUSE_CLICK, nsnull, nsMouseEvent::eReal);
>   event.inputSource = nsIDOMNSMouseEvent::MOZ_SOURCE_UNKNOWN;
> 
>-  nsEventDispatcher::Dispatch(this, context, &event);
>+  nsEventStatus status = nsEventStatus_eIgnore;
>+  nsEventDispatcher::Dispatch(this, context, &event, nsnull, &status);
>+  if (status != nsEventStatus_eConsumeNoDefault) {
>+    PostHandleClick();
>+  }
Why is this change needed?
Couldn't you just do the post handle thing in
nsHTMLCommandElement::PostHandleEvent() ?

We should handle click() in a consistent manner.
Either it should have special powers in all the cases (which is doesn't atm), or
dispatching click should do the default handling, like it is done in all the current browsers.

>+      nsCOMPtr<nsIDOMHTMLCommandElement> command = do_QueryInterface(cur);
>+        
>+      nsAutoString rg;
>+      command->GetRadiogroup(rg);
>+      if (rg.Equals(radiogroup)) {
>+        PRBool checked;
>+        command->GetChecked(&checked);
>+        if (checked)
>+          command->SetChecked(PR_FALSE);
if (expr) {
  stmt;
}
>+nsHTMLMenuElement::BuildSubmenu(const nsAString& aLabel,
>+                                nsIContent* aContent,
>+                                nsIMenuBuilder* aBuilder)
>+{
>+  aBuilder->OpenMenu(aLabel);
>+
>+  PRInt8 separator = ST_TRUE_INIT;
>+  TraverseContent(aContent, aBuilder, separator);
>+
>+  if (separator == ST_TRUE)
>+    aBuilder->RemoveLastElement();
if (expr) {
  stmt;
}
Comment 103 Jan Varga [:janv] 2011-07-12 08:16:10 PDT
ok, I'll move it to PostHandleEvent()
Comment 104 :Ms2ger (⌚ UTC+1/+2) 2011-07-13 11:30:08 PDT
Comment on attachment 544758 [details] [diff] [review]
Part 2 - Core implementation v2

I see a couple of places where a FromContent method, like <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLSelectElement.h#254>, would be useful.

>--- a/content/html/content/src/nsGenericHTMLElement.cpp
>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>+nsGenericHTMLElement::GetContextMenu(nsIDOMHTMLMenuElement** aContextMenu)
>+{
>+    Element* element = doc->GetElementById(value);
>+
>+    if (element && element->IsHTML(nsGkAtoms::menu)) {
>+      CallQueryInterface(element, aContextMenu);

Then this could become

nsRefPtr<nsHTMLMenuElement> element = nsHTMLMenuElement::FromContent(doc->GetElementById(value));
element.forget(aContextMenu);

>--- /dev/null
>+++ b/content/html/content/src/nsHTMLCommandElement.cpp
>+nsHTMLCommandElement::ParseAttribute(PRInt32 aNamespaceID,
>+                                     nsIAtom* aAttribute,
>+                                     const nsAString& aValue,
>+                                     nsAttrValue& aResult)
>+{
>+  if (aNamespaceID == kNameSpaceID_None) {
>+    if (aAttribute == nsGkAtoms::type) {

if (aNamespaceID == kNameSpaceID_None && aAttribute == nsGkAtoms::type) {

>+nsHTMLCommandElement::PostHandleClick()
>+{
>+  if (mType == CMD_TYPE_CHECKBOX) {
>+    PRBool checked;
>+    GetChecked(&checked);
>+    SetChecked(!checked);

Please add a bool nsHTMLCommandElement::Checked() to make this SetChecked(!Checked());

>+  } else if (mType == CMD_TYPE_RADIO) {
>+    nsAutoString radiogroup;
>+    GetRadiogroup(radiogroup);
>+
>+    nsCOMPtr<nsIContent> parent = GetParent();

Is this always non-null?

>+
>+    nsCOMPtr<nsIContent> cur;
>+    for (cur = parent->GetFirstChild(); cur; cur = cur->GetNextSibling()) {

for (nsCOMPtr<nsIContent> cur = ...

>+      if (cur == this || !cur->IsHTML(nsGkAtoms::command)) {
>+        continue;
>+      }
>+
>+      nsCOMPtr<nsIDOMHTMLCommandElement> command = do_QueryInterface(cur);
>+        

Trailing spaces.

>+      nsAutoString rg;
>+      command->GetRadiogroup(rg);
>+      if (rg.Equals(radiogroup)) {
>+        PRBool checked;
>+        command->GetChecked(&checked);
>+        if (checked)
>+          command->SetChecked(PR_FALSE);
>+      }

With FromContent and Checked, this could be

if (Checked()) {
  command->SetChecked(PR_FALSE);
}

>--- /dev/null
>+++ b/content/html/content/src/nsHTMLMenuElement.cpp
>+PRBool
>+nsHTMLMenuElement::ParseAttribute(PRInt32 aNamespaceID,
>+                                  nsIAtom* aAttribute,
>+                                  const nsAString& aValue,
>+                                  nsAttrValue& aResult)
>+{
>+  if (aNamespaceID == kNameSpaceID_None) {
>+    if (aAttribute == nsGkAtoms::type) {

&&

>+      PRInt32 newType;
>+      PRBool success = aResult.ParseEnumValue(aValue, kMenuTypeTable,
>+                                              PR_FALSE);
>+      if (success) {
>+        newType = aResult.GetEnumValue();
>+      } else {
>+        newType = kMenuDefaultType->value;
>+      }
>+
>+      mType = newType;
>+      return success;

Is the newType local necessary?

>+void
>+nsHTMLMenuElement::TraverseContent(nsIContent* aContent,
>+                                   nsIMenuBuilder* aBuilder,
>+                                   PRInt8& aSeparator)
>+{
>+  nsCOMPtr<nsIContent> child;
>+  for (child = aContent->GetFirstChild(); child;
>+       child = child->GetNextSibling()) {
>+    if (!child->IsHTML()) {
>+      continue;
>+    }
>+
>+    nsIAtom* tag = child->Tag();
>+
>+    PRBool childHasLabel = child->HasAttr(kNameSpaceID_None, nsGkAtoms::label);
>+
>+    nsAutoString optionText;
>+    if (tag == nsGkAtoms::option) {
>+      nsContentUtils::GetNodeTextContent(child, PR_TRUE, optionText);
>+    }

I'd move these two into the if-block below.

>+
>+    nsCOMPtr<nsIDOMHTMLCommandElement> command = do_QueryInterface(child);
>+    if (command) {

if (tag == command) {
FromContent(child)
}

would save you two QIs

>+      nsCOMPtr<nsIDOMNSHTMLElement> element = do_QueryInterface(child);
>+
>+      PRBool hidden;
>+      element->GetHidden(&hidden);

And you could add nsGenericHTMLElement::Hidden().

>+    } else if (tag == nsGkAtoms::hr || (tag == nsGkAtoms::option &&
>+    } else if (tag == nsGkAtoms::li || tag == nsGkAtoms::label) {
>+    } else if ((tag == nsGkAtoms::menu && !childHasLabel) ||
>+                tag == nsGkAtoms::select) {
>+    } else if ((tag == nsGkAtoms::optgroup || tag == nsGkAtoms::menu) &&
>+    }

Would a switch work here?

>--- a/content/xul/content/Makefile.in
>+++ b/content/xul/content/Makefile.in
>-PARALLEL_DIRS	= src
>+PARALLEL_DIRS	= public src

Is that necessary? I think we already export headers from src/ dirs.

>--- /dev/null
>+++ b/content/xul/content/src/nsXULContextMenuBuilder.cpp
>+nsXULContextMenuBuilder::AddItemFor(nsIDOMHTMLCommandElement* aElement)
>+    PRBool checked;
>+    aElement->GetChecked(&checked);
>+    if (checked) {

Checked()

>+  PRBool disabled;
>+  aElement->GetDisabled(&disabled);
>+  if (disabled) {

and Disabled()

>+NS_IMETHODIMP
>+nsXULContextMenuBuilder::Click(const nsAString& aIdent)
>+{
>+  PRInt32 rv;
>+  PRInt32 idx = nsString(aIdent).ToInteger(&rv);

You don't check rv here.

>+  if (element)
>+    element->Click();

{}

>+nsXULContextMenuBuilder::CreateElement(nsIDocument* aDocument,
>+                                       nsIAtom* aTag, nsIContent** aResult)

aDocument is always mDocument, as far as I can tell, so I'd just use that.

>+  if (NS_FAILED(rv))
>+    return rv;

{}

>--- /dev/null
>+++ b/toolkit/content/PageMenu.jsm
>+  initPageMenu: function(aTarget, aPopup) {
>+      if (!insertionPoint)
>+        return null;

{}

>+      if (!builder)
>+        return null;

{}

>+  getImmediateChild: function(element, tag) {
>+      if (child.localName == tag)
>+        return child;

{}

>+  getInsertionPoint: function(aPopup) {
>+    if (aPopup.hasAttribute(this.PAGEMENU_ATTR))
>+      return aPopup;

{}

>+          if (result)
>+            return result;

{}
Comment 105 Jan Varga [:janv] 2011-07-13 12:58:11 PDT
(In reply to comment #104)
> >+
> >+    nsAutoString optionText;
> >+    if (tag == nsGkAtoms::option) {
> >+      nsContentUtils::GetNodeTextContent(child, PR_TRUE, optionText);
> >+    }
> 
> I'd move these two into the if-block below.

childHasLabel is checked in two if-blocks

if I move optionText to the if-block then I have to add a new |if| there
so it doesn't matter IMO


> 
> 
> >+    } else if (tag == nsGkAtoms::hr || (tag == nsGkAtoms::option &&
> >+    } else if (tag == nsGkAtoms::li || tag == nsGkAtoms::label) {
> >+    } else if ((tag == nsGkAtoms::menu && !childHasLabel) ||
> >+                tag == nsGkAtoms::select) {
> >+    } else if ((tag == nsGkAtoms::optgroup || tag == nsGkAtoms::menu) &&
> >+    }
> 
> Would a switch work here?

a switch ? a switch based on the tag ?
it would look much more complicated, now it looks quite simple and it can be easily checked if it matches the spec

I'd like to see a switch that would look better or faster here.

> 
> >--- a/content/xul/content/Makefile.in
> >+++ b/content/xul/content/Makefile.in
> >-PARALLEL_DIRS	= src
> >+PARALLEL_DIRS	= public src
> 
> Is that necessary? I think we already export headers from src/ dirs.
> 

haven't you noticed the new IDL file ?
Comment 106 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-07-15 03:43:47 PDT
Comment on attachment 544758 [details] [diff] [review]
Part 2 - Core implementation v2

>+/**
>+ * A private interface.
>+ * All methods throw NS_ERROR_DOM_SECURITY_ERR if the caller is not chrome.
>+ */
>+
>+[scriptable, uuid(d3d068d8-e223-4228-ba39-4d6df21ba616)]
>+interface nsIHTMLMenu : nsISupports
>+{
>+  /**
>+   * Creates and dispatches a trusted event named "show".
>+   * The event is not cancelable and does not bubble.
>+   * See http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#context-menus
>+   */
>+  void sendShowEvent();
>+
>+  /**
>+   * Creates a native menu builder. The builder type is dependent on menu type.
>+   * Currently, it returns nsXULContextMenuBuilder for context menus.
>+   * Toolbar menus are not yet supported (the method returns null).
>+   */
>+  nsIMenuBuilder createBuilder();
>+
>+  /*
>+   * Builds a menu by iterating over menu children.
>+   * See http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#building-menus-and-toolbars
>+   * The caller can use a native builder by calling createBuilder() or provide
>+   * a custom builder that implements the nsIMenuBuilder interface.
>+   * A custom builder can be used for example to build native context menus
>+   * that are not defined using <menupopup>.
>+   */
>+  void build(in nsIMenuBuilder aBuilder);
>+
>+};


>+[scriptable, uuid(12724737-f7db-43b4-94ab-708a7b86e115)]
>+interface nsIMenuBuilder : nsISupports
>+{
>+
>+  /**
>+   * Create the top level menu or a submenu. The implementation should create
>+   * a new context for this menu, so all subsequent methods will add new items
>+   * to this newly created menu.
>+   */
>+  void openMenu(in DOMString aLabel);
...
>+  /**
>+   * Set the context to the parent menu.
>+   */
>+  void closeMenu();


openMenu/closeMenu sound like something which would open/close the menu.
Apparently the methods are only about building the menu.
Perhas startBuilding(), finishBuilding() ?

This interface needs some documentation.
It is not very clear what init does.
>+[scriptable, uuid(f0c35053-14cc-4e23-a9db-f9a68fae8375)]
>+interface nsIXULContextMenuBuilder : nsISupports
>+{
>+
>+  void init(in nsIDOMDocumentFragment aDocumentFragment,
>+            in AString aGeneratedAttrName,
>+            in AString aIdentAttrName);
>+
>+  void click(in DOMString aIdent);
>+
>+};


I'd like to see still an updated patch.
Comment 107 Jan Varga [:janv] 2011-07-15 12:51:42 PDT
Created attachment 546209 [details] [diff] [review]
Part 2 - Core implementation v3
Comment 108 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-07-18 07:27:21 PDT
Comment on attachment 546209 [details] [diff] [review]
Part 2 - Core implementation v3


>+nsresult
>+nsHTMLCommandElement::PostHandleEvent(nsEventChainPostVisitor& aVisitor)
>+{
>+  if (aVisitor.mEventStatus == nsEventStatus_eConsumeNoDefault ||
>+      aVisitor.mEvent->message != NS_MOUSE_CLICK) {
>+    return NS_OK;
>+  }
>+
>+  if (mType == CMD_TYPE_CHECKBOX) {
>+    SetChecked(!IsChecked());
>+  } else if (mType == CMD_TYPE_RADIO) {
>+    nsAutoString radiogroup;
>+    GetRadiogroup(radiogroup);
>+
>+    nsCOMPtr<nsIContent> parent = GetParent();
>+    if (!parent) {
>+      return NS_OK;
>+    }
>+
>+    for (nsCOMPtr<nsIContent> cur = parent->GetFirstChild();
>+         cur;
>+         cur = cur->GetNextSibling()) {
>+      if (cur == this) {
>+        continue;
>+      }
>+
>+      nsRefPtr<nsHTMLCommandElement> command =
>+         nsHTMLCommandElement::FromContent(cur);
>+
>+      if (!command) {
>+        continue;
>+      }
>+
>+      nsAutoString rg;
>+      command->GetRadiogroup(rg);
>+      if (rg.Equals(radiogroup) && command->IsChecked()) {
>+        command->SetChecked(PR_FALSE);
>+      }
>+    }
>+
>+    SetChecked(PR_TRUE);
>+  }
>+
>+  return NS_OK;
>+}
Hmm, looks like the spec requires the checked state to be changed before
dispatching the event. Am I right?
So this all should go to PreHandleEvent.
Sorry, my mistake.

That behavior needs tests.
Comment 109 Jan Varga [:janv] 2011-07-18 09:09:07 PDT
Hixie, it seems there's a bug in the spec or I just don't understand it correctly:

click() for <command> should "run synthetic click activation steps" if it is in the radio or checkbox state
"run synthetic click activation steps" is defined as:
- run pre-click activation steps
- fire a click event
- run post-click activation steps (if the event was not cancelled), which is to run "activation behavior"
activation behavior for <command> in the checked state is defined as:
"the UA must add a checked attribute, with the literal value checked. The UA must then fire a click event at the element."

does it mean that two "click" events are dispatched ?
Comment 110 Jan Varga [:janv] 2011-07-18 10:11:28 PDT
Comment on attachment 546209 [details] [diff] [review]
Part 2 - Core implementation v3

Neil, could you review the toolkit part of the patch ?
Comment 111 Jan Varga [:janv] 2011-07-18 10:29:27 PDT
Comment on attachment 546209 [details] [diff] [review]
Part 2 - Core implementation v3

Dão, could you review the /browser part ?
Comment 112 Hixie (not reading bugmail) 2011-07-18 11:26:57 PDT
re comment 109: yeah, that's a bug. it's supposed to set checked in the pre-click activation steps, and the activation behaviour should be nothing. I'll add this to my pile of feedback to fix.
Comment 113 Jan Varga [:janv] 2011-07-18 11:42:57 PDT
Ok, so the behavior is actually very similar to <input type="checkbox|radio">
that's what Olli and I concluded.

I'll rework the impl that way.
Comment 114 Dão Gottwald [:dao] 2011-07-19 05:21:44 PDT
Comment on attachment 546209 [details] [diff] [review]
Part 2 - Core implementation v3

Please correct me if I'm wrong, but it looks like PageMenu.jsm isn't multi-process ready, as it expects a target node from content. The big context menu rewrite for e10s will need to happen soon. Can the new code take this into account rather than adding to the pile of stuff that will have to be rewritten?
Comment 115 Jan Varga [:janv] 2011-07-19 05:51:54 PDT
Actually, it is multi-process ready. It will be really easy to adjust it for multi-process architecture.

Here's the plan:
- <browser> will have to listen for the contextmenu event instead of using the contextmenu attribute that points to a <menupopup>
- PageMenu.jsm will still produce a document fragment with the XUL, but it will serialize it and pass to the message manager instead of appending to <menupopup>
- chrome process will handle the fragment by appending it to <menupopup>
- another message will be used to invoke command action (chrome -> content)

Believe me, it was designed to work in multi-process architecture.
However, the current PageMenu.js can't be used with <browser remote="true"> immediately of course. I'll adjust it once we move closer to multi-process. For now, it will be better to have clean initial implementation.
Comment 116 Dão Gottwald [:dao] 2011-07-19 11:11:54 PDT
Comment on attachment 546209 [details] [diff] [review]
Part 2 - Core implementation v3

>   initItems: function CM_initItems() {
>+    this.initSeparator();
>     this.initOpenItems();
>     this.initNavigationItems();
>     this.initViewItems();
>     this.initMiscItems();
>     this.initSpellingItems();
>     this.initSaveItems();
>     this.initClipboardItems();
>     this.initMediaPlayerItems();
>   },
> 
>+  initSeparator: function CM_initSeparator() {
>+    this.showItem("page-menu-separator", this.pageMenu ? true : false);
>+  },

s/initSeparator/initPageMenuSeparator/ ?

What prevents the separator from being displayed when the page doesn't provide a menu?

Did you intentionally leave out web-panels.xul?
Comment 117 Jan Varga [:janv] 2011-07-20 06:47:59 PDT
Created attachment 547069 [details] [diff] [review]
Part 2 - Core implementation v4

- changed activation behavior of <command>
- added an automatic test for it
- renamed initSeparator() to initPageMenuSeparator()
- renamed initPageMenu() to init() and changed it to return true/false
- updated web-panels.xul to support the HTML5 context menu feature too
- if (pageMenu) { ... } -> if (!pageMenu) { return false } in PageMenu.jsm
Comment 118 Jan Varga [:janv] 2011-07-20 12:46:48 PDT
We're going to change the behavior of the <command checked=""> attribute to be consistent with <input>. So it will be used only for setting of the initial state.
Comment 119 Dão Gottwald [:dao] 2011-07-20 14:20:51 PDT
Comment on attachment 547069 [details] [diff] [review]
Part 2 - Core implementation v4

>+      <menuseparator id="page-menu-separator"/>

Shouldn't this be in browser-context.inc?

>-  this.initMenu(aBrowser);
>+  this.initMenu(aXulMenu, aBrowser, aIsShift);
> }
> 
> // Prototype for nsContextMenu "class."
> nsContextMenu.prototype = {
>-  initMenu: function CM_initMenu(aBrowser) {
>+  initMenu: function CM_initMenu(aXulMenu, aBrowser, aIsShift) {

I'd prefer if you didn't change the argument order.

otherwise r=me on the browser part
Comment 120 Jan Varga [:janv] 2011-07-20 21:25:59 PDT
Comment on attachment 547069 [details] [diff] [review]
Part 2 - Core implementation v4

Neil, could you review the toolkit part of the patch ?
Comment 121 Jan Varga [:janv] 2011-07-25 04:11:37 PDT
Created attachment 548145 [details] [diff] [review]
Part 2 - Core implementation v5
Comment 122 Jan Varga [:janv] 2011-07-25 05:59:42 PDT
Created attachment 548159 [details] [diff] [review]
Part 2 - Core implementation v6
Comment 123 Jan Varga [:janv] 2011-07-25 07:16:30 PDT
Created attachment 548177 [details] [diff] [review]
interdiff v4 - v5
Comment 124 Jan Varga [:janv] 2011-07-25 07:17:40 PDT
Created attachment 548178 [details] [diff] [review]
interdiff v5 - v6
Comment 125 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-07-27 06:42:02 PDT
Created attachment 548762 [details]
comments
Comment 126 Jan Varga [:janv] 2011-07-27 08:44:00 PDT
Created attachment 548799 [details] [diff] [review]
Part 2 - Core implementation v7
Comment 127 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-07-27 08:59:08 PDT
Comment on attachment 548799 [details] [diff] [review]
Part 2 - Core implementation v7

Please don't forget to file spec bugs.
Comment 128 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-07-27 09:00:45 PDT
One thing still; what happens if  the icon points to some huge image.
Comment 129 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-07-27 09:15:46 PDT
Also, icon url handling in chrome UI should have similar limitations
what favicon has, I think.
Comment 130 Jan Varga [:janv] 2011-07-27 09:45:06 PDT
(In reply to comment #128)
> One thing still; what happens if  the icon points to some huge image.

it doesn't cause any problems AFAIK, the image is scaled down
the browser is a bit slow when the image is loading
Comment 131 Jan Varga [:janv] 2011-07-27 12:00:07 PDT
(In reply to comment #129)
> Also, icon url handling in chrome UI should have similar limitations
> what favicon has, I think.

I added a nsContentUtils::CanLoadImage() check
Olli says it should be enough for now.

I'll attach the final patch soon.
Comment 132 Jan Varga [:janv] 2011-07-27 12:56:21 PDT
Created attachment 548875 [details] [diff] [review]
Part 2 - Core implementation v8
Comment 133 Jan Varga [:janv] 2011-07-27 23:00:00 PDT
(In reply to comment #127)
> Comment on attachment 548799 [details] [diff] [review] [review]
> Part 2 - Core implementation v7
> 
> Please don't forget to file spec bugs.

http://www.w3.org/Bugs/Public/show_bug.cgi?id=13401
https://bugzilla.mozilla.org/show_bug.cgi?id=674822
Comment 134 Jan Varga [:janv] 2011-08-08 06:08:06 PDT
Created attachment 551436 [details] [diff] [review]
Part 2 - Core implementation v9

<command> -> <menuitem>
Comment 135 Jan Varga [:janv] 2011-08-08 06:09:06 PDT
Created attachment 551437 [details] [diff] [review]
Changes between v8 and v9
Comment 136 Jan Varga [:janv] 2011-08-08 13:08:07 PDT
http://hg.mozilla.org/mozilla-central/rev/561821863607
Comment 137 Dão Gottwald [:dao] 2011-08-08 13:12:58 PDT
(In reply to Jan Varga from comment #136)
> http://hg.mozilla.org/mozilla-central/rev/561821863607

PageMenu.jsm is unreviewed, isn't it? Why did this land?
Comment 138 Neil Deakin 2011-08-09 12:04:00 PDT
Comment on attachment 551436 [details] [diff] [review]
Part 2 - Core implementation v9

>+PageMenu.prototype = {
>+  PAGEMENU_ATTR: "pagemenu",
>+  GENERATED_ATTR: "generated",
>+  IDENT_ATTR: "ident",
>+

'ident' seems a little too non-descript and generic. Can you change it to pagemenuid or something. You could also just combine 'generated' and the ident attribute and use only one attribute, no?


>+  init: function(aTarget, aPopup) {

The code here implies that this is supposed to be called during popupshowing when the context menu is opened. It would be clearer to name the function better, as 'init' to me implies that it should only be called once.


>+    if (pos == "end") {
>+      insertionPoint.appendChild(fragment);
>+    } else {
>+      insertionPoint.insertBefore(fragment,
>+                                  insertionPoint.firstChild);
>+    }

I would think adding at the end would be a more logical default.
Comment 139 Jan Varga [:janv] 2011-08-10 00:42:49 PDT
ok, thanks
I'll fix it ASAP (before the m-c to aurora merge)
Comment 140 Jan Varga [:janv] 2011-08-15 06:06:11 PDT
Created attachment 553157 [details] [diff] [review]
A followup fix

addressed Neil's comments
Comment 141 Christopher Blizzard (:blizzard) 2011-08-16 21:51:53 PDT
Did that additional fix get merged to central before the aurora cut today?
Comment 142 Christopher Blizzard (:blizzard) 2011-08-16 21:56:34 PDT
Adding ? for tracking-8 since this might have some unfinished fixes.
Comment 143 Jan Varga [:janv] 2011-08-16 21:57:53 PDT
(In reply to Christopher Blizzard (:blizzard) from comment #141)
> Did that additional fix get merged to central before the aurora cut today?

no, it didn't
Comment 144 Christopher Blizzard (:blizzard) 2011-08-18 22:18:25 PDT
OK, I guess we should disable it for Firefox Update 8 then.
Comment 145 Jan Varga [:janv] 2011-08-18 22:37:25 PDT
no, the last patch is just internal cleanup in the implementation
Comment 146 Jan Varga [:janv] 2011-08-18 22:39:25 PDT
the last patch landed on mozilla-central (mozilla9)
http://hg.mozilla.org/mozilla-central/rev/cbb901789b3b
Comment 147 Jan Varga [:janv] 2011-08-18 22:50:41 PDT
Created attachment 554309 [details]
updated demo

I updated the demo at http://people.mozilla.com/~prouget/bugs/context-menu-test/test.html

I just needed to replace <command> with <menuitem>
Comment 148 Michael[tm] Smith 2011-08-18 23:19:54 PDT
(In reply to Jan Varga from comment #147)
> Created attachment 554309 [details]
> I updated the demo at
> http://people.mozilla.com/~prouget/bugs/context-menu-test/test.html
> 
> I just needed to replace <command> with <menuitem>

I notice you also added </menuitem> end tags. Is that just in order to get the demo to work for now, until the parser can be made aware of the new element? To be clear, what I mean is, like the command element, the menuitem element is meant to be a void element, right?
Comment 149 Jan Varga [:janv] 2011-08-18 23:29:14 PDT
(In reply to Michael(tm) Smith from comment #148)
> (In reply to Jan Varga from comment #147)
> > Created attachment 554309 [details]
> > I updated the demo at
> > http://people.mozilla.com/~prouget/bugs/context-menu-test/test.html
> > 
> > I just needed to replace <command> with <menuitem>
> 
> I notice you also added </menuitem> end tags. Is that just in order to get
> the demo to work for now, until the parser can be made aware of the new
> element? To be clear, what I mean is, like the command element, the menuitem
> element is meant to be a void element, right?

Well, we count with both possibilities ...
there's W3C bug http://www.w3.org/Bugs/Public/show_bug.cgi?id=13608 and mozilla bug 676236 to make <menuitem> a void element, the patch landed but the "voidness" is disabled for now (until the W3C bug gets resolved).

There's also mozilla bug 677463 to use element's textContent if there's no label attribute, the patch for this bug landed too.
Comment 150 Tobias 2011-08-20 02:14:07 PDT
(In reply to Paul Rouget [:paul] from comment #1)
 
> My main concern is security. How can we make it clear that these menu-items
> are not from Firefox?

What about setting the web page favicon left from the action? If the favicon is missing the blank document icon could be used as fallback.
Comment 151 Jan Varga [:janv] 2011-08-20 02:25:07 PDT
(In reply to Tobias from comment #150)
> (In reply to Paul Rouget [:paul] from comment #1)
>  
> > My main concern is security. How can we make it clear that these menu-items
> > are not from Firefox?
> 
> What about setting the web page favicon left from the action? If the favicon
> is missing the blank document icon could be used as fallback.

not a bad idea ... however each menu item can have own icon, so the favicon would be used as a fallback
Comment 152 Christian Heilmann 2011-09-11 10:18:44 PDT
According to the spec <menuitem> should be <command> - can we support both? 

http://www.w3.org/TR/html5/interactive-elements.html#context-menus
Comment 153 Hixie (not reading bugmail) 2011-09-11 11:03:08 PDT
As discussed in http://www.w3.org/Bugs/Public/show_bug.cgi?id=13608 , I really see no value in adding the <menuitem> element. It doesn't seem to add any new features, doesn't solve any problems, and doesn't make anything simpler.
Comment 154 Kohei Yoshino [:kohei] 2011-09-12 21:28:33 PDT
XUL has the menuitem element and WAI-ARIA has the menuitem role. I hope HTML5 would go the same way.
https://developer.mozilla.org/en/XUL/menuitem
http://www.w3.org/TR/wai-aria/complete#menuitem
Comment 155 Hixie (not reading bugmail) 2011-09-29 15:13:36 PDT
Based on the discussion in http://www.w3.org/Bugs/Public/show_bug.cgi?id=13608, I think we should remove the <menuitem> part of this.
Comment 156 Alex b1 2011-09-30 01:53:22 PDT
As comment about
https://bug617528.bugzilla.mozilla.org/attachment.cgi?id=554309

Will be better use html5 charset notation
<meta charset="UTF-8">
and include stylesheet with
<link href="style.css" rel="stylesheet">
Comment 157 Jan Varga [:janv] 2011-09-30 04:08:00 PDT
that is just a simple example for testing/demonstration of the feature
Comment 158 Sean Newman 2011-09-30 04:24:40 PDT
I see 9 buttons instead of 3 on the test page.
I'm using latest Nightly.
Comment 159 Alex b1 2011-09-30 05:05:48 PDT
(In reply to Jan Varga [:janv] from comment #157)
> that is just a simple example for testing/demonstration of the feature

I know but for newbies will be better write examples with best practice inside

 (In reply to Sean Newman from comment #158)
> I see 9 buttons instead of 3 on the test page.
> I'm using latest Nightly.

It's ok
http://i56.tinypic.com/4ruio0.png
Comment 160 Jan Varga [:janv] 2011-09-30 05:47:03 PDT
it works ok in my latest trunk build
Comment 161 Jan Varga [:janv] 2011-09-30 06:23:24 PDT
some early feedback:
https://plus.google.com/115133653231679625609/posts/CJMyExJTbug
Comment 162 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2011-10-09 08:32:35 PDT
About e10s:

> - PageMenu.jsm will still produce a document fragment with the XUL, but it will
> serialize it and pass to the message manager instead of appending to <menupopup>

[So, I'm working on porting Fx's contextmenu to e10s.]

I don't get how this could work, for a couple of reasons:
1.  AFAICT, you cannot create a xul fragment within a html document (remote xul is dead).
2.  Within the chrome process, the serialized fragment (toXMLString()) needs to be converted to back to a dom tree. The api for this is, afaik, async. However, context menu should be opened synchronously.

Please reply as soon as you can.
Comment 163 Jan Varga [:janv] 2011-10-09 12:59:57 PDT
I assume you're going to use the same approach as we do in Fennec (the content process has a listener for the context menu event which collects all necessary data and sends it to the chrome process via the message manager).

I'll try to describe how it is done currently:
- custom context menu items are built by calling PageMenu.maybeBuildAndAttachMenu()
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#83
- maybeBuildAndAttachMenu() builds and attaches custom context menu items
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/PageMenu.jsm#48

So the function maybeBuildAndAttachMenu() currently touches a html document (content) and also a xul document (chrome).

If you want to port it to e10s, you have to split the function.
Building of the xul fragment should be done in the content process (in the contextmenu listener). The fragment can be then serialized:

var serializer = new XMLSerializer();
var xml = serializer.serializeToString(fragment);

and attached to the message for the chrome process.

The chrome process will create a fragment again by:
var parser = new DOMParser();
var fragment = parser.parseFromString(xml, "text/xml");

at this point, the fragment can be finally attached to the context menu

However, I'm afraid parseFromString() can only produce dom documents (not fragments), so you will have to find a workaround or we can add support to the DOM parser to produce document fragments as we do in XSLTProcessor.

When a custom context menu item is selected, the command event is fired. The listener in the chrome process should get generated id of the item:
var id = target.getAttribute("generateditemid");
and then send it to the content process.

The content process will then call:
builder.click(id);
Comment 164 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2011-10-09 13:34:35 PDT
Are you sure xul elements can be created within a html document? I'm not sure in what way remote-xul has been disabled.
Comment 165 Jan Varga [:janv] 2011-10-09 13:53:11 PDT
remote-xul has nothing to do with it AFAIK, remote-xul is e.g. xul documents placed on a webserver, "remote" in e10s means something different

xul in html is not a problem, anyway these xul elements are not even created within a html document. These xul elements are kept in a document fragment and they are created by walking <menu> descendants and they are not attached to any html document

it's something like a transformation ...
html <menu> is transformed into xul <menupopup>

xul elements are created in native (C++) code
Comment 166 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2011-10-18 11:29:16 PDT
per note from imelevin marking this for sec-review-needed, not sure this needs a sec review for sure so I will triage this with secteam
Comment 167 Ian Melven :imelven 2011-11-22 16:46:11 PST
i took another pass through this - my concerns are addressed (and the security implications of this have been well discussed above in the bug), mostly i wanted to see if the 'get to real context menu via shift-right click' was implemented and it was. removing sec-review-needed.
Comment 169 steve faulkner 2012-02-08 02:51:33 PST
How is the context menu opened using the keyboard?
it appears that if image in the context menu image example in http://thewebrocks.com/demos/context-menu/ is given a tabindex=0 the image can be focused using the keyboard, but using the ALt+F10 command for opening a context menu reulsts in the standard menu being displayed not the custom menu.
Likewise if trying the second demo, if text is slected and alt+f10 is pressed the standard menu appears (not next to the selected text, but in the top left corcer of the viewport - which i think is a more general UI bug)
Comment 170 steve faulkner 2012-02-08 02:58:36 PST
(In reply to steve faulkner from comment #169)
> How is the context menu opened using the keyboard?
> it appears that if image in the context menu image example in
> http://thewebrocks.com/demos/context-menu/ is given a tabindex=0 the image
> can be focused using the keyboard, but using the ALt+F10 command for opening
> a context menu reulsts in the standard menu being displayed not the custom
> menu.
> Likewise if trying the second demo, if text is slected and alt+f10 is
> pressed the standard menu appears (not next to the selected text, but in the
> top left corcer of the viewport - which i think is a more general UI bug)

apologies standard command on windows is SHIFT+F10 not alt+F10
Comment 171 alexander :surkov 2012-02-08 02:59:56 PST
Should I file new bug for Steve's issue (comment #169) or is it supposed to be fixed here? What is the status of the bug?
Comment 172 Jan Varga [:janv] 2012-02-08 04:32:02 PST
please file a new bug
Comment 173 alexander :surkov 2012-02-08 04:50:58 PST
(In reply to Jan Varga [:janv] from comment #172)
> please file a new bug

it sounds there's one already: bug 699709
Comment 174 Timothy Nikkel (:tnikkel) 2012-02-08 23:15:27 PST
(In reply to steve faulkner from comment #169)
> (not next to the selected text, but in the
> top left corcer of the viewport - which i think is a more general UI bug)

Can you make sure we have a bug on file for that issue please?
Comment 175 Brett Zamir 2012-09-24 03:29:48 PDT
Was the "title" attribute excluded in the above discussion from being allowable as a tooltip when present on contextmenu menuitems? Would be quite handy...
Comment 176 Florian Bender 2013-10-26 07:22:13 PDT
Bug 704428 and Bug 703998 should be added to the dependancy list of this bug. 

There is some ongoing discussion about the default/"native" context menu items on the W3C bug tracker[1] which should be monitored as well (maybe add as "see also"?). 


[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=12999
Comment 177 Florian Bender 2013-10-26 13:35:50 PDT
The spec also changed since this was implemented, the work is covered by Bug 897102.
Comment 178 jaramat 2014-02-23 13:55:03 PST
Some people don't like this HTML5 context menu feature, for example a recent post on Reddit:
http://www.reddit.com/r/firefox/comments/1ypvyh/how_do_i_prevent_sites_from_modifying_my_context/

I tried to look into it with DOM Inspector and found out I cannot do anything about it since these menu entries appear only if the context menu is displayed, and they don't even have any unique ID or class. I propose an edit to this feature, so every HTML5 menu would have a class, for example ".pagemenu".

This would allow people to simply hide these custom menu items with nothing more than menuitem.pagemenu {display: none;}.

Is anyone here going to look into this or should I fill a separate bug for this?
Comment 179 Florian Bender 2014-02-24 05:37:40 PST
Definitely a separate bug. Though I suspect you really want a pref to prevent HTML5 context menu from adding items to the context menus (the <menu> feature is useful for more things than simply adding items to the context menu!) instead of any magic DOM fiddling …

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