Closed Bug 844370 Opened 12 years ago Closed 11 years ago

Work - By default, center the context menu above the selected object

Categories

(Firefox for Metro Graveyard :: General, defect, P2)

x86
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ywang, Assigned: rsilveira)

References

Details

(Whiteboard: feature=work status=verified)

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [metro-mvp?]
Flags: needinfo?(asa)
Yes, I agree.
Flags: needinfo?(asa)
Thanks Asa.  I will move this Change Story to Iteration #3.  What story can I relate it to?
Blocks: metrov1it3
No longer blocks: metrov1triage
Priority: -- → P1
Whiteboard: [metro-mvp?] → feature=change c=tbd u=tbd p=0
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.
Blocks: 781002
No longer blocks: metrov1it3
Summary: Change - By default, center the context menu above the selected object → Work - By default, center the context menu above the selected object
Whiteboard: feature=change c=tbd u=tbd p=0 → feature=work c=tbd u=tbd p=0
Priority: P1 → P2
Whiteboard: feature=work c=tbd u=tbd p=0 → feature=work
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
Assignee: nobody → rsilveira
Attachment #719746 - Flags: review?(mbrubeck)
Comment on attachment 719746 [details] [diff] [review]
Center the context menu above the selected object.

Removed review flag - Need to fix tests.
Attachment #719746 - Flags: review?(mbrubeck)
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.
Applied review comments and added XPCSHELL tests for Util.extend method.
Attachment #719746 - Attachment is obsolete: true
Attachment #721480 - Flags: review?
Comment on attachment 721480 [details] [diff] [review]
Center the context menu above the selected object.

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

Great, thanks!
Attachment #721480 - Flags: review? → review+
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
https://hg.mozilla.org/mozilla-central/rev/6d85ed744247
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Whiteboard: feature=work → feature=work status=verified
Depends on: 859447
No longer depends on: 859447
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: