Closed Bug 1435142 Opened 7 years ago Closed 7 years ago

Pref to enable close selected tab by dblclicking it

Categories

(Firefox :: Tabbed Browser, enhancement, P2)

Product:
Firefox
Component:
Tabbed Browser
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- verified

People

(Reporter: hectorz, Assigned: hectorz)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 1 obsolete file)

Bug 1435142 - Part 1: pref to enable close selected tab by dblclicking it.
59 bytes, text/x-review-board-request
dao
: review+
Details
Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick.
59 bytes, text/x-review-board-request
dao
: review+
mixedpuppy
: review+
Details
Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
As mentioned in bug 1383669, most browsers in China allows users to close a tab by dblclicking it, and we at Beijing office currently ships a Mozilla signed extension to implement this. Since dblclick on unselected tab would select that tab before it gets closed, I'll limit the implementation to selected tab only. Stats we've collected [1] shows this covers ~80% of the dblclicks. Also, this limitation would prevent a malfunctioning mouse from accidentally close another tab when a user wants to select it, which is a common complaint we get. I'm sending my initial patch for early feedback. Is there a better way to detect a dblclick on selected tab? Also should I still add logic to xbl bindings or not? I'll update the patch to include tests and expose this to WebExtension later. [1]: https://github.com/MozillaOnline/tabtweak/commit/cf363014
Priority: -- → P2
(In reply to Hector Zhao [:hectorz] from comment #0) > > I'm sending my initial patch for early feedback. Is there a better way to > detect a dblclick on selected tab? Also should I still add logic to xbl > bindings or not? > > I'll update the patch to include tests and expose this to WebExtension later. Patches updated to include these two parts. I thought this might be affected by bug 1392352 (looks like it isn't), and waited until it landed before I picked this back up. I also learned about UIEvent.detail of click/dblclick/mousedown/mouseup when trying to figure out the tests, and updated the part 1 to use it instead of counting by myself.
Comment on attachment 8956371 [details] Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. I dont see any reason to expose this in webextensions. The pref can be switched on by default for the china distribution. The option should also be exposed in about:preferences.
Attachment #8956371 - Flags: review?(mixedpuppy) → review-
Comment on attachment 8947708 [details] Bug 1435142 - Part 1: pref to enable close selected tab by dblclicking it. https://reviewboard.mozilla.org/r/217422/#review231290 ::: browser/base/content/tabbrowser.xml:982 (Diff revision 2) > } else if (tab.closing) { > this.tabbrowser._endRemoveTab(tab); > } > ]]></handler> > > + <handler event="mousedown" button="0" phase="capturing"><![CDATA[ Why are you doing this in the tabbrowser-tabs binding rather than in tabbrowser-tab? ::: browser/base/content/tabbrowser.xml:984 (Diff revision 2) > } > ]]></handler> > > + <handler event="mousedown" button="0" phase="capturing"><![CDATA[ > + if (!this._closeTabByDblclick || > + event.button != 0 || You already put button="0" on the handler element. ::: browser/base/content/tabbrowser.xml:987 (Diff revision 2) > + <handler event="mousedown" button="0" phase="capturing"><![CDATA[ > + if (!this._closeTabByDblclick || > + event.button != 0 || > + event.detail != 1 || > + event.target.localName != "tab") { > + return; Invert the conditions and move this._firstMouseDownOnSelectedTab = ... into this block? ::: browser/base/content/tabbrowser.xml:995 (Diff revision 2) > + this._firstMouseDownOnSelectedTab = event.target.selected; > + ]]></handler> > + > <handler event="dblclick"><![CDATA[ > + if (this._closeTabByDblclick && > + this._firstMouseDownOnSelectedTab && Why are you checking whether the tab is selected in the mousedown handler only and not here? The tab could be a different one here. That's actually not that unrealistic. ::: browser/base/content/tabbrowser.xml:1001 (Diff revision 2) > + event.button == 0 && > + event.target.localName == "tab") { > + let tab = event.target; > + if (!tab._overPlayingIcon) { > + this.tabbrowser.removeTab(tab, {animate: true, > + byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE}); nit: format like this: this.tabbrowser.removeTab(tab, { animate: true, byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE, });
Attachment #8947708 - Flags: review?(dao+bmo) → review-
mconca for approval of new api.
Flags: needinfo?(mconca)
Thanks :mixedpuppy for giving me a heads up. The request to expose this browser preference via a WebExtensions API came without much notice. That said, it was a common feature of several add-ons in the past including TabMixPlus, one of the most popular tab managment add-ons. In addition, the UX supported by this change is common in China, making it likely that this feature will continue to be supported in the future. I recommended this API be approved.
Flags: needinfo?(mconca)
Comment on attachment 8947708 [details] Bug 1435142 - Part 1: pref to enable close selected tab by dblclicking it. https://reviewboard.mozilla.org/r/217422/#review231290 > Why are you doing this in the tabbrowser-tabs binding rather than in tabbrowser-tab? Moved into tabbrowser-tab. I think it is because this patch was created based on our extension, where adding an event listener to tabContainer is easier than individual tabs. > You already put button="0" on the handler element. Keep it since there's no `button="0"` on tabbrowser-tab's mousedown handler. > Invert the conditions and move this._firstMouseDownOnSelectedTab = ... into this block? Done. > Why are you checking whether the tab is selected in the mousedown handler only and not here? The tab could be a different one here. That's actually not that unrealistic. When I first tried gathering some stats about dblclicking, I checked `tab.selected` in the dblclick handler, but they're all true. I guess the `mousedown,mouseup,click,mousedown,mouseup,click,dblclick` sequence means the tab where `dblclick` happens must have been selected by the preceding `mousedown`? > nit: format like this: > > this.tabbrowser.removeTab(tab, { > animate: true, > byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE, > }); Done.
(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #9) > Thanks :mixedpuppy for giving me a heads up. The request to expose this > browser preference via a WebExtensions API came without much notice. That > said, it was a common feature of several add-ons in the past including > TabMixPlus, one of the most popular tab managment add-ons. In addition, the > UX supported by this change is common in China, making it likely that this > feature will continue to be supported in the future. > > I recommended this API be approved. Thanks, with this exposed, we'll finally be able to move our tabtweak@mozillaonline.com to a proper WebExtension. Sorry I didn't raise this earlier. (In reply to Shane Caraveo (:mixedpuppy) from comment #6) > > I dont see any reason to expose this in webextensions. > > The pref can be switched on by default for the china distribution. Thanks for flagging :mconca about this. An extension API is more flexable to use than the distribution files. > The option should also be exposed in about:preferences. I think none of the tab behavior prefs behind `openBookmarksInNewTabs`, `openSearchResultsInNewTabs` and the incoming `newTabPosition` are exposed in about:preferences?
Assignee: nobody → bzhao
Status: NEW → ASSIGNED
Comment on attachment 8947708 [details] Bug 1435142 - Part 1: pref to enable close selected tab by dblclicking it. https://reviewboard.mozilla.org/r/217422/#review231598 ::: browser/base/content/tabbrowser.xml:950 (Diff revision 4) > + animate: true, > + byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE, > + }); > + } > + return; > + } Move this to the tabbrowser-tab binding as well? ::: browser/base/content/tabbrowser.xml:1910 (Diff revision 4) > <![CDATA[ > + let tabContainer = this.parentNode; > + if (tabContainer._closeTabByDblclick && > + event.button == 0 && > + event.detail == 1 && > + event.target.localName == "tab") { I don't think event.target.localName == "tab" can ever be false here...
Attachment #8947708 - Flags: review?(dao+bmo)
Comment on attachment 8956371 [details] Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. https://reviewboard.mozilla.org/r/225232/#review231648 This looks fine given my comment below. I'm waiting to r+ for a bit of conversation on whether this is the right approach or something more generic like a click listener wouldn't be better. ::: toolkit/components/extensions/schemas/browser_settings.json:120 (Diff revision 3) > + "closeTabByDblclick": { > + "$ref": "types.Setting", > + "description": "This boolean setting controls whether the selected tab can be closed with a dblclick." > + }, Please use complete words for anything public facing. "closeTabsByDoubleClick" "can be closed with a double click"
Comment on attachment 8947708 [details] Bug 1435142 - Part 1: pref to enable close selected tab by dblclicking it. https://reviewboard.mozilla.org/r/217422/#review231598 > Move this to the tabbrowser-tab binding as well? Moved. I also moved `tabbrowser-tabs._firstMouseDownOnSelectedTab` to `tabbrowser-tab._selectedOnFirstMouseDown`. I think it's better not to read the pref for every tab so I kept `tabbrowser-tabs._closeTabByDblclick` where it is. > I don't think event.target.localName == "tab" can ever be false here... Removed.
Comment on attachment 8956371 [details] Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. https://reviewboard.mozilla.org/r/225232/#review231648 The idea of a more generic tab event listener was proposed and denied in bug 1246706. > Please use complete words for anything public facing. > > "closeTabsByDoubleClick" > > "can be closed with a double click" Done.
Comment on attachment 8956371 [details] Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. https://reviewboard.mozilla.org/r/225232/#review232370 ::: toolkit/components/extensions/ext-browserSettings.js:234 (Diff revision 4) > + closeTabsByDoubleClick: getSettingsAPI( > + extension, "closeTabsByDoubleClick", > + () => { > + return Services.prefs.getBoolPref("browser.tabs.closeTabByDblclick"); > + }), I think you probably should address android here. Have you run a try that includes android?
Comment on attachment 8956371 [details] Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. https://reviewboard.mozilla.org/r/225232/#review232370 > I think you probably should address android here. Have you run a try that includes android? My patch doesn't touch any Android code, I believe this pref won't work there. I haven't pushed this to try, but will do that later today. Are you suggesting adding some documentation about this fact? A default value here to prevent `getBoolPref` throwing in Android? Or making some changes to hide this API from Android?
(In reply to Hector Zhao [:hectorz] from comment #26) > > > I think you probably should address android here. Have you run a try that includes android? > > My patch doesn't touch any Android code, I believe this pref won't work > there. I haven't pushed this to try, but will do that later today. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7969e71766756bfec289cb749db02a63a2b1ee72
Comment on attachment 8956371 [details] Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. https://reviewboard.mozilla.org/r/225232/#review232370 > My patch doesn't touch any Android code, I believe this pref won't work there. I haven't pushed this to try, but will do that later today. > > Are you suggesting adding some documentation about this fact? A default value here to prevent `getBoolPref` throwing in Android? Or making some changes to hide this API from Android? This file runs on android. If you look through it, you'll see code to handle things that dont work on android. Since the pref is not defaulted on android, the getter will throw. e.g. see proxyConfig
Comment on attachment 8956371 [details] Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. https://reviewboard.mozilla.org/r/225232/#review232370 > This file runs on android. If you look through it, you'll see code to handle things that dont work on android. Since the pref is not defaulted on android, the getter will throw. e.g. see proxyConfig Updated it to a no-op on Android, like proxyConfig.
Comment on attachment 8956371 [details] Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. https://reviewboard.mozilla.org/r/225232/#review232370 > Updated it to a no-op on Android, like proxyConfig. Sorry, not exactly a no-op, and the tests are skipped on Android, like proxyConfig.
Comment on attachment 8956371 [details] Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. https://reviewboard.mozilla.org/r/225232/#review233510 r+ with assertion test ::: toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js:168 (Diff revisions 4 - 6) > await testSetting( > "contextMenuShowEvent", "mousedown", > {"ui.context_menus.after_mouseup": false}); > } > > + if (AppConstants.platform !== "android") { This is just skipping any test on android now. I'm guessing you should be able to add to test_bad_value, something like await browser.test.assertRejects( browser.browserSettings.closeTabsByDoubleClick.set({value: true}), /closeTabsByDoubleClick is not supported on android/, "closeTabsByDoubleClick.set rejects on Android.");
Attachment #8956371 - Flags: review?(mixedpuppy) → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #35) > > This is just skipping any test on android now. I'm guessing you should be > able to add to test_bad_value, something like Since the original check in `test_bad_value` applies to both Android and other platforms, a separate function named `test_bad_value_android` was added for this. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a49bc18cb64734496d298718d01c339a9d36fe3
Comment on attachment 8947708 [details] Bug 1435142 - Part 1: pref to enable close selected tab by dblclicking it. https://reviewboard.mozilla.org/r/217422/#review234176 ::: browser/base/content/tabbrowser.xml:2012 (Diff revision 6) > + let tabContainer = this.parentNode; > + if (tabContainer._closeTabByDblclick && > + this._selectedOnFirstMouseDown && > + this.selected && > + !this._overPlayingIcon) { > + tabContainer.tabbrowser.removeTab(this, { The tabbrowser property has been removed, please use gBrowser instead
Attachment #8947708 - Flags: review?(dao+bmo) → review+
Comment on attachment 8956370 [details] Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick. https://reviewboard.mozilla.org/r/225230/#review234182 ::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:19 (Diff revision 5) > + let tab = gBrowser.selectedTab; > + > + let promise = BrowserTestUtils.waitForEvent(tab, "dblclick"); > + triggerDblclickOn(tab); > + await promise; > + Assert.notStrictEqual(tab.closing, true, ok(!tab.closing, ...); ::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:20 (Diff revision 5) > + > + let promise = BrowserTestUtils.waitForEvent(tab, "dblclick"); > + triggerDblclickOn(tab); > + await promise; > + Assert.notStrictEqual(tab.closing, true, > + `Double click the selected tab won't close it`); nit: use double quotes ::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:31 (Diff revision 5) > + ]}); > + > + let promise = BrowserTestUtils.waitForNewTab(gBrowser, "about:mozilla"); > + BrowserTestUtils.addTab(gBrowser, "about:mozilla"); > + let tab = await promise; > + Assert.notEqual(tab, gBrowser.selectedTab, isnot(...); ::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:37 (Diff revision 5) > + `The new tab is in the background`); > + > + promise = BrowserTestUtils.waitForEvent(tab, "dblclick"); > + triggerDblclickOn(tab); > + await promise; > + Assert.equal(tab, gBrowser.selectedTab, is(...); ::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:43 (Diff revision 5) > + `Double click a background tab will select it`); > + > + promise = BrowserTestUtils.waitForEvent(tab, "dblclick"); > + triggerDblclickOn(tab); > + await promise; > + Assert.strictEqual(tab.closing, true, ok(tab.closing, ...);
Attachment #8956370 - Flags: review?(dao+bmo) → review+
Comment on attachment 8956370 [details] Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick. https://reviewboard.mozilla.org/r/225230/#review234188 ::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:47 (Diff revision 5) > + await promise; > + Assert.strictEqual(tab.closing, true, > + `Double click the selected tab will close it`); > + > + await BrowserTestUtils.tabRemoved(tab); > + await SpecialPowers.popPrefEnv(); This will happen automatically after the test: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/testing/mochitest/browser-test.js#740
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=92899f59174aecd3a0b2e7e735d073c8f43667b7 Failures of M-e10s(cl) are results of using `./mach try fuzzy` with paths, I've filed bug 1446884 for that.
Keywords: checkin-needed
Attached patch conflict-with-bug-1442465.patch (obsolete) — — Splinter Review
I just saw bug 1442465, not sure if that would be on autoland when my patches land. Not sure what's the right thing to do here, I'm attaching a fix for the conflict as tradtional patch here and flagging :dao for review.
Attachment #8960100 - Flags: review?(dao+bmo)
Comment on attachment 8960100 [details] [diff] [review] conflict-with-bug-1442465.patch Review of attachment 8960100 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Hector Zhao [:hectorz] from comment #45) > > I just saw bug 1442465, not sure if that would be on autoland when my > patches land. > > Not sure what's the right thing to do here, I'm attaching a fix for the > conflict as tradtional patch here and flagging :dao for review. Okay, bug 1442465 was now in autoland, I'll just merge this into part 2.
Attachment #8960100 - Flags: review?(dao+bmo)
Attachment #8960100 - Attachment is obsolete: true
Please see comment 44 for the try push. The patches were rebased onto current m-c to resolve the conflict with bug 1442465.
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/069febd18b9e Part 1: pref to enable close selected tab by dblclicking it. r=dao https://hg.mozilla.org/integration/autoland/rev/e9ddfb08a712 Part 2: add a browser mochitest for closeTabByDblclick. r=dao https://hg.mozilla.org/integration/autoland/rev/bcde232543f8 Part 3: expose closeTabByDblclick to WebExtension. r=mixedpuppy
Keywords: checkin-needed
Oops, sorry, just realized I "can finish without waiting for the closing tabs" with changes in https://hg.mozilla.org/mozilla-central/diff/aa1497ed75e5/testing/mochitest/browser-test.js. Will push the updated commits soon.
Flags: needinfo?(bzhao)
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d83b5fa7d3d96486f491e27778c6354be0f0bea Failures of test-android-4.3-arm7-api-16/opt-test-verify look like bug 1442503.
Keywords: checkin-needed
Pushed by nerli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/79b72c904600 Part 1: pref to enable close selected tab by dblclicking it. r=dao https://hg.mozilla.org/integration/autoland/rev/ff4b0af1e773 Part 2: add a browser mochitest for closeTabByDblclick. r=dao https://hg.mozilla.org/integration/autoland/rev/9193e64cfe29 Part 3: expose closeTabByDblclick to WebExtension. r=mixedpuppy
Keywords: checkin-needed
Comment on attachment 8956370 [details] Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick. https://reviewboard.mozilla.org/r/225230/#review235110 ::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:40 (Diff revisions 7 - 8) > is(tab, gBrowser.selectedTab, "Double click a background tab will select it"); > > promise = BrowserTestUtils.waitForEvent(tab, "dblclick"); > triggerDblclickOn(tab); > await promise; > ok(tab.closing, "Double click the selected tab will close it"); Please wait for the tab to be closed. await BrowserTestUtils.tabRemoved(tab);
Comment on attachment 8956370 [details] Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick. https://reviewboard.mozilla.org/r/225230/#review235114 ::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:8 (Diff revision 8) > +function triggerDblclickOn(target) { > + EventUtils.synthesizeMouseAtCenter(target, { clickCount: 1 }); > + EventUtils.synthesizeMouseAtCenter(target, { clickCount: 2 }); > +} Change to: let promise = BrowserTestUtils.waitForEvent(tab, "dblclick"); EventUtils.synthesizeMouseAtCenter(target, { clickCount: 1 }); EventUtils.synthesizeMouseAtCenter(target, { clickCount: 2 }); return promise;
Comment on attachment 8956370 [details] Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick. https://reviewboard.mozilla.org/r/225230/#review235110 > Please wait for the tab to be closed. > > await BrowserTestUtils.tabRemoved(tab); Thanks for helping me with diagnosing this. But tabRemoved was removed in bug 1442465, and its replacement waitForSessionStoreUpdate caused failures in yesterday’s first attempt.
Comment on attachment 8956370 [details] Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick. https://reviewboard.mozilla.org/r/225230/#review235110 > Thanks for helping me with diagnosing this. But tabRemoved was removed in bug 1442465, and its replacement waitForSessionStoreUpdate caused failures in yesterday’s first attempt. I removed the wait of `tabRemoved` in my second attempt, because with the changes in bug 1442465, mochitest stops complaining about "Found an unexpected tab at the end of test run" as long as `tab.closing` is true. Failures in the second attempt are time outs after "The new tab is in the background". My current guess is the animation for adding a new tab is interfering with subsequent dblclick events and I've managed to eliminate failures of `browser_close_tab_by_dblclick.js` by waiting until `tab._fullyOpen` is set (try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ad202981a61beaae6f68d8d581e27cf624bc419). However there're multiple M-e10s(bc3) failures on Windows (tracked in bug 1397098), I don't see how my patches might make it happen more frequently, but I'm not so sure, especially after two backouts.
Comment on attachment 8956370 [details] Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick. https://reviewboard.mozilla.org/r/225230/#review235474 ::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:26 (Diff revision 9) > + let tab = BrowserTestUtils.addTab(gBrowser, "about:mozilla"); > + await BrowserTestUtils.waitForCondition(() => tab._fullyOpen); waitforcondition should not be necessary, and this is rather hacky. You could try: let tab = await BrowserTestUtils.waitForNewTab(gBrowser, "about:mozilla", true); or maybe: let tab = BrowserTestUtils.addTab(gBrowser, "about:mozilla", { skipAnimation: true }); await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
Attachment #8956370 - Flags: review?(mixedpuppy)
You shouldn't worry about the BC failures from Bug 1397098.
Comment on attachment 8956370 [details] Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick. https://reviewboard.mozilla.org/r/225230/#review235474 > waitforcondition should not be necessary, and this is rather hacky. > > You could try: > > let tab = await BrowserTestUtils.waitForNewTab(gBrowser, "about:mozilla", true); > > or maybe: > > let tab = BrowserTestUtils.addTab(gBrowser, "about:mozilla", { skipAnimation: true }); > await BrowserTestUtils.browserLoaded(tab.linkedBrowser); I assume you said waitForCondition is "hacky" because it's polling. I don't want to use `await BrowserTestUtils.browserLoaded(tab.linkedBrowser)` as that feels like "should wait for some other thing, and waiting for ... was longer enough for the other thing" as discouraged in bug 1442465, and "transionend" turns out to be a more accurate signal of the end of said "animation" I'm concerned.
Try run with patches from earlier today: https://treeherder.mozilla.org/#/jobs?repo=try&revision=433210ed64d404041b10abd92a4119dddfa60d80&group_state=expanded It's part of bc7 on Win 7, and part of bc2 on Win 10 x64.
Flags: needinfo?(bzhao)
(In reply to Hector Zhao [:hectorz] from comment #72) > Comment on attachment 8956370 [details] > Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick. > > https://reviewboard.mozilla.org/r/225230/#review235474 > > > waitforcondition should not be necessary, and this is rather hacky. > > > > You could try: > > > > let tab = await BrowserTestUtils.waitForNewTab(gBrowser, "about:mozilla", true); > > > > or maybe: > > > > let tab = BrowserTestUtils.addTab(gBrowser, "about:mozilla", { skipAnimation: true }); > > await BrowserTestUtils.browserLoaded(tab.linkedBrowser); > > I assume you said waitForCondition is "hacky" because it's polling. I don't > want to use `await BrowserTestUtils.browserLoaded(tab.linkedBrowser)` as > that feels like "should wait for some other thing, and waiting for ... was > longer enough for the other thing" as discouraged in bug 1442465, and > "transionend" turns out to be a more accurate signal of the end of said > "animation" I'm concerned. It was hacky because the polling and because you're digging into the internals which is not necessary because the right functionality is provided in BTU. BrowserTestUtils.browserLoaded handles this just fine, waitForNewTab is better as it does that internally. These are used in tons of tests that need to wait for the tab to be accessible.
(In reply to Shane Caraveo (:mixedpuppy) from comment #74) > > I assume you said waitForCondition is "hacky" because it's polling. I don't > > want to use `await BrowserTestUtils.browserLoaded(tab.linkedBrowser)` as > > that feels like "should wait for some other thing, and waiting for ... was > > longer enough for the other thing" as discouraged in bug 1442465, and > > "transionend" turns out to be a more accurate signal of the end of said > > "animation" I'm concerned. > > It was hacky because the polling and because you're digging into the > internals which is not necessary because the right functionality is provided > in BTU. BrowserTestUtils.browserLoaded handles this just fine, > waitForNewTab is better as it does that internally. These are used in tons > of tests that need to wait for the tab to be accessible. This test shouldn't care about the content being loaded. From what I can tell it should open a tab without the animation and then go on without waiting for anything.
(In reply to DĂŁo Gottwald [::dao] from comment #75) > (In reply to Shane Caraveo (:mixedpuppy) from comment #74) > > > > It was hacky because the polling and because you're digging into the > > internals which is not necessary ... I see. > > This test shouldn't care about the content being loaded. From what I can > tell it should open a tab without the animation and then go on without > waiting for anything. Thanks for the advice.
Comment on attachment 8956370 [details] Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick. https://reviewboard.mozilla.org/r/225230/#review235804 ::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:26 (Diff revision 10) > + let tab = BrowserTestUtils.addTab(gBrowser, "about:mozilla"); > + await BrowserTestUtils.waitForEvent(tab, "transitionend", false, event => { > + return event.propertyName == "max-width"; > + }); Given comments from dao and myself, you should just add the skipAnimation flag and remove any wait after addTab.
Attachment #8956370 - Flags: review?(mixedpuppy) → review+
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=939f89fdbe2f1cacca2557b03c359e2525918a0e&group_state=expanded Failure of TV on Android is bug 1442503; Failures of M-e10s(bc1) on Win 7 and M-e10s(bc7) on Win 10 x64 are bug 1397098.
Keywords: checkin-needed
(In reply to Hector Zhao [:hectorz] from comment #81) > Try: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=939f89fdbe2f1cacca2557b03c359e2525918a0e&group_state=e > xpanded Retrigger of build-signing-win32-pgo/opt doesn't seem to work properly, so I did another try push for that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e14d13f842ba4594443b40157ec5a8fe63e728dd&group_state=expanded
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1505855fc858 Part 1: pref to enable close selected tab by dblclicking it. r=dao https://hg.mozilla.org/integration/autoland/rev/cd802f66b7aa Part 2: add a browser mochitest for closeTabByDblclick. r=dao,mixedpuppy https://hg.mozilla.org/integration/autoland/rev/7334da12de35 Part 3: expose closeTabByDblclick to WebExtension. r=mixedpuppy
Keywords: checkin-needed
browser_preloadedBrowser_zoom.js was disabled in bug 1397098 due to failures in it. I don't think the patches here are a cause of that problem.
Flags: needinfo?(nbeleuzu)
(In reply to Narcis Beleuzu [:NarcisB] from comment #84) > Backed out for browser-chrome failures on browser_preloadedBrowser_zoom.js Thanks to :arai's tireless work over the weekend, bug in browser_preloadedBrowser_zoom.js was identified and fixed. :NarcisB, Can you please clarify when a patch would be backed out with failures in a known intermittent test? Thanks.
Flags: needinfo?(bzhao)
Keywords: checkin-needed
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f6615d8ddb5d Part 1: pref to enable close selected tab by dblclicking it. r=dao https://hg.mozilla.org/integration/autoland/rev/46bbc788a3d4 Part 2: add a browser mochitest for closeTabByDblclick. r=dao,mixedpuppy https://hg.mozilla.org/integration/autoland/rev/a27611cba7d7 Part 3: expose closeTabByDblclick to WebExtension. r=mixedpuppy
Keywords: checkin-needed
(In reply to Hector Zhao [:hectorz] from comment #86) > (In reply to Narcis Beleuzu [:NarcisB] from comment #84) > > Backed out for browser-chrome failures on browser_preloadedBrowser_zoom.js > > Thanks to :arai's tireless work over the weekend, bug in > browser_preloadedBrowser_zoom.js was identified and fixed. > > :NarcisB, Can you please clarify when a patch would be backed out with > failures in a known intermittent test? Thanks. I did that because I noticed that the test started to almost-perma fail starting with that push. The fail was on "browser/base/content/test/tabs/" and on cd802f66b7aa there were changes made there. Also, seems that after that backout, the test didn`t fail anymore. https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=dbfc7212166852e040b8acb6c02ef28fcef45e44&fromchange=cbcc7b902237d350cc78ca9b3cd2d04abbf2e7d5&tochange=e4e466004d34b06fec1e4abddb0f537f9e74493c
Flags: needinfo?(nbeleuzu)
(In reply to Narcis Beleuzu [:NarcisB] from comment #88) > > I did that because I noticed that the test started to almost-perma fail > starting with that push. > The fail was on "browser/base/content/test/tabs/" and on cd802f66b7aa there > were changes made there. > Also, seems that after that backout, the test didn`t fail anymore. Even with fix in in bug 1397098, I couldn't see how my patch will make it more likely to fail. But I can understand the rationale behind the backout now, thanks for the explanation.
https://hg.mozilla.org/mozilla-central/rev/f6615d8ddb5d https://hg.mozilla.org/mozilla-central/rev/46bbc788a3d4 https://hg.mozilla.org/mozilla-central/rev/a27611cba7d7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
(In reply to OrangeFactor Robot from comment #91) > > For more details, see: > https://treeherder.mozilla.org/intermittent-failures.html#/ > bugdetails?bug=1435142&startday=2018-03-20&endday=2018-03-26&tree=trunk Looks like this was meant to be bug 1432430
Verified with Nightly 61.0a1 build 20180327220126 on Windows 10 64 bit.
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified
It would be very helpful to hide close button if we enable closeTabByDblclic. Or introduce a different preference for that at least.
Hi Irene, thanks for documenting this! (In reply to Irene Smith from comment #96) > Added page for the property, and created a pull request for BCD > > https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/ > browserSettings/browserSettings.closeTabsByDoubleClick You might want to make it more clear this option only applies to the current selected tab. > > https://github.com/mdn/browser-compat-data/pull/3237 This doens't really work on Fx for Android.
Flags: needinfo?(bzhao)

Please, add another variable to close background tabs with a double click or rename this one as "browser.tabs.closeForegroundTabByDblclick".
Otherwise, it's misleading. I need to close background tabs with a double click, as it was possible before Quantum.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: