589132 - Tab group names are too big, text needs to be cropped on end, with ellipses for long titles.

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P4)

Description

[Maniac Vlad Florin (:vladmaniac)]

Attachment: 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]

Attachment: 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]]

Comment on attachment 473386 [details] [diff] [review]
v1

Looks good to me.

Comment 5

[Dietrich Ayala (:dietrich)]

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]

(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]

Attachment: v2 WIP

* Properly calculating text size
* Title text input follows text size more smoothly
* Needs: ellipses for long titles, code cleanup

Comment 9

[Sean Dunn]

Attachment: 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]

Attachment: v4 WIP

Comment 11

[Sean Dunn]

Attachment: v4 WIP

* CSS fixed for Win and OSX
* Titles resize with window correctly

Comment 12

[Aza Raskin [:aza]]

This looks and acts fantastic, Sean!

Comment 14

[Ian Gilman [:iangilman]]

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]

Attachment: v5

Comment 16

[Ian Gilman [:iangilman]]

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]]

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]]

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]

Attachment: 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]

Attachment: 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]]

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]]

Comment on attachment 481571 [details] [diff] [review]
v7 (with test)

Load balancing the review to me.

Comment 23

[Ian Gilman [:iangilman]]

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]

Moving to b9

Comment 25

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 26

[Kevin Hanes]

bugspam (removing b9)

Comment 27

[Michael Yoshitaka Erlewine [:mitcho]]

Sean, are you still working on this?

Comment 28

[Sean Dunn]

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]]

(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]

bugspam. Moving b10 to b11

Comment 31

[Kevin Hanes]

bugspam. Removing b10

Comment 32

[Michael Yoshitaka Erlewine [:mitcho]]

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]

Mitcho, I'll get this unrotted this afternoon. It's pretty gnarly.

Comment 34

[Michael Yoshitaka Erlewine [:mitcho]]

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]]

[bugspam: mitcho's late-night betaN triage; feel free to comment or reverse]

Comment 36

[Ian Gilman [:iangilman]]

bugspam: returning Sean's bugs to the pool

Comment 38

[Kevin Hanes]

bugspam

Comment 39

[Patrick Walton (:pcwalton)]

Bug 312156 will implement text-overflow, and it's scheduled for Fx6.

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 41

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 42

[Justin Dolske [:Dolske]]

This should be trivial to fix now that bug 312156 is fixed.

Comment 44

[Patrick Walton (:pcwalton)]

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 46

[Teddy Ni]

Attachment: 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]

Attachment: 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] (inactive)]

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] (inactive)]

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]

Indeed, only 'text-overflow: ellipsis' is needed, and one could combine that with bug 671243.

Comment 51

[Tim Taubert [:ttaubert] (inactive)]

Fixed by bug 671243.