Yuan Wang(:Yuan) – Mobile Firefox Design Lead Reporter
	Description • 12 years ago

Currently the context menu opens on the right-hand side of the user's touch point.

Based on the MSDN standard, it's recommended to set the context menu above the object that it's acting on. http://msdn.microsoft.com/en-us/library/windows/apps/hh465308

I think that also makes sense for either right-hand users or left-hand users. 

One type of edge cases is when there is no enough space to display context menu above. In that case, the context menu should be centered below the selected text.

	Asa Dotzler [:asa]
	Comment 1 • 12 years ago

Yes, I agree.

	Marco Mucci [:MarcoM]
	Comment 2 • 12 years ago

Thanks Asa.  I will move this Change Story to Iteration #3.  What story can I relate it to?

	Asa Dotzler [:asa]
	Comment 3 • 12 years ago

This is work that can block an existing and unscheduled story, Bug 781002 - Story - Apply metro styling to the context menu.  Marking it as such.

	Matt Brubeck (:mbrubeck)
	Comment 4 • 12 years ago

Rodrigo, this might be a good bug to work on during the next iteration.  We'll want to change the options passed from ContextMenuUI.showContextMenu to MenuPopup.prototype.show here:

http://hg.mozilla.org/mozilla-central/file/06935f2db267/browser/metro/base/content/helperui/MenuUI.js#l181

The options are interpreted here; you may need to add some new options:

http://hg.mozilla.org/mozilla-central/file/06935f2db267/browser/metro/base/content/helperui/MenuUI.js#l374

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 6 • 12 years ago

Comment on attachment 719746 [details] [diff] [review]
Center the context menu above the selected object.

Removed review flag - Need to fix tests.

	Matt Brubeck (:mbrubeck)
	Comment 7 • 12 years ago

Comment on attachment 719746 [details] [diff] [review]
Center the context menu above the selected object.

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

Cool!  Here's a drive-by review with some nit-picky style requests:

::: browser/metro/base/content/Util.js
@@ +38,5 @@
>    },
>  
> +  // Pass several objects in and it will combine them all into the first object and return it.
> +  // NOTE: Deep copy is not supported
> +  extend: function extend() {

I wonder if we can use the merge function here:
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/util/object.js

To make object.js importable as a JSM, we might need to add some module boilerplate like the one in promise.js:
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/core/promise.js

Adding our own version to Util.js is fine for now; we can file a follow-up bug to share this with the SDK.

::: browser/metro/base/content/helperui/MenuUI.js
@@ +120,5 @@
>     *
>     * json: TBD
>     */
>    showContextMenu: function ch_showContextMenu(aMessage) {
> +    this._popupState = Util.extend(this._defaultPopupState, aMessage.json);

This will modify _defaultPopupState, right?  It seems safer to clone it, so we can't accidentally modify defaults we care about:

  this._popupState = Util.extend({}, this._defaultPopupState, aMessage.json);

Actually, I think we should separate the position options from all the non-position information in the json property, and make it explicit what we are actually reading from the json:

  this._popupState = aMessage.json;
  // ...
  this.show(Util.extend({}, this._defaultPositionOptions, {
    xPos: aMessage.json.xPos,
    yPos: aMessage.json.yPos,
    source: aMessage.json.source
  });

I know it makes the code a bit more verbose, but it'll save us from having to backtrack through several layers of indirection to figure out where options are coming from.

@@ +385,5 @@
> +    let forcePosition = !!aPositionOptions.forcePosition;
> +    let isRightAligned = !!aPositionOptions.rightAligned;
> +    let isBottomAligned = !!aPositionOptions.bottomAligned;
> +    let isCenterHorizontally = !!aPositionOptions.centerHorizontally;
> +    let isMoveBelowToFit = !!aPositionOptions.moveBelowToFit;

I like "!!x" better than "x || false", but I'd actually rather get rid of these variables completely.  We can just use "if (aPositionOptions.forcePosition)" below, with no change in meaning.

If "aPositionOptions" is too long to repeat everywhere, we can shorten the name to "aOptions" or something.

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 8 • 12 years ago

Applied review comments and added XPCSHELL tests for Util.extend method.

	Matt Brubeck (:mbrubeck)
	Comment 9 • 12 years ago

Comment on attachment 721480 [details] [diff] [review]
Center the context menu above the selected object.

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

Great, thanks!

	Matt Brubeck (:mbrubeck)
	Comment 10 • 12 years ago

I pushed this to Try just to make sure that xpcshell does not break on platforms where we do not build Metro:
https://tbpl.mozilla.org/?tree=Try&rev=4ce296baac6d

	Matt Brubeck (:mbrubeck)
	Comment 11 • 12 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/6d85ed744247

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/6d85ed744247

	juan becerra [:juanb]
	Comment 13 • 11 years ago

Tested on 2013-03-12 on nightly built from http://hg.mozilla.org/mozilla-central/rev/7433bc4545c9
- Verified that the context menu showed up centered and above touch point on links, text selections, and images.
- Filed bug 850335 for contex menu overflow on edges.