Closed Bug 589132 Opened 14 years ago Closed 13 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, P4)

x86
Windows 7

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: vladmaniac, Unassigned)

References

Details

(Whiteboard: [visual][polish][good first bug])

Attachments

(3 files, 8 obsolete files)

Attached image 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
Summary: [FF4][Tab Candy] Tab set names move over the "x" dismiss button → Tab set names move over the "x" dismiss button
Assignee: nobody → seanedunn
Attached patch v1 (obsolete) — Splinter Review
Made a simpler calculation for available space using the left side of the close button, and the left side of the text field.
Attachment #473386 - Flags: feedback?(ian)
Attachment #473386 - Flags: feedback?(ian) → feedback?(mitcho)
Comment on attachment 473386 [details] [diff] [review]
v1

Looks good to me.
Attachment #473386 - Flags: feedback?(mitcho) → feedback+
Attachment #473386 - Flags: review?(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.
Attachment #473386 - Flags: review?(dietrich) → review-
Priority: -- → P3
(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.
Summary: Tab set names move over the "x" dismiss button → Tab set names are too big, text needs to be cropped on end, with ellipses for long titles.
Attached patch v2 WIP (obsolete) — Splinter Review
* Properly calculating text size
* Title text input follows text size more smoothly
* Needs: ellipses for long titles, code cleanup
Attachment #473386 - Attachment is obsolete: true
Attached patch v3 WIP (obsolete) — Splinter Review
* 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.
Attachment #476190 - Attachment is obsolete: true
Attached patch v4 WIP (obsolete) — Splinter Review
Attachment #476508 - Flags: ui-review?
Attached patch v4 WIP (obsolete) — Splinter Review
* CSS fixed for Win and OSX
* Titles resize with window correctly
Attachment #476502 - Attachment is obsolete: true
Attachment #476508 - Attachment is obsolete: true
Attachment #476510 - Flags: ui-review?
Attachment #476508 - Flags: ui-review?
Attachment #476510 - Flags: ui-review? → ui-review?(aza)
Blocks: 597043
Attachment #476510 - Flags: ui-review?(aza)
Attachment #476510 - Flags: ui-review+
Attachment #476510 - Flags: feedback?(ian)
This looks and acts fantastic, Sean!
Priority: P3 → P2
Target Milestone: --- → Firefox 4.0
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?
Attachment #476510 - Flags: feedback?(ian) → feedback+
Attached patch v5 (obsolete) — Splinter Review
Attachment #476510 - Attachment is obsolete: true
Attachment #477368 - Flags: review?(dietrich)
Comment on attachment 477368 [details] [diff] [review]
v5

Reassigning review to dolske while dietrich is jammed up with b7 stuff.
Attachment #477368 - Flags: review?(dietrich) → review?(dolske)
Status: NEW → ASSIGNED
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.
Attachment #477368 - Flags: review?(dolske) → review-
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.
Attached patch v6 (using DIV measurement now) (obsolete) — Splinter Review
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.
Attachment #477368 - Attachment is obsolete: true
Attachment #480650 - Flags: review?(dolske)
Attached patch v7 (with test)Splinter Review
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.
Attachment #480650 - Attachment is obsolete: true
Attachment #481571 - Flags: review?(dolske)
Attachment #480650 - Flags: review?(dolske)
Attachment #481571 - Flags: review?(dolske) → review?(dietrich)
Blocks: 597792
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.
Attachment #481571 - Flags: review?(dietrich) → review?(dolske)
Comment on attachment 481571 [details] [diff] [review]
v7 (with test)

Load balancing the review to me.
Attachment #481571 - Flags: review?(dolske) → review?(ian)
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.
Attachment #481571 - Flags: review?(ian) → review-
Moving to b9
Blocks: 598154
No longer blocks: 597043
bugspam (moving b9 to b10)
Blocks: 608028
bugspam (removing b9)
No longer blocks: 598154
Sean, are you still working on this?
Whiteboard: [visual][polish][good first bug]
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.
(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! ;)
bugspam. Moving b10 to b11
Blocks: 627096
bugspam. Removing b10
No longer blocks: 608028
Priority: P2 → P3
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.
Mitcho, I'll get this unrotted this afternoon. It's pretty gnarly.
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.
Priority: P3 → P4
[bugspam: mitcho's late-night betaN triage; feel free to comment or reverse]
Blocks: 603789
No longer blocks: 627096
Target Milestone: Firefox 4.0 → Future
bugspam: returning Sean's bugs to the pool
Assignee: seanedunn → nobody
Status: ASSIGNED → NEW
Blocks: 642891
bugspam
Target Milestone: Future → ---
No longer blocks: 603789
Bug 312156 will implement text-overflow, and it's scheduled for Fx6.
Depends on: 312156
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
This should be trivial to fix now that bug 312156 is fixed.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Tab set 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 for long titles.
Attached patch another patch (obsolete) — Splinter Review
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.
Attachment #546352 - Flags: review?(ian)
Attached patch another patchSplinter Review
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.
Attachment #546352 - Attachment is obsolete: true
Attachment #546391 - Flags: review?(ian)
Attachment #546352 - Flags: review?(ian)
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.
Attachment #546391 - Flags: review?(ian)
bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
No longer blocks: 660175
Indeed, only 'text-overflow: ellipsis' is needed, and one could combine that with bug 671243.
Fixed by bug 671243.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: