Last Comment Bug 589132 - Tab group names are too big, text needs to be cropped on end, with ellipses for long titles.
: Tab group names are too big, text needs to be cropped on end, with ellipses f...
Status: RESOLVED FIXED
[visual][polish][good first bug]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: unspecified
: x86 Windows 7
: P4 minor
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 586545 589900 590249 594957 642883 671238 (view as bug list)
Depends on: 312156 671243
Blocks: 597792 624936 642891
  Show dependency treegraph
 
Reported: 2010-08-20 05:21 PDT by Maniac Vlad Florin (:vladmaniac)
Modified: 2016-04-12 14:00 PDT (History)
20 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot (26.38 KB, image/png)
2010-08-20 05:21 PDT, Maniac Vlad Florin (:vladmaniac)
no flags Details
v1 (1.02 KB, patch)
2010-09-08 21:13 PDT, Sean Dunn
dietrich: review-
mitcho: feedback+
Details | Diff | Splinter Review
v2 WIP (5.85 KB, patch)
2010-09-16 23:59 PDT, Sean Dunn
no flags Details | Diff | Splinter Review
v3 WIP (19.37 KB, patch)
2010-09-18 01:57 PDT, Sean Dunn
no flags Details | Diff | Splinter Review
v4 WIP (19.44 KB, patch)
2010-09-18 02:43 PDT, Sean Dunn
no flags Details | Diff | Splinter Review
v4 WIP (19.44 KB, patch)
2010-09-18 02:46 PDT, Sean Dunn
azaaza: ui‑review+
ian: feedback+
Details | Diff | Splinter Review
v5 (22.06 KB, patch)
2010-09-21 18:07 PDT, Sean Dunn
dolske: review-
Details | Diff | Splinter Review
v6 (using DIV measurement now) (23.73 KB, patch)
2010-10-04 09:59 PDT, Sean Dunn
no flags Details | Diff | Splinter Review
v7 (with test) (33.64 KB, patch)
2010-10-07 11:14 PDT, Sean Dunn
ian: review-
Details | Diff | Splinter Review
another patch (11.60 KB, patch)
2011-07-16 14:42 PDT, Teddy Ni
no flags Details | Diff | Splinter Review
another patch (12.14 KB, patch)
2011-07-16 23:59 PDT, Teddy Ni
no flags Details | Diff | Splinter Review

Description Maniac Vlad Florin (:vladmaniac) 2010-08-20 05:21:09 PDT
Created attachment 467726 [details]
screenshot

Steps: 
1. Assume you have a set of tabs opened 
2. Go to Tab view 
3. Make a group of tabs and give it a name, preferably a long string of chars. 
(to reproduce this easily) 
4. Close the group 

================================================================================

Name moves over to "x" button which sometimes can be difficult to access due tot this. Selection zone for text field will jump over the selection zone of "x" button. 

Note: screenshot attached
Comment 1 Sean Dunn 2010-09-08 21:13:57 PDT
Created attachment 473386 [details] [diff] [review]
v1

Made a simpler calculation for available space using the left side of the close button, and the left side of the text field.
Comment 2 Michael Yoshitaka Erlewine [:mitcho] 2010-09-10 15:05:05 PDT
Comment on attachment 473386 [details] [diff] [review]
v1

Looks good to me.
Comment 3 juan becerra [:juanb] 2010-09-10 15:10:49 PDT
*** Bug 594957 has been marked as a duplicate of this bug. ***
Comment 4 Michael Yoshitaka Erlewine [:mitcho] 2010-09-10 16:51:49 PDT
*** Bug 586545 has been marked as a duplicate of this bug. ***
Comment 5 Dietrich Ayala (:dietrich) 2010-09-14 01:55:43 PDT
Comment on attachment 473386 [details] [diff] [review]
v1

while testing to see the problem, i also noticed that when the length of the title is greater than the width of the group box, the title is cropped on the *beginning*. instead it should be cropped (with ellipsis) at the end of the title.

