Closed Bug 844113 Opened 9 years ago Closed 9 years ago

Context menu placement is thrown off by sub frames

Categories

(Firefox for Metro Graveyard :: Input, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jimm, Assigned: jimm)

Details

Attachments

(5 files, 3 obsolete files)

STR:

1) load test case
2) using mouse right-click on the link in the sub frame

note the context menu is positioned correctly

3) scroll page up a bit, and right-click on the same link

note the context menu is positioned incorrectly.

In ContextMenuHandler, we process contextmenu events to analyze what the user clicked on. From this we work up a list of content types, which we forward over to BrowserTouchHandler which passes these events into MenuUI if a context menu is deemed appropriate.

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/contenthandlers/ContextMenuHandler.js#110

In this routine, we are using clientX and clientY of the dom event. In the case of a subframe, clientX and clientY are relative to the frame, not the browser, which throws positioning off.
Assignee: nobody → jmathies
Attached file inner textblock.html
Attached file test case
This is just cleanup / reorg work to make this file more readable. I've also removed some unused params from the json message. The added _onContextAtPoint is related to some selection work I'm doing on top of this, it's not used here.
Attachment #717179 - Flags: review?(fyan)
oops - Util.dumpLn("hello!") has been removed from the local patch. :)
Attached patch bug fix v.1 (obsolete) — Splinter Review
actual fix.
Attachment #717185 - Flags: review?(fyan)
Attached patch tests v.1 (obsolete) — Splinter Review
Attachment #717188 - Flags: review?(fyan)
Attached patch tests v.1Splinter Review
- added missing test files
Attachment #717188 - Attachment is obsolete: true
Attachment #717188 - Flags: review?(fyan)
Attachment #717190 - Flags: review?(fyan)
Comment on attachment 717185 [details] [diff] [review]
bug fix v.1

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

Thanks for writing this patch! :)

::: browser/metro/base/content/contenthandlers/ContextMenuHandler.js
@@ +171,5 @@
> +        element = element.ownerDocument.defaultView.frameElement;
> +        foundFrame = true;
> +        offsetX += element.offsetLeft;
> +        offsetY += element.offsetTop;
> +        return calculateFrameOffset(element);

This kind of recursive loop is actually very inefficient in JS, since you can't optimize tail calls, and you risk triggering "too much recursion" errors.
In this particular, removing the inner function and replacing the `if` with `while` results in the same behavior with simpler code. :)

