1146193 - Dragging and dropping a tab is broken

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

STR:
1. Open two tabs in one window
2. Change tab order
3. Nothing happens



Regression window (mozilla-inbound-win32)
Good:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1426877857/

Bad:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1426877934/

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2cb6af5972f8&tochange=72c740d37eb9



@ Bill McCloskey (:billm) - Could you look on this issue?



[Tracking Requested - why for this release]: Regression

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

I see this in Browser Console:

TypeError: this.AppConstants is undefined
   4401           let copyModifier = this.AppConstants.platform == "macosx" ? event.altKey : event.ctrlKey;

Even though I too am running on win7 x64 win32 builds.

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

In my case in Error Console I'm seeing many Errors like these:

Timestamp: 2015-03-23 00:15:31
Error: TypeError: this.AppConstants is undefined
Source File: chrome://browser/content/tabbrowser.xml
Line: 4401

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Fix tab drag and drop.

quick fix :)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8581557 [details] [diff] [review]
Fix tab drag and drop.

Confirmed locally that this fixes it.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8581557 [details] [diff] [review]
Fix tab drag and drop.

Any chance you could write an automated test for this as well?

Comment 9

[Tooru Fujisawa [:arai]]

Thank you for reviewing and feedback :)
I'll post a test shortly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4412d1dcd6

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/840cfd5bc971

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add test for tab reorder and duplicate.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73a89044dc09

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8582232 [details] [diff] [review]
Part 2: Add test for tab reorder and duplicate.

>diff --git a/browser/base/content/test/general/browser_tabReorder.js b/browser/base/content/test/general/browser_tabReorder.js

>+function test() {
>+  let otherTabsLength = gBrowser.tabs.length;

initialTabsLength?

>+  registerCleanupFunction(function () {
>+    while (gBrowser.tabs.length > otherTabsLength) {
>+      gBrowser.removeTab(gBrowser.tabs[otherTabsLength]);
>+    }
>+  });

Always better to explicitly close the tabs you opened than to use a loop like this. Seems easy enough here:

for (let t of [newTab1, newTab2, newTab3]) {
  gBrowser.removeTab(t);
}

>+  let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
>+                     getService(Ci.mozIJSSubScriptLoader);
>+  let EventUtils = {};
>+  scriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils);

You shouldn't need to call loadSubScript - EventUtils is already defined in the global scope of all browser chrome tests (https://hg.mozilla.org/mozilla-central/annotate/840cfd5bc971/testing/mochitest/browser-test.js#l674).

>+  function dragAndDrop(tab1, tab2, copy) {

Can you not just use the synthesizeDrop helper from ChromeUtils (which you will need to import, unlike EventUtils)?

I'm not that picky if this works, but it seems better to rely on a widely used helper than to re-implement it somewhat in this test.

Comment 18

[Tooru Fujisawa [:arai]]

Thank you for feedback :)

(In reply to :Gavin Sharp [email: [e-mail]] from comment #15)
> Comment on attachment 8582232 [details] [diff] [review]
> Part 2: Add test for tab reorder and duplicate.
> 
> >diff --git a/browser/base/content/test/general/browser_tabReorder.js b/browser/base/content/test/general/browser_tabReorder.js
> 
> >+function test() {
> >+  let otherTabsLength = gBrowser.tabs.length;
> 
> initialTabsLength?

Thanks, fixed locally.

> >+  registerCleanupFunction(function () {
> >+    while (gBrowser.tabs.length > otherTabsLength) {
> >+      gBrowser.removeTab(gBrowser.tabs[otherTabsLength]);
> >+    }
> >+  });
> 
> Always better to explicitly close the tabs you opened than to use a loop
> like this. Seems easy enough here:
> 
> for (let t of [newTab1, newTab2, newTab3]) {
>   gBrowser.removeTab(t);
> }

Yeah, but the problem is that new tab is opened by second dragAndDrop call (by duplicating dropped tab), and the cleanup function should close the opened tab with similar code anyway.
Of course, removing known tabs before that will be nice. So s/while/if/ in that case?

> >+  let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
> >+                     getService(Ci.mozIJSSubScriptLoader);
> >+  let EventUtils = {};
> >+  scriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils);
> 
> You shouldn't need to call loadSubScript - EventUtils is already defined in
> the global scope of all browser chrome tests
> (https://hg.mozilla.org/mozilla-central/annotate/840cfd5bc971/testing/
> mochitest/browser-test.js#l674).
> 
> >+  function dragAndDrop(tab1, tab2, copy) {
> 
> Can you not just use the synthesizeDrop helper from ChromeUtils (which you
> will need to import, unlike EventUtils)?
> 
> I'm not that picky if this works, but it seems better to rely on a widely
> used helper than to re-implement it somewhat in this test.

There are several differences between current synthesizeDrop and this test
  * keyboard modifiers (ctrlKey, altKey) in initDragEvent parameter
    to test duplicating a tab
  * drop position (screenX, screenY, clientX, clientY) in initDragEvent parameter
    to move dragging tab into correct position
  * this test doesn't overwrite data in dataTransfer nor call preventDefault/stopPropagation
    it's testing dragstart handler too
Can I introduce those options to synthesizeDrop?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e4412d1dcd6

Comment 20

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Wes Kocher (:KWierso) from comment #10)
> https://hg.mozilla.org/mozilla-central/rev/840cfd5bc971

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> https://hg.mozilla.org/mozilla-central/rev/7e4412d1dcd6

So it landed second time now? ;)

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Virtual_ManPL [:Virtual] from comment #20)
> (In reply to Wes Kocher (:KWierso) from comment #10)
> > https://hg.mozilla.org/mozilla-central/rev/840cfd5bc971
> 
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> > https://hg.mozilla.org/mozilla-central/rev/7e4412d1dcd6
> 
> So it landed second time now? ;)

It landed to inbound in the middle of a lot of broken things, which took longer to sort out than it took for the next Nightly to be built. In the interest of fixing Nightly, I manually grafted the fix to mozilla-central yesterday (the first landing). Now that the normal merge happened, it shows a second landing because our merge tool isn't smart enough to know about the grafting.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tooru Fujisawa [:arai] from comment #18)
> Yeah, but the problem is that new tab is opened by second dragAndDrop call
> (by duplicating dropped tab), and the cleanup function should close the
> opened tab with similar code anyway.
> Of course, removing known tabs before that will be nice. So s/while/if/ in
> that case?

Oh OK. The original code is fine I guess.

> There are several differences between current synthesizeDrop and this test
>   * keyboard modifiers (ctrlKey, altKey) in initDragEvent parameter
>     to test duplicating a tab
>   * drop position (screenX, screenY, clientX, clientY) in initDragEvent
> parameter
>     to move dragging tab into correct position
>   * this test doesn't overwrite data in dataTransfer nor call
> preventDefault/stopPropagation
>     it's testing dragstart handler too
> Can I introduce those options to synthesizeDrop?

Neil might have an opinion on the value of adding these to synthesizeDrop (in another bug), just landing this as-is for now seems fine.

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add test for tab reorder and duplicate.

Thanks again. fixed initialTabsLength and EventUtils.
I'll file a bug for adding options to synthesizeDrop.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=175a7479529e

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8582909 [details] [diff] [review]
Part 2: Add test for tab reorder and duplicate.

Thanks again!

Comment 25

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bf7f53464d6

Filed the bug for synthesizeDrop as bug 1147700.

Comment 26

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking since this is a recent regression.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0bf7f53464d6