1406478 - Set the default of `browser.tabs.tabMinWidth` to 76.

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

+++ This bug was initially created as a clone of Bug #1404465 +++

If you want to debate the default setting, please don't do that in this bug or bug 1404465. Please do that in this dev-platform thread instead: https://groups.google.com/forum/#!topic/mozilla.dev.platform/4RAQOAMT9PI

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Request to do this was made by canuckistani and signed off by shorlander - see bug 1404465 comment 47.

Comment 2

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from comment #1)
> Request to do this was made by canuckistani and signed off by shorlander -
> see bug 1404465 comment 47.

It's not gonna fly, see bug 1404465 comment 59.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Given comment 2, how should I proceed?

Comment 4

[Tim Nguyen :ntim]

While 70 allows me to see more tabs at once, it's not necessarily helpful when you have multiple similar tabs open. I often open up multiple gmail tabs with different accounts, and what I end up seeing is "Inbox", "Inbox", "Inbox"... which is not very helpful. I increased the width to 90, and I could start to see the inbox count, which was more helpful.

I guess this is an use case that could be covered by container tabs, but unfortunately, container tabs don't have great integration with bookmarks and the awesomebar (would be nice to have suggestions like: Gmail in Work container"), and automatically opening URIs in containers isn't helpful either, because Gmail has the same URI across containers.

Another consequence of decreasing the max tab width for me was that I had more duplicate tabs opened, and discouraged me to clean up my tabs periodically.


A min-width of 90 (lower than the previous 100px), works well for my purpose.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Thanks ntim.

Note that I _really_ encourage this sort of conversation to happen in the dev-platform thread at https://groups.google.com/forum/#!topic/mozilla.dev.platform/4RAQOAMT9PI rather than here. I don't really want this bug to spiral out into discussion and debate like bug 1404465.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1406478 - Set the browser.tabs.tabMinWidth default to 70 (but allow it to go as low as 50).

Review commit: https://reviewboard.mozilla.org/r/187432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187432/

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Haven't heard back from shorlander or canuckistani here, but I've posted the patch for review anyway. Pretty easy to change the number if they come back with something different, I guess.

I'm away on Monday due to a holiday, but anyone should feel free to land (or steal, correct and land) if the desired number comes in.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: 1406478-for-beta.diff

Here's this patch _and_ the patch for bug 1404465 folded and re-based for mozilla-beta, in the event that this needs to be landed on Monday and I'm not around due to the holiday.

Again, please feel free to steal, modify, request uplift, and land in the event that different numbers come down.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8916180 [details]
Bug 1406478 - Set the browser.tabs.tabMinWidth default to 70 (but allow it to go as low as 50).

https://reviewboard.mozilla.org/r/187432/#review192524

::: browser/app/profile/firefox.js:491
(Diff revision 1)
>  pref("browser.tabs.showAudioPlayingIcon", true);
>  // This should match Chromium's audio indicator delay.
>  pref("browser.tabs.delayHidingAudioPlayingIconMS", 3000);
>  
>  // The minimum tab width in pixels
> -pref("browser.tabs.tabMinWidth", 50);
> +pref("browser.tabs.tabMinWidth", 70);

In touch mode, a selected tab with a favicon and the sound playing icon seems to consume 82 pixels in my testing on Ubuntu (but this should be consistent across platforms). That would be the lowest value we can set without other modifications to the tab layout.

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from comment #8)
> Created attachment 8916190 [details] [diff] [review]
> 1406478-for-beta.diff
> 
> Here's this patch _and_ the patch for bug 1404465 folded and re-based for
> mozilla-beta, in the event that this needs to be landed on Monday and I'm
> not around due to the holiday.
> 
> Again, please feel free to steal, modify, request uplift, and land in the
> event that different numbers come down.

We're good to go for a default setting of 75, I've adjusted the description to reflect this.

Comment 11

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Mike: this can wait until Tuesday but no later. I'll circle back around with you on Tuesday morning.

Comment 12

[Oriol Brufau [:Oriol]]

Aren't comment 9 and comment 10 contradictory?

> 82 pixels [...] would be the lowest value we can set

> We're good to go for a default setting of 75

Comment 13

[Dão Gottwald [:dao]]

