Closed Bug 1434229 Opened 6 years ago Closed 6 years ago

Improve test coverage for dragging and dropping tabs

Categories

(Firefox :: Tabbed Browser, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: dao, Assigned: hemantsingh1612, Mentored)

References

Details

Attachments

(1 file)

From bug 1387861:

> 1) please file a bug to ensure we have better automated test coverage of
> this. This regression shouldn't have survived automated tests. We have
> automated test coverage of dnd on about:newtab (or at least, we used to
> before activity stream), those might be a useful template to work from as it
> might require synthesizing native events.
If I could know the tests, that I need to look for and starting pointers.
Flags: needinfo?(dao+bmo)
(In reply to hemant from comment #1)
> If I could know the tests, that I need to look for and starting pointers.

browser/base/content/test/general/browser_tabReorder.js and browser/base/content/test/general/browser_tab_drag_drop_perwindow.js might be a good start.
Flags: needinfo?(dao+bmo)
Right now we can select tab, and drag!
Flags: needinfo?(jaws)
Assignee: nobody → rpathangi
Mentor: jaws
Status: NEW → ASSIGNED
Assignee: rpathangi → hemantsingh1612
Comment on attachment 8955365 [details]
Bug 1434229 - Test tab reorder when tabbar is overflown

https://reviewboard.mozilla.org/r/224548/#review232838

::: browser/base/content/test/general/browser.ini:441
(Diff revision 1)
>  [browser_tab_close_dependent_window.js]
>  # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
>  [browser_tabDrop.js]
>  # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
>  [browser_tabReorder.js]
> +[browser_tabReorderNative.js]

As the comments in this file require, no new test should be added to this directory ("general"). This test should go in the /browser/base/content/test/tabs/ directory.

::: browser/base/content/test/general/browser_tabReorderNative.js:35
(Diff revision 1)
> +  await dragAndDrop(newTab2, newTab1);
> +  is(gBrowser.tabs.length, initialTabsLength + 4, "a tab is duplicated");

Why does this cause a tab to get duplicated?
Flags: needinfo?(jaws)
> ::: browser/base/content/test/general/browser_tabReorderNative.js:35
> (Diff revision 1)
> > +  await dragAndDrop(newTab2, newTab1);
> > +  is(gBrowser.tabs.length, initialTabsLength + 4, "a tab is duplicated");
> 
> Why does this cause a tab to get duplicated?

Actually, we do not need those test cases here, unless we are duplicating a tab. Pressing control key will duplicate a tab.
I don't see anywhere in your test where you are pressing control key. So why would your test show a tab getting duplicated?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> I don't see anywhere in your test where you are pressing control key. So why
> would your test show a tab getting duplicated?

My fault :) .I will update patch.
I have two issues:
 
1) This patch will pass all test cases only if mouse is manually hit after drag start ends(please use debugger). So far couldn't find something for it in https://searchfox.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js.

2) From bug 1387681
> 1. Open lots of tabs (e.g. use Ctrl+T)
> 2. Mousedown a tab, drag it to some side
> 
> Expected: The tab is reordered to where the mouse is.
> Result: It moves to another place.
> 
> I have 500 opened tabs, with this I lose track of them and Nightly is
> completely unusable. This needs a quick fix or backout but 1387084.

So how many tabs should I consider here?
Flags: needinfo?(dao+bmo)
(In reply to hemant from comment #10)
> I have two issues:
>  
> 1) This patch will pass all test cases only if mouse is manually hit after
> drag start ends(please use debugger). So far couldn't find something for it
> in
> https://searchfox.org/mozilla-central/source/testing/mochitest/tests/
> SimpleTest/EventUtils.js.

Have you checked the tests from comment 2 or are these somehow different enough that you can't use the same method here?

> 2) From bug 1387681
> > 1. Open lots of tabs (e.g. use Ctrl+T)
> > 2. Mousedown a tab, drag it to some side
> > 
> > Expected: The tab is reordered to where the mouse is.
> > Result: It moves to another place.
> > 
> > I have 500 opened tabs, with this I lose track of them and Nightly is
> > completely unusable. This needs a quick fix or backout but 1387084.
> 
> So how many tabs should I consider here?

Just enough tabs for the tab bar to overflow, I think.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #11)
> Have you checked the tests from comment 2 or are these somehow different
> enough that you can't use the same method here?
Yes methods are different here, trying to synthesize native mouse events.
Flags: needinfo?(dao+bmo)
(In reply to hemant from comment #12)
> (In reply to Dão Gottwald [::dao] from comment #11)
> > Have you checked the tests from comment 2 or are these somehow different
> > enough that you can't use the same method here?
> Yes methods are different here, trying to synthesize native mouse events.

Have you tried synthesizeMouse?
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #13)

> Have you tried synthesizeMouse?

No because I think it:
 1. does not comply with the methods I am currently using in my patch.
 2. does not sysnthesize native mouse events
Flags: needinfo?(dao+bmo)
It's not clear to me why you need a native event, but there's nsIDOMWindowUtils::sendNativeMouseEvent.
Flags: needinfo?(dao+bmo)
Attachment #8955365 - Flags: review?(jaws) → review?(dao+bmo)
Comment on attachment 8955365 [details]
Bug 1434229 - Test tab reorder when tabbar is overflown

https://reviewboard.mozilla.org/r/224548/#review243658

::: browser/base/content/test/tabs/browser.ini:33
(Diff revision 3)
>  [browser_pinnedTabs_closeByKeyboard.js]
>  [browser_positional_attributes.js]
>  [browser_preloadedBrowser_zoom.js]
>  [browser_reload_deleted_file.js]
> +[browser_tabReorder_overflow.js]
>  skip-if = (debug && os == 'mac') || (debug && os == 'linux' && bits == 64) #Bug 1421183, disabled on Linux/OSX for leaked windows

This skip-if belongs to browser_reload_deleted_file.js, not browser_tabReorder_overflow.js.

Looks good otherwise!
Attachment #8955365 - Flags: review?(dao+bmo) → review+
Comment on attachment 8955365 [details]
Bug 1434229 - Test tab reorder when tabbar is overflown

https://reviewboard.mozilla.org/r/224548/#review244946
Attachment #8955365 - Flags: review+
If you mark Jaws' issues as fixed, we can get this landed: https://reviewboard.mozilla.org/r/224548/#issue-summary
Flags: needinfo?(hemantsingh1612)
(In reply to Dão Gottwald [::dao] from comment #20)
> If you mark Jaws' issues as fixed, we can get this landed:
> https://reviewboard.mozilla.org/r/224548/#issue-summary

Done. 

Thank you :dao and jaws for helping me on this. This was a great experience. I will learn from my mistakes & be a better version of myself, thats my only goal :)
Flags: needinfo?(hemantsingh1612) → needinfo?(dao+bmo)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8cac1a891b5
Test tab reorder when tabbar is overflown r=dao
Thank you and sorry that the review took longer than it should have.
Flags: needinfo?(dao+bmo)
Attachment #8955365 - Flags: review?(jaws)
https://hg.mozilla.org/mozilla-central/rev/f8cac1a891b5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.