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

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?

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.

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.

Paul, is that implemented by any UA?

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

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

This feature could be only available for App Tabs (I think we can consider these pages as trusted by the user).

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

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.

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.

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

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?

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

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

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

We also need to take into account exposure to accessibility APIs. e. g. the XUL generated context menu mixed in with the HTML5 stuff.

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

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

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 ?

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

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

(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

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.

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>

(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

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

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

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

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.

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

(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

Jan, can the already reviewed patch be pushed alone? In addition, is it possible to split the patch? is it ready to be reviewed?

(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

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

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

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.

Neil, could you take a look at XUL related changes ? especially changes to these classes/interfaces: nsXULElement, nsXULPopupListener and nsIDOMXULElement

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

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

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

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

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

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

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

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

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

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.

(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

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

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

(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

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

ok, it just means the attribute can return null

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

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

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

Note that accessKeyLabel is being implemented in bug 583533.

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

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.

(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

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

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

(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"? ;)

(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

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

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

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

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

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

Just for reference, here is some background behind html menus: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2005-October/004883.html

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 ?

No, HTMLOptionElement.label should always return its label.

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.

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>

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

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

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.

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

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.

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

Added comments to IDL interfaces and an automatic test.

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

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

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

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; }

ok, I'll move it to PostHandleEvent()

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; {}

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

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 on attachment 546209 [details] [diff] [review] Part 2 - Core implementation v3 Neil, could you review the toolkit part of the patch ?

Comment on attachment 546209 [details] [diff] [review] Part 2 - Core implementation v3 Dão, could you review the /browser part ?

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.

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

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

- 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

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 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 on attachment 547069 [details] [diff] [review] Part 2 - Core implementation v4 Neil, could you review the toolkit part of the patch ?

Comment on attachment 548799 [details] [diff] [review] Part 2 - Core implementation v7 Please don't forget to file spec bugs.

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

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

(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

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

(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

<command> -> <menuitem>

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

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

addressed Neil's comments

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

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

(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

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

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

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

I updated the demo at http://people.mozilla.com/~prouget/bugs/context-menu-test/test.html I just needed to replace <command> with <menuitem>

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

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

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

(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

According to the spec <menuitem> should be <command> - can we support both? http://www.w3.org/TR/html5/interactive-elements.html#context-menus

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.

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

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.

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

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

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

(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

it works ok in my latest trunk build

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

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.

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

Are you sure xul elements can be created within a html document? I'm not sure in what way remote-xul has been disabled.

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

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

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.

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)

(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

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?

please file a new bug

(In reply to Jan Varga [:janv] from comment #172) > please file a new bug it sounds there's one already: bug 699709

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

Was the "title" attribute excluded in the above discussion from being allowable as a tooltip when present on contextmenu menuitems? Would be quite handy...

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

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

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?

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 …

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.

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

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?