Last Comment Bug 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
: nsXULPopupListener::HandleEvent intends to focus contextmenu target on platfo...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Graeme McCutcheon [:graememcc]
:
Mentors:
Depends on:
Blocks: 686024
  Show dependency treegraph
 
Reported: 2011-08-26 10:54 PDT by Graeme McCutcheon [:graememcc]
Modified: 2011-09-27 11:59 PDT (History)
4 users (show)
graeme.mccutcheon: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (901 bytes, patch)
2011-08-26 10:58 PDT, Graeme McCutcheon [:graememcc]
enndeakin: review+
Details | Diff | Review

Description Graeme McCutcheon [:graememcc] 2011-08-26 10:54:12 PDT
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] 2011-08-26 10:58:07 PDT
Created attachment 556073 [details] [diff] [review]
Patch v1

Running through try as we speak...
Comment 2 Graeme McCutcheon [:graememcc] 2011-08-27 04:41:23 PDT
Apart from a WebGL crash I'm claiming wasn't my fault, try was green across the board
Comment 3 Neil Deakin 2011-08-30 07:20:56 PDT
How does test_contextmenu.html fail? I tested this and don't see any failure on windows.
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-07 14:27:44 PDT
(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 2011-09-09 11:31:45 PDT
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] 2011-09-10 08:51:47 PDT
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) 2011-09-11 06:37:12 PDT
http://hg.mozilla.org/mozilla-central/rev/ab8c0ebe600b

Note You need to log in before you can comment on or make changes to this bug.