Closed Bug 1426250 Opened 6 years ago Closed 6 years ago

Use non-0 coordinates for simulated drag events in customizableui mochitests so we actually test dnd behaviour properly

Categories

(Firefox :: Toolbars and Customization, enhancement)

Product:
Firefox
Component:
Toolbars and Customization
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 6 obsolete files)

Bug 1426250 - allow EventUtils to dispatch drag/mouse events through the DOM only,
59 bytes, text/x-review-board-request
Details
Bug 1426250 - allow changing log preference at runtime for CUI.jsm,
59 bytes, text/x-review-board-request
Details
Bug 1426250 - make tests actually pass valid mouse coordinates when testing Customize Mode,
59 bytes, text/x-review-board-request
Details 
Split off from bug 1424452. Jared and I did some debugging, and in JS-land, we call sendDragEvent in EventUtils.js . The drag event object we send off to the pres shell (via DOMWindowUtils) to trigger dragover and then drop has a non-0 clientX/clientY (which EventUtils.js calculates based on the position of the target of the drop). But the event that our listener gets has them both as 0. The code currently has a workaround for this, but unfortunately this means part of the code isn't executed when its automated tests run, which hides actual issues with that code, which is how bug 1424452 wasn't caught by our tests. We'd like to fix this, but it's not clear where the mouse coordinates are getting dropped on the floor and how to fix that.

Olli, do you have any ideas off-hand?
Flags: needinfo?(bugs)
Yes, nsIDOMWindowUtils::DispatchDOMEventViaPresShell uses the underlying widget event which doesn't contain the client coordinates (that's stored in the dom event) so essentially it gets ignored.

That said, I think some tests that aren't actually testing the drag and drop mechanism and just testing that event handlers work correctly can just use dispatchEvent instead as browser_newtab_bug723121.js does.

