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)
Firefox
Tabbed Browser
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.
Reporter | ||
Updated•6 years ago
|
status-firefox60:
affected → ---
Assignee | ||
Comment 1•6 years ago
|
||
If I could know the tests, that I need to look for and starting pointers.
Flags: needinfo?(dao+bmo)
Reporter | ||
Comment 2•6 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Assignee: nobody → rpathangi
Mentor: jaws
Status: NEW → ASSIGNED
Assignee | ||
Updated•6 years ago
|
Assignee: rpathangi → hemantsingh1612
Comment 5•6 years ago
|
||
mozreview-review |
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?
Updated•6 years ago
|
Flags: needinfo?(jaws)
Assignee | ||
Comment 6•6 years ago
|
||
> ::: 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.
Comment 7•6 years ago
|
||
I don't see anywhere in your test where you are pressing control key. So why would your test show a tab getting duplicated?
Assignee | ||
Comment 8•6 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
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?
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(dao+bmo)
Reporter | ||
Comment 11•6 years ago
|
||
(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)
Assignee | ||
Comment 12•6 years ago
|
||
(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)
Reporter | ||
Comment 13•6 years ago
|
||
(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)
Assignee | ||
Comment 14•6 years ago
|
||
(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)
Reporter | ||
Comment 15•6 years ago
|
||
It's not clear to me why you need a native event, but there's nsIDOMWindowUtils::sendNativeMouseEvent.
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Attachment #8955365 -
Flags: review?(jaws) → review?(dao+bmo)
Reporter | ||
Comment 17•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Reporter | ||
Comment 19•6 years ago
|
||
mozreview-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+
Reporter | ||
Comment 20•6 years ago
|
||
If you mark Jaws' issues as fixed, we can get this landed: https://reviewboard.mozilla.org/r/224548/#issue-summary
Flags: needinfo?(hemantsingh1612)
Assignee | ||
Comment 21•6 years ago
|
||
(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)
Comment 22•6 years ago
|
||
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f8cac1a891b5 Test tab reorder when tabbar is overflown r=dao
Reporter | ||
Comment 23•6 years ago
|
||
Thank you and sorry that the review took longer than it should have.
Flags: needinfo?(dao+bmo)
Reporter | ||
Updated•6 years ago
|
Attachment #8955365 -
Flags: review?(jaws)
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f8cac1a891b5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in
before you can comment on or make changes to this bug.
Description
•