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)

defect

Tracking

()

ASSIGNED

People

(Reporter: ehsan.akhgari, Assigned: dao)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

See the discussion around bug 486262 comment 139.
Attached patch patch (deleted) — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8642028 - Flags: review?(ehsan)
I think the code changes are mostly fine, but this needs to get ui-review. Let me upload screenshots...
Attached image Before tab overflow (deleted) —
Attached image After tab overflow (deleted) —
Attachment #8642076 - Flags: ui-review?(shorlander)
Attachment #8642076 - Flags: ui-review?(madhava)
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
We should at least decide what to do here before shipping.
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)
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 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-
Attachment #8642077 - Flags: ui-review?(shorlander)
(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)
Talking to shorlander, we don't think this is a blocker.
No longer blocks: tab-sound-indicator
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.
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.
Blocks: 1217258
Clearing old needinfo request. Please re-request or redirect if it is still pertinent.
Flags: needinfo?(shorlander)
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)
Attachment #8642077 - Flags: ui-review?(madhava)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: