Closed Bug 1146193 Opened 9 years ago Closed 9 years ago

Dragging and dropping a tab is broken

Categories

(Firefox :: Tabbed Browser, defect)

39 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 39
Iteration:
39.3 - 30 Mar
Tracking Status
firefox38 --- unaffected
firefox39 + verified

People

(Reporter: Virtual, Assigned: arai)

References

Details

(Keywords: nightly-community, regression)

Attachments

(2 files, 1 obsolete file)

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
Flags: needinfo?(wmccloskey)
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.
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
No longer blocks: 1145394
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: Dragging and dropping a tab in the same window in broken → Dragging and dropping a tab is broken
quick fix :)
Assignee: nobody → arai.unmht
Attachment #8581557 - Flags: review?(gavin.sharp)
Comment on attachment 8581557 [details] [diff] [review]
Fix tab drag and drop.

Confirmed locally that this fixes it.
Attachment #8581557 - Flags: feedback+
Comment on attachment 8581557 [details] [diff] [review]
Fix tab drag and drop.

Any chance you could write an automated test for this as well?
Flags: needinfo?(wmccloskey)
Attachment #8581557 - Flags: review?(gavin.sharp) → review+
Flags: qe-verify+
Flags: in-testsuite?
Flags: firefox-backlog+
Thank you for reviewing and feedback :)
I'll post a test shortly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4412d1dcd6
Keywords: leave-open
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.
Attachment #8582232 - Flags: review?(gavin.sharp) → feedback+
Thank you for feedback :)

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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?
Flags: needinfo?(gavin.sharp)
(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.
(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.
Flags: needinfo?(gavin.sharp)
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
Attachment #8582232 - Attachment is obsolete: true
Attachment #8582909 - Flags: review?(gavin.sharp)
Comment on attachment 8582909 [details] [diff] [review]
Part 2: Add test for tab reorder and duplicate.

Thanks again!
Attachment #8582909 - Flags: review?(gavin.sharp) → review+
Tracking since this is a recent regression.
https://hg.mozilla.org/mozilla-central/rev/0bf7f53464d6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Iteration: --- → 39.3 - 30 Mar
You need to log in before you can comment on or make changes to this bug.