Context menu placement is thrown off by sub frames

RESOLVED FIXED

Status

Firefox for Metro
Input
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
x86_64
Windows 8.1
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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)

Updated

5 years ago
Assignee: nobody → jmathies
(Assignee)

Comment 1

5 years ago
Created attachment 717130 [details]
inner textblock.html
(Assignee)

Comment 2

5 years ago
Created attachment 717131 [details]
test case
(Assignee)

Comment 3

5 years ago
Created attachment 717179 [details] [diff] [review]
ContextMenuHandler cleanup patch

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

Comment 4

5 years ago
oops - Util.dumpLn("hello!") has been removed from the local patch. :)
(Assignee)

Comment 5

5 years ago
Created attachment 717185 [details] [diff] [review]
bug fix v.1

actual fix.
Attachment #717185 - Flags: review?(fyan)
(Assignee)

Comment 6

5 years ago
Created attachment 717188 [details] [diff] [review]
tests v.1
Attachment #717188 - Flags: review?(fyan)
(Assignee)

Comment 7

5 years ago
Created attachment 717190 [details] [diff] [review]
tests v.1

- added missing test files
Attachment #717188 - Attachment is obsolete: true
Attachment #717188 - Flags: review?(fyan)
Attachment #717190 - Flags: review?(fyan)

Comment 8

5 years ago
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-
(Assignee)

Comment 9

5 years ago
> @@ +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.
(Assignee)

Comment 10

5 years ago
(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 11

5 years ago
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+
(Assignee)

Comment 12

5 years ago
Created attachment 717214 [details] [diff] [review]
bug fix v.2

- updated per comments
Attachment #717185 - Attachment is obsolete: true
Attachment #717214 - Flags: review?(fyan)
(Assignee)

Updated

5 years ago
Attachment #717214 - Attachment is patch: true
(Assignee)

Comment 13

5 years ago
Oh, crud, I can remove foundFrame now. will update locally.
(Assignee)

Comment 14

5 years ago
Created attachment 717216 [details] [diff] [review]
bug fix v.2

sorry for the bug spam.
Attachment #717214 - Attachment is obsolete: true
Attachment #717214 - Flags: review?(fyan)
Attachment #717216 - Flags: review?(fyan)
(Assignee)

Comment 15

5 years ago
(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 16

5 years ago
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 17

5 years ago
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+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c766cfa31d1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fe51bcd9a8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/369c128126d5
https://hg.mozilla.org/mozilla-central/rev/c766cfa31d1c
https://hg.mozilla.org/mozilla-central/rev/1fe51bcd9a8a
https://hg.mozilla.org/mozilla-central/rev/369c128126d5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.