617528 - implement the HTML5 "context menu" feature (contextmenu attribute)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Michael[tm] Smith [:sideshowbarker]]

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]]

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]

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]

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)]

Paul, is that implemented by any UA?

Comment 5

[Antoine Turmel [:GeekShadow]]

(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]]

(In reply to comment #4)
> Paul, is that implemented by any UA?

No.

Comment 7

[Paul Rouget [:paul]]

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)]

(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]]

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)]

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]]

(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)]

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]]

(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)]

Jan, could you explain how bug 556743 blocks this bug?

Comment 15

[Jan Varga [:janv]]

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)]

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)]

(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]]

Ok, I'll implement a helper to get the labels.

Comment 19

[Jan Varga [:janv]]

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]]

Attachment: 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]]

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][[e-mail]]]

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]]

Attachment: patch for event dispatcher v2

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

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]]

(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]]

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][[e-mail]]]

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]]

(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]]

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]]

utils.dispatchDOMEventViaPresShell() works ok, but then I can't test the retargeting ...

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

(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]]

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]]

Attachment: test for event dispatcher

Comment 34

[Jan Varga [:janv]]

Attachment: patch v1

Comment 35

[Jan Varga [:janv]]

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 (he/him; ⌚ UTC+1/+2)]

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]]

(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)]

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]]

(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]]

Attachment: browser/ part

Comment 41

[Jan Varga [:janv]]

(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 (he/him; ⌚ UTC+1/+2)]

Should be -x-moz-nodefault (or _moz-nodefault), and please send email to whatwg or file a bug.

Comment 43

[Ole Dormann]

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]]

Attachment: patch v2

Comment 45

[Jan Varga [:janv]]

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][[e-mail]]]

(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]]

who should do a ux review ?
can you cc him/her ?

Comment 48

[Ole Dormann]

(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]

(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][[e-mail]]]

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 (he/him; ⌚ UTC+1/+2)]

(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]]

(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]]

(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]]

(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]

(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]]

Attachment: patch for event dispatcher v3 (ready to be pushed)

Comment 57

[Neil Deakin]

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]]

(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]

> 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]]

(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)]

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]]

(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]]

Attachment: Part 1 - Dispatch shift right click (contextmenu event) only to chrome (ready to be pushed)

Comment 64

[Jan Varga [:janv]]

http://hg.mozilla.org/mozilla-central/rev/6d2cd42969f9

Comment 65

[Jan Varga [:janv]]

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]]

ok, it just means the attribute can return null

Comment 67

[Michael[tm] Smith [:sideshowbarker]]

(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]]

filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=12999
for the nodefault attribute

Comment 69

[Jan Varga [:janv]]

Attachment: Part 2 - Add support for HTML5 Commands

Comment 70

[Jan Varga [:janv]]

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)]

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)]

Note that accessKeyLabel is being implemented in bug 583533.

Comment 73

[Jan Varga [:janv]]

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)]

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]]

(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]]

(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 (he/him; ⌚ UTC+1/+2)]

(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][[e-mail]]]

(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]]

(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]]

(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)]

(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]]

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)]

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]]

(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]]

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]]

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)]

No, HTMLOptionElement.label should always return its label.

Comment 88

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

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]]

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]]

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)]

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]]

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][[e-mail]]]

(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)]

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]]

Attachment: Part2 - Core implementation

Here's the patch, I'll post more details tomorrow.

Comment 97

[Jan Varga [:janv]]

Attachment: Part 2 - Core implementation v2

Added comments to IDL interfaces and an automatic test.

Comment 98

[Jan Varga [:janv]]

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]]

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)]

I changed the command API to not overload the previously-defined members, which might help this a bit.

Comment 101

[Jan Varga [:janv]]

ok, thanks
I'll file a new bug for the command API implementation.

Comment 102

[Olli Pettay [:smaug][[e-mail]]]

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]]

ok, I'll move it to PostHandleEvent()

Comment 104

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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]]

(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][[e-mail]]]

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]]

Attachment: Part 2 - Core implementation v3

Comment 108

[Olli Pettay [:smaug][[e-mail]]]

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]]

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]]

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]]

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)]

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]]

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]]

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]]

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]]

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]]

Attachment: 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]]

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]]

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]]

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]]

Attachment: Part 2 - Core implementation v5

Comment 122

[Jan Varga [:janv]]

Attachment: Part 2 - Core implementation v6

Comment 123

[Jan Varga [:janv]]

Attachment: interdiff v4 - v5

Comment 124

[Jan Varga [:janv]]

Attachment: interdiff v5 - v6

Comment 125

[Olli Pettay [:smaug][[e-mail]]]

Attachment: comments

Comment 126

[Jan Varga [:janv]]

Attachment: Part 2 - Core implementation v7

Comment 127

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

