Closed Bug 682338 Opened 8 years ago Closed 8 years ago
XULPopup Listener::Handle Event 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
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...
Running through try as we speak...
Attachment #556073 - Flags: review?(enndeakin)
Apart from a WebGL crash I'm claiming wasn't my fault, try was green across the board
How does test_contextmenu.html fail? I tested this and don't see any failure on windows.
(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 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.
Attachment #556073 - Flags: review?(enndeakin) → review+
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
Assignee: nobody → graememcc_firefox
No longer blocks: 46555
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.