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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ywang, Assigned: rsilveira)
References
Details
(Whiteboard: feature=work status=verified)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•12 years ago
|
Whiteboard: [metro-mvp?]
Updated•12 years ago
|
Blocks: metrov1triage
Flags: needinfo?(asa)
Comment 2•12 years ago
|
||
Thanks Asa. I will move this Change Story to Iteration #3. What story can I relate it to?
Priority: -- → P1
Updated•12 years ago
|
Whiteboard: [metro-mvp?] → feature=change c=tbd u=tbd p=0
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.
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
Updated•12 years ago
|
Priority: P1 → P2
Whiteboard: feature=work c=tbd u=tbd p=0 → feature=work
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
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → rsilveira
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #719746 -
Flags: review?(mbrubeck)
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.
Attachment #719746 -
Flags: review?(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.
Assignee | ||
Comment 8•11 years ago
|
||
Applied review comments and added XPCSHELL tests for Util.extend method.
Attachment #719746 -
Attachment is obsolete: true
Attachment #721480 -
Flags: review?
Comment 9•11 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!
Attachment #721480 -
Flags: review? → review+
Comment 10•11 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
Comment 12•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6d85ed744247
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•11 years ago
|
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.
Whiteboard: feature=work → feature=work status=verified
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•