(In reply to Oriol Brufau [:Oriol] from comment #12)
> Aren't comment 9 and comment 10 contradictory?
> 
> > 82 pixels [...] would be the lowest value we can set
> 
> > We're good to go for a default setting of 75

75 works with compact and normal UI density (I think) but for touch we'll need more space.

Comment 14

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #13)
> (In reply to Oriol Brufau [:Oriol] from comment #12)
> > Aren't comment 9 and comment 10 contradictory?
> > 
> > > 82 pixels [...] would be the lowest value we can set
> > 
> > > We're good to go for a default setting of 75
> 
> 75 works with compact and normal UI density (I think)

Hm, no, it doesn't. Bug 1391539 made the separators consume another pixel, so now 76 seems to be the minimum...

Comment 15

[Dão Gottwald [:dao]]

Attachment: Bug 1406478 - Set browser.tabs.tabMinWidth to 76.

Review commit: https://reviewboard.mozilla.org/r/187534/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187534/

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8916367 [details]
Bug 1406478 - Set browser.tabs.tabMinWidth to 76.

https://reviewboard.mozilla.org/r/187534/#review192740

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4fc441d0663
Set browser.tabs.tabMinWidth to 76. r=jaws

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser-chrome's browser/base/content/test/general/browser_tabReorder.js:

https://hg.mozilla.org/integration/autoland/rev/ea295817d104eda10909e6155369019cb7e0887a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a4fc441d0663d647e0f734420bc1c1eff7951434&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135725193&repo=autoland

[task 2017-10-09T16:36:32.057Z] 16:36:32     INFO - TEST-START | browser/base/content/test/general/browser_tabReorder.js
[task 2017-10-09T16:36:37.974Z] 16:36:37     INFO - TEST-INFO | started process screentopng
[task 2017-10-09T16:36:39.481Z] 16:36:39     INFO - TEST-INFO | screentopng: exit 0
[task 2017-10-09T16:36:39.485Z] 16:36:39     INFO - Buffered messages logged at 16:36:32
[task 2017-10-09T16:36:39.485Z] 16:36:39     INFO - Entering test bound 
[task 2017-10-09T16:36:39.485Z] 16:36:39     INFO - TEST-PASS | browser/base/content/test/general/browser_tabReorder.js | new tabs are opened - 
[task 2017-10-09T16:36:39.488Z] 16:36:39     INFO - TEST-PASS | browser/base/content/test/general/browser_tabReorder.js | newTab1 position is correct - 
[task 2017-10-09T16:36:39.490Z] 16:36:39     INFO - TEST-PASS | browser/base/content/test/general/browser_tabReorder.js | newTab2 position is correct - 
[task 2017-10-09T16:36:39.498Z] 16:36:39     INFO - TEST-PASS | browser/base/content/test/general/browser_tabReorder.js | newTab3 position is correct - 
[task 2017-10-09T16:36:39.500Z] 16:36:39     INFO - Buffered messages finished
[task 2017-10-09T16:36:39.503Z] 16:36:39     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_tabReorder.js | Uncaught exception - Waiting for tab position to be updated - timed out after 50 tries.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8916367 [details]
Bug 1406478 - Set browser.tabs.tabMinWidth to 76.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187534/diff/1-2/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d91a88f5f5d
Set browser.tabs.tabMinWidth to 76. r=jaws

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2d91a88f5f5d

Comment 22

[Jim Jeffery not reading bug-mail 1/2/11]

Title of bug is misleading in that the final size was set to 76, not 75

Comment 23

[Clément Lefèvre]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #22)
> Title of bug is misleading in that the final size was set to 76, not 75

Changed it to reflect commit's change.

Comment 24

[Dão Gottwald [:dao]]

Attachment: patch for uplift

Comment 25

[Mike Conley (:mconley) (:⚙️)]

It looks like this most recent patch still makes the title invisible in touch mode with a favicon and tab mute icon... what was the plan there?

Comment 26

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from comment #25)
> It looks like this most recent patch still makes the title invisible in
> touch mode with a favicon and tab mute icon... what was the plan there?

There's no plan that I know of :(
I just made it so that the default tab minimum width doesn't undercut the tab contents' minimum width. Is this not working on your end?

Comment 27

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Attachment: Screen Shot 2017-10-10 at 11.58.08 AM.png

Stephen, this is what it looks like as per Mike's comment. Is this acceptable?

Comment 28

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from comment #25)
> It looks like this most recent patch still makes the title invisible in
> touch mode with a favicon and tab mute icon... what was the plan there?

(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #27)
> Created attachment 8917026 [details]
> Screen Shot 2017-10-10 at 11.58.08 AM.png
> 
> Stephen, this is what it looks like as per Mike's comment. Is this
> acceptable?

Note that this isn't limited to touch mode.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8916905 [details] [diff] [review]
patch for uplift

Approval Request Comment
[Feature/Bug causing the regression]:

None. This was something requested by mayo / canuckistani for 57.

[User impact if declined]:

Users are more likely to enter the "overflow" state for the browser tab strip, which is something we want to avoid when possible.

[Is this code covered by automated tests?]:

Yes.

[Has the fix been verified in Nightly?]:

Yes, I have manually verified this change.

[Needs manual test from QE? If yes, steps to reproduce]: 

No.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

As far as I can tell, this is only risky for one reason (see below).

[Why is the change risky/not risky?]:

The only risk that we've identified is that in Touch Mode (browser.uidensity = 2), at least on my MBP, tabs that have both a favicon and the media mute icons (so, for example, a YouTube video playing music) hide the tab title. I'm waiting for confirmation on whether or not this is acceptable.

[String changes made/needed]:

None.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #28)
> (In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from
> comment #25)
> > It looks like this most recent patch still makes the title invisible in
> > touch mode with a favicon and tab mute icon... what was the plan there?
> 
> (In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #27)
> > Created attachment 8917026 [details]
> > Screen Shot 2017-10-10 at 11.58.08 AM.png
> > 
> > Stephen, this is what it looks like as per Mike's comment. Is this
> > acceptable?
> 
> Note that this isn't limited to touch mode.

Yes, you're right - this seems to affect normal and compact mode as well.

So all modes.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

canuckistani says we're still a go for 76px min tab width for 57.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916905 [details] [diff] [review]
patch for uplift

must fix, beta57+

Comment 33

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-beta/rev/22a065a6a5fe7f01fdda155e74969d85ac0fa6bc

Comment 35

[Andrei Vaida [:avaida]]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from comment #29)
> [Is this code covered by automated tests?]:
> 
> Yes.
> 
> [Has the fix been verified in Nightly?]:
> 
> Yes, I have manually verified this change.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: 
> 
> No.

Setting qe-verify- based on Mike's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 36

[Sajedul Islam]

I have reproduced this bug with Nightly 58.0a1 (2017-10-06) on Windows 10 , 64 Bit !

This bug's fix is Verified with latest Beta & Nightly !

Build   ID    20171023180840
User Agent    Mozilla/5.0 (Windows NT 10.0; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

Build   ID    20171025100449
User Agent    Mozilla/5.0 (Windows NT 10.0; WOW64; rv:58.0) Gecko/20100101 Firefox/58.0

[bugday-20171025]