@@ +177,5 @@
> +    }
> +    calculateFrameOffset(element);
> +    if (foundFrame) {
> +      offsetX -= content.document.defaultView.scrollX;
> +      offsetY -= content.document.defaultView.scrollY;

What if a subframe is also scrolled?
Do we need to move this into the loop?

I don't think offsetTop and offsetLeft are the right properties to check.
If we check the following instead:
let rect = element.getBoundingClientRect();
offsetX += rect.left;
offsetY += rect.top;
then we don't need to adjust for scrolling; plus, offsetLeft and offsetTop are relative to offsetParent, which is not always the root element, because there could be positioning going on: https://developer.mozilla.org/en-US/docs/DOM/element.offsetParent

Also, I think content is the same as content.document.defaultView.

@@ +191,5 @@
>     */
>    _processPopupNode: function _processPopupNode(aPopupNode, aX, aY, aInputSrc) {
>      if (!aPopupNode)
>        return;
> +    let offsetInfo = this._translateToTopLevelWindow(aPopupNode);

If you replace this with:
let { offsetX: offsetX, offsetY: offsetY } = this._translateToTopLevelWindow(aPopupNode);
then offsetX and offsetY will be defined in this scope.

See https://developer.mozilla.org/en-US/docs/JavaScript/New_in_JavaScript/1.7#Destructuring_assignment_%28Merge_into_own_page.2Fsection%29 for more destructured assignment examples.
Attachment #717185 - Flags: review?(fyan) → review-
> @@ +177,5 @@
> > +    }
> > +    calculateFrameOffset(element);
> > +    if (foundFrame) {
> > +      offsetX -= content.document.defaultView.scrollX;
> > +      offsetY -= content.document.defaultView.scrollY;
> 
> What if a subframe is also scrolled?
> Do we need to move this into the loop?

No we really just want frame offsets, but for some reason this didn't work unless we adjusted for the root frame scroll. Maybe your suggestion below will address this.
(In reply to Jim Mathies [:jimm] from comment #9)
> > @@ +177,5 @@
> > > +    }
> > > +    calculateFrameOffset(element);
> > > +    if (foundFrame) {
> > > +      offsetX -= content.document.defaultView.scrollX;
> > > +      offsetY -= content.document.defaultView.scrollY;
> > 
> > What if a subframe is also scrolled?
> > Do we need to move this into the loop?
> 
> No we really just want frame offsets, but for some reason this didn't work
> unless we adjusted for the root frame scroll. Maybe your suggestion below
> will address this.

Yep that fixed it. :)
Comment on attachment 717190 [details] [diff] [review]
tests v.1

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

Overall, it looks fine, but I'm not familiar enough with tests to be confident in reviewing this one.
Deferring to Matt for a final review.

::: browser/metro/base/tests/browser_context_menu_tests.js
@@ +413,5 @@
> +
> +    ContextUI.dismiss();
> +
> +    let frame = win.document.getElementById("frame1");
> +    let flink = frame.contentDocument.getElementById("link1");

Could you make these IDs make their corresponding variable names?

::: browser/metro/base/tests/browser_context_menu_tests_03.html
@@ +3,5 @@
> +<meta http-equiv="content-type" content="text/html; charset=UTF-8">
> +<meta charset="utf-8">
> +</head>
> +
> +<body  bgcolor=white>

Not sure how picky we are with regards to HTML in tests, but I would prefer having the following instead of the above:

<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
</head>
<body>

::: browser/metro/base/tests/text-block.html
@@ +2,5 @@
> +<html xmlns="http://www.w3.org/1999/xhtml" lang="en"><head>
> +<meta http-equiv="content-type" content="text/html; charset=UTF-8">
> +<meta charset="utf-8">
> +</head>
> +<body>

Likewise for this header.

I don't think it's necessary to have this large of a block of text for testing, but perhaps I'm just being byte-stingy.
Attachment #717190 - Flags: review?(mbrubeck)
Attachment #717190 - Flags: review?(fyan)
Attachment #717190 - Flags: feedback+
Attached patch bug fix v.2 (obsolete) — Splinter Review
- updated per comments
Attachment #717185 - Attachment is obsolete: true
Attachment #717214 - Flags: review?(fyan)
Attachment #717214 - Attachment is patch: true
Oh, crud, I can remove foundFrame now. will update locally.
Attached patch bug fix v.2Splinter Review
sorry for the bug spam.
Attachment #717214 - Attachment is obsolete: true
Attachment #717214 - Flags: review?(fyan)
Attachment #717216 - Flags: review?(fyan)
(In reply to Frank Yan (:fryn) from comment #11)
> I don't think it's necessary to have this large of a block of text for
> testing, but perhaps I'm just being byte-stingy.

I can cut that down in size a bit.
Comment on attachment 717216 [details] [diff] [review]
bug fix v.2

This looks right. Have you tested it? :)
Attachment #717216 - Flags: review?(fyan) → review+
Comment on attachment 717179 [details] [diff] [review]
ContextMenuHandler cleanup patch

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

Thanks for writing this. :)

::: browser/metro/base/content/contenthandlers/ContextMenuHandler.js
@@ +41,5 @@
> +      case "Browser:ContextCommand":
> +        this._onContextCommand(aMessage);
> +      break;
> +      case "Browser:InvokeContextAtPoint":
> +        Util.dumpLn("hello!");

Please remove this line. :P
Attachment #717179 - Flags: review?(fyan) → review+
Comment on attachment 717190 [details] [diff] [review]
tests v.1

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

Looks good.  I agree with fryn's review comments above.

::: browser/metro/base/tests/browser_context_menu_tests.js
@@ +460,5 @@
> +    // should be visible
> +    ok(ContextMenuUI._menuPopup._visible, "is visible");
> +
> +    ok(ContextMenuUI._panel.left > 730 && ContextMenuUI._panel.left < 745, "position");
> +    ok(ContextMenuUI._panel.top > 600 && ContextMenuUI._panel.top < 610, "position");

I'm guessing this test will fail when run on a small screen in portrait mode.  I... don't really care about that, but perhaps we should document a minimum screen size requirement somewhere.  Or just log a message and bail out if the window is under 1024px wide when the test starts.
Attachment #717190 - Flags: review?(mbrubeck) → review+
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.