13.56 KB, image/png
11.32 KB, image/png
Titles in sidebar aren't cropped and cross borders between tabs (titles are to long in some locales).
2.72 KB, patch
|Details | Diff | Splinter Review|
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)
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]
(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.
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.
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!
Whiteboard: [good first bug][polish-backlog] → [good first bug][polish-backlog][difficulty=easy]
Hi Jan, Who can review this patch? Thanks!
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
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.
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.