All users were logged out of Bugzilla on October 13th, 2018

Dragging and dropping a tab is broken

VERIFIED FIXED in Firefox 39

Status

()

--
major
VERIFIED FIXED
4 years ago
a year ago

People

(Reporter: Virtual, Assigned: arai)

Tracking

({nightly-community, regression})

39 Branch
Firefox 39
nightly-community, regression
Points:
---
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox38 unaffected, firefox39+ verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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
(Assignee)

Comment 3

4 years ago
Created attachment 8581557 [details] [diff] [review]
Fix tab drag and drop.

quick fix :)
Assignee: nobody → arai.unmht
Attachment #8581557 - Flags: review?(gavin.sharp)
Duplicate of this bug: 1146329
Duplicate of this bug: 1146343
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+

Updated

4 years ago
Duplicate of this bug: 1146445
(Assignee)

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4412d1dcd6
Keywords: leave-open
(Assignee)

Comment 11

4 years ago
Created attachment 8582232 [details] [diff] [review]
Part 2: Add test for tab reorder and duplicate.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73a89044dc09
Attachment #8582232 - Flags: review?(gavin.sharp)

Updated

4 years ago
Duplicate of this bug: 1146689
Duplicate of this bug: 1146820
Duplicate of this bug: 1146749
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+
Duplicate of this bug: 1146880
Duplicate of this bug: 1146893
(Assignee)

Comment 18

4 years ago
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 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? ;)
(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)
(Assignee)

Comment 23

4 years ago
Created attachment 8582909 [details] [diff] [review]
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
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+
Keywords: leave-open
Tracking since this is a recent regression.
tracking-firefox39: ? → +
https://hg.mozilla.org/mozilla-central/rev/0bf7f53464d6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: affected → fixed
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39

Updated

4 years ago
Iteration: --- → 39.3 - 30 Mar
Status: RESOLVED → VERIFIED
status-firefox39: fixed → verified
You need to log in before you can comment on or make changes to this bug.