I'm not clear from bug 1424452 which tests are affected here.
(In reply to Neil Deakin from comment #1)
> That said, I think some tests that aren't actually testing the drag and drop
> mechanism and just testing that event handlers work correctly can just use
> dispatchEvent instead as browser_newtab_bug723121.js does.
> 
> I'm not clear from bug 1424452 which tests are affected here.

Pretty much anything under browser/components/customizableui/ that uses the drag/drop helpers in head.js . So just updating those helpers might be enough, though obviously that might result in new intermittents/failures.
Flags: needinfo?(bugs)
Attachment #8941004 - Flags: review?(enndeakin)
Comment on attachment 8941005 [details]
Bug 1426250 - allow changing log preference at runtime for CUI.jsm,

https://reviewboard.mozilla.org/r/211292/#review217066

::: browser/components/customizableui/CustomizableUI.jsm:165
(Diff revision 1)
>  };
>  
>  XPCOMUtils.defineLazyGetter(this, "log", () => {
>    let scope = {};
>    Cu.import("resource://gre/modules/Console.jsm", scope);
> -  let debug = Services.prefs.getBoolPref(kPrefCustomizationDebug, false);
> +  XPCOMUtils.defineLazyPreferenceGetter(this, "gDebuggingEnabled", kPrefCustomizationDebug, false,

To be clear, I'm only doing this so we can toggle this pref e.g. with `pushPrefEnv` in tests. Before, because we call `log.debug` immediately in `initialize()`, it gets run well before the tests get loaded and so it's impossible to just turn on logging for 1 test, you have to modify firefox.js and rebuild, which is annoying. So it's not directly related to the patch but helped me debug failures I ran into when modifying code...

::: browser/components/customizableui/CustomizableUI.jsm:168
(Diff revision 1)
>    let scope = {};
>    Cu.import("resource://gre/modules/Console.jsm", scope);
> -  let debug = Services.prefs.getBoolPref(kPrefCustomizationDebug, false);
> +  XPCOMUtils.defineLazyPreferenceGetter(this, "gDebuggingEnabled", kPrefCustomizationDebug, false,
> +    (pref, oldVal, newVal) => log.maxLogLevel = newVal ? "all" : "log");
>    let consoleOptions = {
> -    maxLogLevel: debug ? "all" : "log",
> +    maxLogLevel: gDebuggingEnabled ? "all" : "log", // eslint-disable-line no-undef

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1429024 for eslint not coping here.
Summary: Figure out why dragover clientX/clientY are 0 for simulated drag events in mochitests, despite the mochitest framework setting them up correctly → Use non-0 coordinates for simulated drag events in customizableui mochitests so we actually test dnd behaviour properly
Comment on attachment 8941004 [details]
Bug 1426250 - allow EventUtils to dispatch drag/mouse events through the DOM only,

https://reviewboard.mozilla.org/r/211290/#review217146
(In reply to Neil Deakin from comment #9)
> Comment on attachment 8941004 [details]
> Bug 1426250 - allow EventUtils to dispatch drag/mouse events through the DOM
> only,
> 
> https://reviewboard.mozilla.org/r/211290/#review217146

Did you mean to set r+ ? :-)
Flags: needinfo?(enndeakin)
Flags: needinfo?(enndeakin)
Attachment #8941004 - Flags: review?(enndeakin) → review+
Comment on attachment 8941004 [details]
Bug 1426250 - allow EventUtils to dispatch drag/mouse events through the DOM only,

https://reviewboard.mozilla.org/r/211290/#review217170
Comment on attachment 8941005 [details]
Bug 1426250 - allow changing log preference at runtime for CUI.jsm,

https://reviewboard.mozilla.org/r/211292/#review217524

::: browser/components/customizableui/CustomizableUI.jsm:168
(Diff revision 1)
>    let scope = {};
>    Cu.import("resource://gre/modules/Console.jsm", scope);
> -  let debug = Services.prefs.getBoolPref(kPrefCustomizationDebug, false);
> +  XPCOMUtils.defineLazyPreferenceGetter(this, "gDebuggingEnabled", kPrefCustomizationDebug, false,
> +    (pref, oldVal, newVal) => log.maxLogLevel = newVal ? "all" : "log");
>    let consoleOptions = {
> -    maxLogLevel: debug ? "all" : "log",
> +    maxLogLevel: gDebuggingEnabled ? "all" : "log", // eslint-disable-line no-undef

Looks like I should just move the pref getter to the global scope and then this will work.
Comment on attachment 8941005 [details]
Bug 1426250 - allow changing log preference at runtime for CUI.jsm,

https://reviewboard.mozilla.org/r/211292/#review217908

::: browser/components/customizableui/CustomizableUI.jsm:168
(Diff revision 1)
>    let scope = {};
>    Cu.import("resource://gre/modules/Console.jsm", scope);
> -  let debug = Services.prefs.getBoolPref(kPrefCustomizationDebug, false);
> +  XPCOMUtils.defineLazyPreferenceGetter(this, "gDebuggingEnabled", kPrefCustomizationDebug, false,
> +    (pref, oldVal, newVal) => log.maxLogLevel = newVal ? "all" : "log");
>    let consoleOptions = {
> -    maxLogLevel: debug ? "all" : "log",
> +    maxLogLevel: gDebuggingEnabled ? "all" : "log", // eslint-disable-line no-undef

Alternatively you could declare that gDebuggingEnabled is a global, otherwise these "eslint-disable" comments will get littered all over.

/* globals gDebuggingEnabled */
Attachment #8941005 - Flags: review?(jaws) → review+
Comment on attachment 8941006 [details]
Bug 1426250 - make tests actually pass valid mouse coordinates when testing Customize Mode,

https://reviewboard.mozilla.org/r/211294/#review217918

::: browser/components/customizableui/test/head.js:183
(Diff revision 1)
> +    if (ev == "end") {
> +      ev = {clientX: bounds.right - 2, clientY: bounds.bottom - 2};
> +    } else {
> +      ev = {clientX: bounds.left + 2, clientY: bounds.top + 2};

The names are "start" and "end" but this doesn't account for RTL, and most likely most of our tests don't either but I figure I should point it out. Should we add one test that does a RTL drag/drop so we have some test coverage there? You can do a follow-up for this if you'd like.
Attachment #8941006 - Flags: review?(jaws) → review+
Attachment #8941004 - Attachment is obsolete: true
Attachment #8941005 - Attachment is obsolete: true
Attachment #8941006 - Attachment is obsolete: true
Uhhh... thanks, mozreview, for losing all my reviews. :-\
Attachment #8942139 - Attachment is obsolete: true
Attachment #8942140 - Attachment is obsolete: true
Attachment #8942140 - Flags: review?(jaws)
Attachment #8942141 - Attachment is obsolete: true
Attachment #8942141 - Flags: review?(jaws)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/462b35675a11
allow EventUtils to dispatch drag/mouse events through the DOM only, r=enndeakin
https://hg.mozilla.org/integration/mozilla-inbound/rev/5551470e71c3
allow changing log preference at runtime for CUI.jsm, r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/21ce1e95d697
make tests actually pass valid mouse coordinates when testing Customize Mode, r=jaws
Attachment #8942143 - Flags: review?(jaws)
Attachment #8942144 - Flags: review?(jaws)
Depends on: 1430062
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: