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

RESOLVED FIXED

Status

Firefox Graveyard
Panorama
P4
minor
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: vladmaniac, Unassigned)

Tracking

Details

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

Attachments

(3 attachments, 8 obsolete attachments)

(Reporter)

Description

7 years ago
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

Updated

7 years ago
Summary: [FF4][Tab Candy] Tab set names move over the "x" dismiss button → Tab set names move over the "x" dismiss button

Updated

7 years ago
Assignee: nobody → seanedunn

Comment 1

7 years ago
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.
Attachment #473386 - Flags: feedback?(ian)

Updated

7 years ago
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+

Updated

7 years ago
Duplicate of this bug: 594957

Updated

7 years ago
Attachment #473386 - Flags: review?(dietrich)
Duplicate of this bug: 586545
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

Comment 6

7 years ago
(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.

Updated

7 years ago
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.

Comment 7

7 years ago
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
Attachment #473386 - Attachment is obsolete: true

Updated

7 years ago
Duplicate of this bug: 589900

Comment 9

7 years ago
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.
Attachment #476190 - Attachment is obsolete: true

Comment 10

7 years ago
Created attachment 476508 [details] [diff] [review]
v4 WIP
Attachment #476508 - Flags: ui-review?

Comment 11

7 years ago
Created attachment 476510 [details] [diff] [review]
v4 WIP

* 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?

Updated

7 years ago
Attachment #476510 - Flags: ui-review? → ui-review?(aza)

Updated

7 years ago
Blocks: 597043

Updated

7 years ago
Attachment #476510 - Flags: ui-review?(aza)
Attachment #476510 - Flags: ui-review+
Attachment #476510 - Flags: feedback?(ian)

Comment 12

7 years ago
This looks and acts fantastic, Sean!

Updated

7 years ago
Priority: P3 → P2
Target Milestone: --- → Firefox 4.0

Updated

7 years ago
Duplicate of this bug: 590249
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+

Comment 15

7 years ago
Created attachment 477368 [details] [diff] [review]
v5
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.

Comment 19

7 years ago
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.
Attachment #477368 - Attachment is obsolete: true
Attachment #480650 - Flags: review?(dolske)

Comment 20

7 years ago
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.
Attachment #480650 - Attachment is obsolete: true
Attachment #481571 - Flags: review?(dolske)
Attachment #480650 - Flags: review?(dolske)

Updated

7 years ago
Attachment #481571 - Flags: review?(dolske) → review?(dietrich)

Updated

7 years ago
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-

Comment 24

7 years ago
Moving to b9
Blocks: 598154

Updated

7 years ago
No longer blocks: 597043

Comment 25

7 years ago
bugspam (moving b9 to b10)
Blocks: 608028

Comment 26

7 years ago
bugspam (removing b9)
No longer blocks: 598154
Sean, are you still working on this?
Whiteboard: [visual][polish][good first bug]

Comment 28

7 years ago
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! ;)
Blocks: 624936

Comment 30

7 years ago
bugspam. Moving b10 to b11
Blocks: 627096

Comment 31

7 years ago
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.

Comment 33

7 years ago
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

Updated

7 years ago
Duplicate of this bug: 642883

Updated

7 years ago
Blocks: 642891

Comment 38

7 years ago
bugspam
Target Milestone: Future → ---
No longer blocks: 603789
Blocks: 653099
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.

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 666842
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 → ---
Duplicate of this bug: 671238
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.

Comment 46

6 years ago
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.
Attachment #546352 - Flags: review?(ian)

Comment 47

6 years ago
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.
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

Comment 50

6 years ago
Indeed, only 'text-overflow: ellipsis' is needed, and one could combine that with bug 671243.
Depends on: 671243
Fixed by bug 671243.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.