Open
Bug 1186632
Opened 9 years ago
Updated 2 years ago
Use the overlay sound icon when the width of the tab drops below 140px
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
ASSIGNED
People
(Reporter: ehsan.akhgari, Assigned: dao)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
shorlander
:
ui-review-
|
Details |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details |
See the discussion around bug 486262 comment 139.
Assignee | ||
Comment 1•9 years ago
|
||
Reporter | ||
Comment 2•9 years ago
|
||
I think the code changes are mostly fine, but this needs to get ui-review. Let me upload screenshots...
Reporter | ||
Comment 3•9 years ago
|
||
Reporter | ||
Comment 4•9 years ago
|
||
Attachment #8642076 -
Flags: ui-review?(shorlander)
Attachment #8642076 -
Flags: ui-review?(madhava)
Reporter | ||
Updated•9 years ago
|
Summary: Figure out if we want to handle tabs with minimum width that have a playing icon differently → Use the overlay sound icon when the width of the tab drops below 140px
Reporter | ||
Comment 5•9 years ago
|
||
We should at least decide what to do here before shipping.
Blocks: tab-sound-indicator
Reporter | ||
Comment 6•9 years ago
|
||
Hmm, this looks weird to me. In this case, when the tab bar is not overflown, we display no tab icon, and then the tab suddenly gets a blank icon...
Maintaining the icon at the right side in this case doesn't seem to waste any more space, but this is of course more consistent with what happens when the tab does have a favicon, which is the majority of the cases.
Up to the UX team...
Attachment #8642077 -
Flags: ui-review?(shorlander)
Attachment #8642077 -
Flags: ui-review?(madhava)
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8642028 [details] [diff] [review]
patch
Review of attachment 8642028 [details] [diff] [review]:
-----------------------------------------------------------------
I think given that this code is purely changing the presentation, the existing browser_audioTabIcon.js should be enough, but if you think it's worth it, feel free to extend that test to cover the tab bar overflow scenario as well. We already execute the test "recursively" to test everything for the foreground and background tab case here: https://dxr.allizom.org/mozilla-central/source/browser/base/content/test/general/browser_audioTabIcon.js?offset=0#110. A similar recursion can be used to test the tabbar overflown case.
::: browser/base/content/tabbrowser.css
@@ +19,5 @@
> }
>
> .tab-close-button[pinned],
> +.tabbrowser-tabs[cliptabcontent] > * > * > * > .tab-close-button:not([visuallyselected="true"]),
> +.tab-icon-sound:not([soundplaying]),
Please incorporate bug 1190081 into this since I suspect that bug to land first.
@@ +31,5 @@
>
> +.tab-icon-image[src],
> +.tab-icon-image[pinned],
> +.tab-icon-image[crashed],
> +.tabbrowser-tabs[cliptabcontent] > * > * > * > .tab-icon-image[soundplaying],
Depending on the UI review for attachment 8642077 [details], this may need to change...
Attachment #8642028 -
Flags: review?(ehsan) → review+
Comment 8•9 years ago
|
||
Comment on attachment 8642076 [details]
After tab overflow
I don't think we should do this.
We would be trading consistency and hit target size for a few extra characters in a few cases.
Attachment #8642076 -
Flags: ui-review?(shorlander) → ui-review-
Updated•9 years ago
|
Attachment #8642077 -
Flags: ui-review?(shorlander)
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Stephen Horlander [:shorlander] from comment #8)
> Comment on attachment 8642076 [details]
> After tab overflow
>
> I don't think we should do this.
>
> We would be trading consistency
We're already moving the indicator this for pinned tabs. This is merely changing where we draw the line. And then we're also similarly trading consistency with the close buttons.
> and hit target size for a few extra characters
Just like we're doing for the close buttons, right? Although this case doesn't affect all tabs (only those playing sound), it doesn't really seem different in principle.
As noted before, we display about three (selected tab) or five (background tabs) characters when reaching the tab minimum width (100px). Removing another three characters from there is significant.
> in a few cases.
I'm not quite sure what you mean here. Tabs that won't have their titles cropped below the tab clip width seem rather rare, so it seems that we'd get extra characters for most cases, although the relative gain of those characters becomes greater the close we get to the minimum width. (Which again means that we need to draw a line somewhere, and 140 seems to have worked alright over the years.)
Flags: needinfo?(shorlander)
Assignee | ||
Comment 10•9 years ago
|
||
Reporter | ||
Comment 11•9 years ago
|
||
Talking to shorlander, we don't think this is a blocker.
No longer blocks: tab-sound-indicator
Comment 13•9 years ago
|
||
As you may know, I am providing complete themes (Qute5++ and Qute6++). So I have modified UI design and released. Please refer to attached image.
Reporter | ||
Comment 14•9 years ago
|
||
Looks nice!
Note that in your theme, the minimum width of the text in a tab seems to be larger than the default theme, which makes the issue less visible there.
Comment 16•8 years ago
|
||
Clearing old needinfo request. Please re-request or redirect if it is still pertinent.
Flags: needinfo?(shorlander)
Comment 17•8 years ago
|
||
Comment on attachment 8642076 [details]
After tab overflow
Clearing old requests. Please re-request if this is still wanted.
Attachment #8642076 -
Flags: ui-review?(madhava)
Updated•8 years ago
|
Attachment #8642077 -
Flags: ui-review?(madhava)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•