682338 - nsXULPopupListener::HandleEvent intends to focus contextmenu target on platforms where it's shown on mousedown, but not those where it's shown on mouseup; been doing exact opposite since 2001

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Graeme McCutcheon [:graememcc]]

Background: Running my patch for bug 46555 through try, browser/base/content/test/test_contextmenu.html on test failed on Windows, passed on Mac (it's disabled on linux). Adjusting the test for the change in behaviour my patch introduced, and I got the same failure on Mac, but now passing on Windows. While debugging, I had reenabled the test on my Linux box, and had noticed when loading the page being tested in the browser, I was unable to focus the empty text field when right-clicking.

Investigating this, I ended up performing some CVS archaeology.

Bug 48838 ensured that on platforms where the context menu is shown on mousedown (i.e. not Windows/OS2) the element is focused first:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/content/src/nsXULPopupListener.cpp&rev=1.64&root=/cvsroot&mark=284-290

AIUI XP_PC was considered a synonym for XP_WIN back in the day.

Along comes version 1.81 of the file in 2001, which #ifndef's the focus-firing code to run only on platforms where NS_CONTEXT_MENU_IS_MOUSEUP isn't defined. Unfortunately, it goes ahead and defines it on platforms where the context menu is shown on mousedown. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/content/src/nsXULPopupListener.cpp&rev=1.81&root=/cvsroot&mark=70-74 and lines 366-370 in the same file.

For test_contextMenu on linux, (and presumably Mac) this meant the test wasn't exactly testing what it thought it was: when constructing the context menu for the input element, nsWindowRoot was dispatching the various IsCommandEnabled calls to the contenteditable element.

On the other hand, the popup code's been that way for 10 and a half years, and no-one else seems to have complained...

Comment 1

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v1

Running through try as we speak...

Comment 2

[Graeme McCutcheon [:graememcc]]

Apart from a WebGL crash I'm claiming wasn't my fault, try was green across the board

Comment 3

[Neil Deakin]

How does test_contextmenu.html fail? I tested this and don't see any failure on windows.

Comment 4

[(no longer active)]

(In reply to Neil Deakin from comment #3)
> How does test_contextmenu.html fail? I tested this and don't see any failure
> on windows.

In comment 0, Graeme explains that this happens when the patch to bug 46555 is applied.

Comment 5

[Neil Deakin]

Comment on attachment 556073 [details] [diff] [review]
Patch v1

OK, we could try this and see how it goes.

However, when I enable the test on linux, it fails in the same manner with the patches in both bugs applied.

Comment 6

[Graeme McCutcheon [:graememcc]]

Neil, sorry missed the bugmail from comment 3.

There was actually a bug relating to widget focus with the test I discovered later - bug 682618 - hence the failures you were seeing either way.

This code looked wrong in any case.

http://hg.mozilla.org/integration/mozilla-inbound/rev/d069439624f5

Comment 7

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/ab8c0ebe600b

Comment 8

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336