Titles in sidebar aren't cropped and cross borders between tabs (titles are to long in some locales)

RESOLVED FIXED in Firefox 44

Status

--
trivial
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: arni2033, Assigned: rlustin)

Tracking

Trunk
Firefox 44

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed)

Details

(Whiteboard: [good first bug][polish-backlog][difficulty=easy])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8652399 [details]
Titles in sidebar aren't cropped and cross borders between tabs.png

STR:   (or just see the screenshot)
1. Install Russian localization from https://www.mozilla.org/en-US/firefox/all/
2. Open Devtools-Inspector
3. Open sidebar in Inspector
4. Reduce width of the sidebar so that All Tabs dropmarker appeared bug you could still see part of Animation tab

Result:       Sidebar tab title "Блоковая модель" (Box model) crosses borders between tabs
Expectations: It should display "Блоковая м...", I guess.

Possible solutions:
A) Add [crop="end"] to <tab>s. That would make titles crop, just like Devtools tabs
B) Add "overflow:hidden;" for ".devtools-sidebar-tabs tabs > tab > .tab-middle"   (see screenshot "Possible solution B.png")

I prefer option (B), because I often change sidebar width, and text-overflow with "..." makes text twitch. The best solution here would be fade-in at the end of tab, as seen in Goggle chrome's tabs (not sure how to say that in English)
(Reporter)

Comment 1

3 years ago
Created attachment 8652400 [details]
Possible solution B.png
Thanks for the report and for listing potential solutions.
I don't see a very big difference between A and B to be honest, and the advantage of A is that it'd be more consistent with how we deal with overflowing tab titles elsewhere in firefox. So I'd recommended this (also because it's a really simple fix).
Whiteboard: [good first bug][polish-backlog]
(Reporter)

Comment 3

3 years ago
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #2)
> I don't see a very big difference between A and B to be honest
I only mentioned B because it displays more characters and doesn't cause text twitching on overflow.
(Reporter)

Comment 4

3 years ago
Btw I think that the same should be applied to tabs in Debugger and Netmonitor (maybe in other places). That shouldn't be much harder. If somebody's going to work on this bug, ask Patrick about that.
Created attachment 8665582 [details] [diff] [review]
Titles in sidebar aren't cropped and cross borders between tabs (titles are to long in some locales).
Hi.

I've just attached a patch for this issue.
I set the attribute crop="end" to every tab added through the ToolSidebar.addTab() method.

Patrick, can you take a look? Thanks!
Flags: needinfo?(pbrosset)
Flags: needinfo?(pbrosset)
Whiteboard: [good first bug][polish-backlog] → [good first bug][polish-backlog][difficulty=easy]
Hi Jan,
Who can review this patch?
Thanks!
Flags: needinfo?(janx)
Hi Raphaël! Thanks a lot for fixing this. I believe you correctly identified Patrick as a good reviewer for your patch, so I'll set the r? flag on him.
Assignee: nobody → raphael
Flags: needinfo?(janx)
Attachment #8665582 - Flags: review?(pbrosset)
See Also: → bug 1209023
Comment on attachment 8665582 [details] [diff] [review]
Titles in sidebar aren't cropped and cross borders between tabs (titles are to long in some locales).

Review of attachment 8665582 [details] [diff] [review]:
-----------------------------------------------------------------

That's a nice fix because it's an sidebar.js which is a shared widget, so that will fix it everywhere it's used.
There's one detail though, this widget can be used with existing tabs, i.e. you don't have to use the widget's addTab method and in that case, that means that crop="end" attribute isn't going to be added. So we must add it where the other <tab>s are defined.

See /devtools/client/netmonitor/netmonitor.xul

This file defines a <tabbox id="event-details-pane"> element that contains a series of <tab> elements.
I believe you should add crop="end" to each one of them, so that we don't only use this bug to fix the problem in the inspector, but also in the netmonitor.

I've filed bug 1209023 to take care of the problem in the debugger, if you're interested in fixing another bug after this one, that's a good candidate.

So, I'm going to cancel the review for this patch, feel free to upload a new one with the fix in the netmonitor xul file, and ping me for review again.
Attachment #8665582 - Flags: review?(pbrosset)
Created attachment 8666978 [details] [diff] [review]
Titles in sidebar aren't cropped and cross borders between tabs (titles are to long in some locales).
Attachment #8665582 - Attachment is obsolete: true
Attachment #8666978 - Flags: review?(pbrosset)
Hi Patrick,
Thanks for your feedback. I've updated my patch accordingly. Can your review it please?
I'll take bug 1209023 to apply this in the debugger.
Comment on attachment 8666978 [details] [diff] [review]
Titles in sidebar aren't cropped and cross borders between tabs (titles are to long in some locales).

Review of attachment 8666978 [details] [diff] [review]:
-----------------------------------------------------------------

I like it. Thanks.
Try push so we make sure no tests fail because of this change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78ee068a1fd2
Attachment #8666978 - Flags: review?(pbrosset) → review+
The try build hasn't completed, but given the nature of the change (css-only) and knowing that the tests have succeeded on some platforms already, we don't need to wait more to check this in.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e82a342d591
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
(Reporter)

Updated

3 years ago
Has STR: --- → yes

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.