One thing still; what happens if  the icon points to some huge image.

Comment 129

[Olli Pettay [:smaug][[e-mail]]]

Also, icon url handling in chrome UI should have similar limitations
what favicon has, I think.

Comment 130

[Jan Varga [:janv]]

(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]]

(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]]

Attachment: Part 2 - Core implementation v8

Comment 133

[Jan Varga [:janv]]

(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]]

Attachment: Part 2 - Core implementation v9

<command> -> <menuitem>

Comment 135

[Jan Varga [:janv]]

Attachment: Changes between v8 and v9

Comment 136

[Jan Varga [:janv]]

http://hg.mozilla.org/mozilla-central/rev/561821863607

Comment 137

[Dão Gottwald [:dao]]

(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]

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]]

ok, thanks
I'll fix it ASAP (before the m-c to aurora merge)

Comment 140

[Jan Varga [:janv]]

Attachment: A followup fix

addressed Neil's comments

Comment 141

[Christopher Blizzard (:blizzard)]

Did that additional fix get merged to central before the aurora cut today?

Comment 142

[Christopher Blizzard (:blizzard)]

Adding ? for tracking-8 since this might have some unfinished fixes.

Comment 143

[Jan Varga [:janv]]

(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)]

OK, I guess we should disable it for Firefox Update 8 then.

Comment 145

[Jan Varga [:janv]]

no, the last patch is just internal cleanup in the implementation

Comment 146

[Jan Varga [:janv]]

the last patch landed on mozilla-central (mozilla9)
http://hg.mozilla.org/mozilla-central/rev/cbb901789b3b

Comment 147

[Jan Varga [:janv]]

Attachment: 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 [:sideshowbarker]]

(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]]

(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]

(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]]

(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]

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)]

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]

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)]

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]

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]]

that is just a simple example for testing/demonstration of the feature

Comment 158

[Sean Newman]

I see 9 buttons instead of 3 on the test page.
I'm using latest Nightly.

Comment 159

[Alex b1]

(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]]

it works ok in my latest trunk build

Comment 161

[Jan Varga [:janv]]

some early feedback:
https://plus.google.com/115133653231679625609/posts/CJMyExJTbug

Comment 162

[Asaf Romano (gone)]

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]]

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

[Asaf Romano (gone)]

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]]

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]]]

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]

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 168

[Jan Varga [:janv]]

http://hacks.mozilla.org/2011/11/html5-context-menus-in-firefox-screencast-and-code/

Comment 169

[steve faulkner]

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]

(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 (:asurkov)]

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]]

please file a new bug

Comment 173

[alexander :surkov (:asurkov)]

(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)]

(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]

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]

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]

The spec also changed since this was implemented, the work is covered by Bug 897102.

Comment 178

[jaramat]

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]

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 …

Comment 180

[blittiflio]

there should at least be an about:config entry allowing the user to disable this, much like dom.requestcontext.enabled -- perhaps mozilla should even assume that if that existing preference is set to false, the user does not want the website to be able to insert items into the native context menu either.

i just learned about this "feature" and am concerned about the ability for a website to alter the native context menus. It's true that the site could execute the javascript actions themselves, regardless, but the issue I see is that the user could be tricked because they believe they are interacting with trusted native browser code when interacting with the native context menu.

the possibilities for abuse are endless, given that the website owner could insert menu items which could pass as new Firefox controls (especially since mozilla adds new features all the time). But even if we set aside spoofed "new features", existing context menu items could be duplicated such that the user thinks they are interacting with Firefox code when they are instead interacting with website code. examples might include:

1) "Bookmark This Link" -- instead of using Firefox's native bookmark system, the user might be directed to a third-party service, and come to believe it is approved/recommended/run by Mozilla. The user might even be encouraged to install an extension which supplants Firefox's bookmark system.

2) "Exit Full Screen" -- instead of exiting full screen, it doesn't, but then the website's interface is changed to mimic the browser window.

3) "View Page Source" -- maybe user is checking to see if the site is doing anything potentially malicious -- instead of seeing the actual source, the user could be directed to a page which shows the source minus the malicious bits.

4) "Learn more about DRM..." -- instead of being directed to a Mozilla page, the user could be directed to a spoof site which encourages the user to install software under the guise that it's recommended by Mozilla and required to view certain content.

these are just a few examples I came up with. like i said, the possibilities are endless.

Comment 181

[Simon Pieters [:zcorpan]]

FYI: This feature was recently removed from Chromium and so, given the WHATWG working mode (https://whatwg.org/working-mode ), the feature does not meet the criteria for addition but meets the criteria for removal.

See https://github.com/whatwg/html/issues/2730

Comment 182

[Zéfling]

It will always be possible to make a fake context menu in HTML that simulate a native context menu of the browser. Why do not block the actions of the right click in this case?