please ensure this patch fixes that problem, while you're in this code. also, please write a test (or modify an existing one) to 1) test the fix and 2) ensure subsequent changes do not regress this.
Comment 6 Sean Dunn 2010-09-16 17:13:06 PDT
(In reply to comment #5)
> while testing to see the problem, i also noticed that when the length of the
> title is greater than the width of the group box, the title is cropped on the
> *beginning*. instead it should be cropped (with ellipsis) at the end of the
> title.

I see what you're talking about, but I didn't consider these issues to be part of the original bug (which only dealt with textbox size). I'll add those behaviors to this bug.
Comment 7 Sean Dunn 2010-09-16 23:59:02 PDT
Created attachment 476190 [details] [diff] [review]
v2 WIP

* Properly calculating text size
* Title text input follows text size more smoothly
* Needs: ellipses for long titles, code cleanup
Comment 8 Aza Raskin [:aza] 2010-09-17 16:24:57 PDT
*** Bug 589900 has been marked as a duplicate of this bug. ***
Comment 9 Sean Dunn 2010-09-18 01:57:33 PDT
Created attachment 476502 [details] [diff] [review]
v3 WIP

* Added ellipses for long titles, which was a large change. Firefox does not have overflow:ellipses support, nor can one change the .innerHTML of a form text input to change the way the input looks (shortened with ellipses). The solution was to do clever show()/hide()ing of a div containing the abridged title, and the input field.
* Pointer over title switched from text to hand pointer.
Comment 10 Sean Dunn 2010-09-18 02:43:06 PDT
Created attachment 476508 [details] [diff] [review]
v4 WIP
Comment 11 Sean Dunn 2010-09-18 02:46:06 PDT
Created attachment 476510 [details] [diff] [review]
v4 WIP

* CSS fixed for Win and OSX
* Titles resize with window correctly
Comment 12 Aza Raskin [:aza] 2010-09-18 17:41:30 PDT
This looks and acts fantastic, Sean!
Comment 13 Aza Raskin [:aza] 2010-09-18 17:45:01 PDT
*** Bug 590249 has been marked as a duplicate of this bug. ***
Comment 14 Ian Gilman [:iangilman] 2010-09-20 14:11:23 PDT
Comment on attachment 476510 [details] [diff] [review]
v4 WIP

The approach looks good. Most of my feedback is nitpicks, but you'll get them from the reviewer if I don't give them to you now.

+  getTitleLabel: function GroupItem_getTitleLabel() {
+      let labelVal = this.$nameInput.val();

You've got 4 spaces of indent here; should be 2.

+      if ( fullWidth > labelWidth) {

Space after left paren.

+  // ----------
+  // Function: getElementFont
+  // Returns the serialized font description.
+  getElementFont : function Utils_getElementFont(elem) {
+    let font = "";
+    font = elem.css('font-style') + " " +

While this routine is called getElementFont and the argument is called elem, it appears to actually be expecting an iQ object. For greater flexibility I'd recommend actually taking in a DOM element and converting it to iQ internally: 

  let $elem = iQ(elem);
  
In fact, with that in place, you can pass either a DOM element or an iQ object and it'll do the right thing. Please also update the documentation comment to be explicit about this. 

+  // ----------
+  // Function: getTextWidth
+  // Returns the number of pixels required to fit the width of the input text.
+  getTextWidth: function Utils_getTextWidth(document, text, font) {
+    // create a canvas object, draw text in object, get extents.
+    let canvas = document.createElement('canvas');
+    let ctx = canvas.getContext('2d');
+    ctx.save();
+    ctx.font = font;
+    // Calculate the text plus a space, since an exact fit is not enough
+    let metrics = ctx.measureText(text);
+    ctx.restore();
+    return metrics.width;
+  },

I wonder what the overhead is for creating a new canvas everytime (especially considering that truncateStringToWidth calls this routine repeatedly). Perhaps it would be better to create the canvas once (or once per document if necessary) and cache it? Then we'd probably have to do some clean up when the document unloads to keep from leaking. 

If we don't end up caching the canvas and just use them once per, you can kill the ctx.save/restore. 

Any thoughts about a test?
Comment 15 Sean Dunn 2010-09-21 18:07:35 PDT
Created attachment 477368 [details] [diff] [review]
v5
Comment 16 Ian Gilman [:iangilman] 2010-09-24 14:25:47 PDT
Comment on attachment 477368 [details] [diff] [review]
v5

Reassigning review to dolske while dietrich is jammed up with b7 stuff.
Comment 17 Justin Dolske [:Dolske] 2010-09-29 16:43:20 PDT
Comment on attachment 477368 [details] [diff] [review]
v5

Sigh. If only we had a fix for bug 312156.

Anyway, r- for a few things that jumped out from a first-pass skim...

>+      let ellipses = "...";

That's not an ellipses. It's 3 periods. :-) You want "\u2026".

And, perhaps surprisingly, this also needs localized. There's an existing pref you can use (intl.ellipsis)... See toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js#283 for an example.

>+  getTextWidth: function Utils_getTextWidth(canvas, text, font) {
>+    // draw text into canvas object, get extents.
>+    let ctx = canvas.getContext('2d');

Sigh. This is clever, but also a hack. :/

How about setting the text the usual way, and use getComputedStyle() to check if it's width is larger than you want? (Might need some cleverness to suppress line wrapping, or to watch for height increases).

Also needs the aforementioned test. Should be sufficient (wrt to overflow) to basically set a long title (that will surely overflow), and then check that the final displayed value is the expected truncated string, +/- some slop to allow for platform variances.
Comment 18 Justin Dolske [:Dolske] 2010-09-30 15:02:11 PDT
Oh, hey, is the tabview iframe privileged? I think it is... That would let you solve this fairly trivially by using <xul:description crop="end"/>. I think that should work in this case (given that we generally disabled unprivileged XUL). That would avoid any need to calculate text width, just style the max-width with CSS and let XUL handle the overflow.
Comment 19 Sean Dunn 2010-10-04 09:59:39 PDT
Created attachment 480650 [details] [diff] [review]
v6 (using DIV measurement now)

After trying to make XUL description work (I can't make it show up and function as a DIV), and then trying to temporarily change the CSS state of the label to measure it (this gets messy and I don't think it's a good pattern), I've settled on creating a special DIV that is solely used for measuring text, offscreen.
Comment 20 Sean Dunn 2010-10-07 11:14:50 PDT
Created attachment 481571 [details] [diff] [review]
v7 (with test)

In addition to it using a DIV measurement technique, I added a test and fixed a previously unseen issue where the title would hang off the group during animated resizing.
Comment 21 Ian Gilman [:iangilman] 2010-10-13 14:04:58 PDT
Comment on attachment 481571 [details] [diff] [review]
v7 (with test)

Switching review to dolske, as dietrich says he's not reviewing non-blockers for the time being.
Comment 22 Ian Gilman [:iangilman] 2010-12-07 14:19:18 PST
Comment on attachment 481571 [details] [diff] [review]
v7 (with test)

Load balancing the review to me.
Comment 23 Ian Gilman [:iangilman] 2010-12-08 10:58:26 PST
Comment on attachment 481571 [details] [diff] [review]
v7 (with test)

Sorry this has been languishing! It all basically looks good, but I'm concerned about the perf implications of adding the truncation check for every setBounds() call. At the very least, we could limit it to just setBounds calls that change the width, so it won't affect dragging speed. Beyond that we could not do it for every setBounds of a drag resize (presumably supressing it with an option) and then call it at the end. What do you think?

Beyond that, a couple of nits while you're in there:

>+  availableTitleWidth : function GroupItem_availableTitleWidth() {
>+    return this.$close.bounds().left - this.$nameContainer.bounds().left;
>+    //return this.$nameContainer.bounds().width;
>+  },

Remove the commented out code.

>+    let optionsComplete;
>+    if (typeof options.complete == "function") {
>+      optionsComplete = options.complete;
>+    } else {
>+      optionsComplete = function(){};
>+    }

Document this new option in the comment block at the top of the routine.
Comment 24 Kevin Hanes 2010-12-10 11:42:13 PST
Moving to b9
Comment 25 Kevin Hanes 2011-01-10 10:06:39 PST
bugspam (moving b9 to b10)
Comment 26 Kevin Hanes 2011-01-10 10:09:18 PST
bugspam (removing b9)
Comment 27 Michael Yoshitaka Erlewine [:mitcho] 2011-01-15 20:39:34 PST
Sean, are you still working on this?
Comment 28 Sean Dunn 2011-01-15 20:42:01 PST
Yes, but it's sadly fallen behind priority on a few other things. It should be ready to go once I add Ian's changes and unrot everything.
Comment 29 Michael Yoshitaka Erlewine [:mitcho] 2011-01-15 20:55:19 PST
(In reply to comment #28)
> Yes, but it's sadly fallen behind priority on a few other things. It should be
> ready to go once I add Ian's changes and unrot everything.

No problem. Might be a lot of rot, though! ;)
Comment 30 Kevin Hanes 2011-01-21 14:11:06 PST
bugspam. Moving b10 to b11
Comment 31 Kevin Hanes 2011-01-21 14:12:43 PST
bugspam. Removing b10
Comment 32 Michael Yoshitaka Erlewine [:mitcho] 2011-02-06 12:18:05 PST
Sean, ping again. :) While not high priority, it may solve both this immediate bug and bug 624936, and would thus be nice to have. If you'd like someone else to try unrotting and taking it the last mile, let us know.
Comment 33 Sean Dunn 2011-02-07 08:45:54 PST
Mitcho, I'll get this unrotted this afternoon. It's pretty gnarly.
Comment 34 Michael Yoshitaka Erlewine [:mitcho] 2011-02-09 22:22:02 PST
With bug 624936, being a quick CSS fix, being more likely at this point to actually land for fx4, I'm much less concerned about this bug. I suggest formally punting it.
Comment 35 Michael Yoshitaka Erlewine [:mitcho] 2011-02-09 22:48:59 PST
[bugspam: mitcho's late-night betaN triage; feel free to comment or reverse]
Comment 36 Ian Gilman [:iangilman] 2011-03-03 16:06:26 PST
bugspam: returning Sean's bugs to the pool
Comment 37 Kevin Hanes 2011-03-25 13:29:07 PDT
*** Bug 642883 has been marked as a duplicate of this bug. ***
Comment 38 Kevin Hanes 2011-03-31 10:51:19 PDT
bugspam
Comment 39 Patrick Walton (:pcwalton) 2011-04-28 14:24:18 PDT
Bug 312156 will implement text-overflow, and it's scheduled for Fx6.
Comment 40 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-05-27 02:21:28 PDT
bugspam
Comment 41 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-05-27 02:26:35 PDT
bugspam
Comment 42 Justin Dolske [:Dolske] 2011-06-26 22:41:46 PDT
This should be trivial to fix now that bug 312156 is fixed.
Comment 43 Dão Gottwald [:dao] 2011-06-26 23:42:11 PDT

*** This bug has been marked as a duplicate of bug 666842 ***
Comment 44 Patrick Walton (:pcwalton) 2011-06-27 09:13:18 PDT
This isn't a dupe, I don't think; this bug is for tab set names, while bug 666842 is for the tab item names.
Comment 45 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-13 10:03:22 PDT
*** Bug 671238 has been marked as a duplicate of this bug. ***
Comment 46 Teddy Ni 2011-07-16 14:42:05 PDT
Created attachment 546352 [details] [diff] [review]
another patch

This also includes some of the changes I made in my patch to 671243, which I've asked Tim to review. Don't know who wants to review this, so I chose Ian since he did the last one.

Due to text-overflow ellipsis landing, this uses that and tries to be as simple as possible. As a result, let me know if there is any bad behavior, which I can try to correct.

Next up is to add some tests, but I have very little experience with writing and running tests. This means anyone who wants to should submit tests. Otherwise, I will look through Sean's test and try to mimic that.

Thought I would get this out here first for comments and suggestions.
Comment 47 Teddy Ni 2011-07-16 23:59:30 PDT
Created attachment 546391 [details] [diff] [review]
another patch

Sorry for spam. Slightly updated code to keep the input box expansion less jittery with a hardcoded right padding. Don't know if there is a more elegant solution.
Comment 48 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-17 07:53:37 PDT
Comment on attachment 546391 [details] [diff] [review]
another patch

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

Thanks for your patch! Alas, I think this is a little bit too complicated. With the new patch for bug 671243 this should be as easy as adding

input.name { text-overflow: ellipsis; }

to browser/base/content/tabview/tabview.css.
Comment 49 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-24 18:37:34 PDT
bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
Comment 50 Alfred Kayser 2011-07-25 07:41:12 PDT
Indeed, only 'text-overflow: ellipsis' is needed, and one could combine that with bug 671243.
Comment 51 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-10-11 02:42:14 PDT
Fixed by bug 671243.

Note You need to log in before you can comment on or make changes to this bug.