431325 - Bookmark tooltip's title/url is cut off

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Gabriele Best [:Gabri]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.9pre) Gecko/2008042806 Minefield/3.0pre
Build Identifier: 

If I show a tooltip longer than 70/75 chars it will cut off.

It doesn't seem to be a skin's bug.

Reproducible: Always

Actual Results:  
Tooltip's width has no limits.

Comment 1

[Gabriele Best [:Gabri]]

Attachment: Screenshot of bug

Comment 2

[Kai Liu]

I should add that this only affects themed Windows.  On classic Windows, the tooltip will expand to accommodate the text (space permitting, of course) and does not cut off like that.  This bug is also related to bug 378638, but that one is about wrapping the bookmark tooltip if it gets too long, not about why it doesn't expand in themed Windows.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> I should add that this only affects themed Windows.

I can confirm this in Windows Vista using this build...
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

...and with "appearance settings" set to "Windows Vista Basic".

But the bug is gone on "Windows Classic".

Comment 4

[Gabriele Best [:Gabri]]

I use WinXP and it is present on all the themes.

Comment 5

[Daniel Holbert [:dholbert]]

Mrtb: It'd be useful if you could list the themes you've tested and that you see the bug on -- in particular, do you see it on the "Windows Classic" theme?

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Not going to block, as it doesn't seem to affect tooltips in content, but it would be good to figure out what's going on.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
Mrtb: Oh, sorry, I missed the part about you using Windows XP.  Interesting that the bug would be present in more themes there than on Vista...

Comment 8

[Kai Liu]

The bug is present only if IsAppThemed().  It doesn't manifest itself in classic, regardless of whether it's XP or Vista (at least, that's what I observed in my testing).  What puzzled me when I first saw this is why I am seeing this on XP Luna but not on XP Classic; bug 161600 forced tooltips to follow the classic code path on XP.

Comment 9

[Kai Liu]

Okay, the problem turns out to be much, much simpler than I had initially thought: the CSS for tooltips in toolkit specifies a max-width of 440px, which is the cause of the cutoff.

But why didn't the tooltip get constrained to 440px in Classic?  It's because there seems to be a bug that causes max-width constraints to not work if min-width is set to 0 (this seems to affect only tooltips).  ClassicGetMinimumWidgetSize does not set a min-width for tooltips, while GetMinimumWidgetSize sets a non-zero min-width when a theme is present (and to answer my own question in comment 8, bug 161600 only forced the painting to take the classic code path for tooltips; GetWidgetPadding and GetMinimumWidgetSize still take the themed code path for tooltips).  So the non-broken appearance in Classic is actually the result of a bug (and this is why it took me this long to notice the 440px max-width in the CSS; I was barking up the wrong tree all this time).

Simply setting max-width: none will fix the cutoff.  Do we even need a max-width for this particular tooltip?  It won't expand beyond the available screen space anyway...

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Kai: great work tracking this down! We should probably just remove the max-width, yeah...

Any idea why this would only affect chrome tooltips, per comment 6?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Looks like that max-width was added to themes/classic/global/win/menu.css revision 1.12 in a checkin referring to bug 46613 (which doesn't have the patch), without any real reasoning, and eventually made it's way to themes/classic/global/win/popup.css which was then forked to become toolkit/themes/winstripe/global/popup.css.

On second thought, just removing the max-width and having unbounded tooltips might be strange. I wonder whether there's some other easy way to fix this.

Comment 12

[Neil Deakin]

The tooltip should have a maxwidth because longer text is easier to read over a few short lines rather than one screen-wide line.

Comment 13

[Kai Liu]

(In reply to comment #10)
> Any idea why this would only affect chrome tooltips, per comment 6?
> 
None yet.  Nor am I sure if it's something that only affects tooltips.  I found the problem right before going to bed, so I hadn't had time to look at it more closely yet.  I'll try to do that some time today.

(In reply to comment #12)
> The tooltip should have a maxwidth because longer text is easier to read over a
> few short lines rather than one screen-wide line.
> 
I agree, especially since allowing the tooltip to expand means that these bookmark tooltips can still be cut off if the title or URL are sufficiently long.  The problem here is that this tooltip isn't wrapping, and as a result, the max-width is just chopping off the tooltip, which is, IMHO, worse than having a wide tooltip.

In any case, implementing wrapping for this tooltip (bug 378638) seems to be the correct long-term solution here.  I'm still pretty new to all this, so please correct me if I'm wrong, but from skimming through bug 45375 and bug 218223, it seems that making it wrap would be as simple as adding white-space: -moz-pre-wrap, right?

Comment 14

[Neil Deakin]

> I agree, especially since allowing the tooltip to expand means that these
> bookmark tooltips can still be cut off if the title or URL are sufficiently
> long.  The problem here is that this tooltip isn't wrapping, and as a result,
> the max-width is just chopping off the tooltip, which is, IMHO, worse than
> having a wide tooltip.

Wait, did you say bookmark tooltips? This issue only occurs for bookmark tooltips? If so, they use a different <tooltip> element.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Yeah, but they're both still <tooltip>s. Is it caused by the bookmark tooltip's flexed vbox?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.xul&rev=1.465&mark=97#97
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.xul&rev=1.465&mark=233-239#232

Comment 16

[Kai Liu]

Attachment: labels are fun

(In reply to comment #15)
> Yeah, but they're both still <tooltip>s. Is it caused by the bookmark tooltip's
> flexed vbox?
> 
Labels seem to behave strangely when it comes to layout.  I was wrong in comment 9; the strange min-width/max-width isn't exclusive to this tooltip: it happens when there is a label involved.

In the attachment, you'll see that despite setting a max-width of 100px on all the <box> elements, the only time when it actually shrunk to 100px was when it had a non-zero min-width (<box>es without label children are not affected and will respect max-width correctly).  Interestingly, even if the label was constrained in size, the box would still be wide unless the max-width had a corresponding min-width (third test case).  Also, if crop="end" was not specified, the label itself suffers from the min-width/max-width quirk (fourth and fifth test cases)

As for wrapping, <tooltip> already specifies wrapping.  It seems that the key difference between btTooltip and aHTMLTooltip, from looking at the DOM tree, is that the latter stores the text in a #text node.

Finally, it seems that the behavior of <label value="xxx" /> illustrated by this attachment has been noticed before.  A quick search turned up bug 187188 and bug 430608.  So I think the best thing to do here is to just avoid using <label value= for btToolTip.  Or we could even get rid of the <vbox> and <label>s from btToolTip entirely and just use a line break in the text to separate the title and URL portions.

Comment 17

[Kai Liu]

Attachment: labels are fun (the correct file)

It would help if I select the correct file when attaching...

Comment 18

[Neil Deakin]

(In reply to comment #16)
> Labels seem to behave strangely when it comes to layout.

It looks correct to me. The label extends past the containing box because it defaults to overflow: visible. You'll find the same effect applies if you put a form control in a box with a smaller max-width.

It looks like the bookmark tooltip should just wrap its content by using <label>content</label> rather than <label value="content"/> so that the text behaves like an inline.

Comment 19

[Kai Liu]

(In reply to comment #18)
> It looks correct to me. The label extends past the containing box because it
> defaults to overflow: visible. You'll find the same effect applies if you put a
> form control in a box with a smaller max-width.
> 
Yes.  The strangeness that I was referring to was not the overflow (which was expected), but how it affected the behavior of the containing box (specifically, the failure of the containing box to respect max-width when min-width is zero, even if the label itself had been constrained).  But that's really an issue for another bug.  :)

Comment 20

[Kai Liu]

Attachment: Option 1: wrap title & center-crop URL

Well, that was a fun wild goose chase.  Turns out that the fix is pretty simple...

The strangeness with min-/max-width should probably be spun off into a new bug (unless there's an existing xul:label bug that fits).

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 319824 [details] [diff] [review]
Option 1: wrap title & center-crop URL

Neil is probably a better reviewer for this. Why are we center cropping the URL?

Comment 23

[Kai Liu]

(In reply to comment #21)
> (From update of attachment 319824 [details] [diff] [review])
> Why are we center cropping the URL?
> 
It's what the urlTooltip is doing, and a long wrapped URL looks a little odd and noisy, but that's my personal opinion.  I'll post screenshots of both...

Comment 24

[Kai Liu]

Attachment: screenshots

Comment 25

[Kai Liu]

Attachment: Option 2: wrap title & URL

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 319824 [details] [diff] [review]
Option 1: wrap title & center-crop URL

OK, r=me then, beltzner can decide which of the two he prefers.

Comment 27

[Gabriele Best [:Gabri]]

I'd prefer that labels don't wrap and that tooltips' width will be wider, IMHO.

Comment 28

[Gabriele Best [:Gabri]]

And what about first option and crop="right"?

Comment 29

[Kai Liu]

(In reply to comment #28)
> And what about first option and crop="right"?
> 
A end-cropping does not make sense for URLs, because the two most important parts of the URL are the beginning (protocol & server) and the end (the name & extension of the file/page that you are looking at).  This is why the location bar's tooltip uses center-cropping, and if cropping is to be used by this tooltip, it should be center-cropping.

Comment 30

[Gabriele Best [:Gabri]]

With the last nightly tooltips aren't cut off anymore O___O
Have you the same result?

Comment 31

[Kai Liu]

(In reply to comment #30)
> With the last nightly tooltips aren't cut off anymore O___O
> Have you the same result?
> 
They still are in Vista Aero, but not in NT 5.x, and this is only a side-effect of a change to the tooltip widget code that was made in another bug (which had addressed the issue of tooltips in Luna still taking a themed code path for metrics even though it was using an unthemed code path for rendering; see comment 9).  But keep in mind that the expanded tooltip that you see is actually the result of buggy behavior where max-width is not being respected, so the wrapping is still something that needs to be done (esp. since this still affects Vista).

Comment 32

[Gabriele Best [:Gabri]]

(In reply to comment #31)
> (In reply to comment #30)
> > With the last nightly tooltips aren't cut off anymore O___O
> > Have you the same result?
> > 
> They still are in Vista Aero, but not in NT 5.x, and this is only a side-effect
> of a change to the tooltip widget code that was made in another bug (which had
> addressed the issue of tooltips in Luna still taking a themed code path for
> metrics even though it was using an unthemed code path for rendering; see
> comment 9).  But keep in mind that the expanded tooltip that you see is
> actually the result of buggy behavior where max-width is not being respected,
> so the wrapping is still something that needs to be done (esp. since this still
> affects Vista).
> 

Do you think that this bug should be fixed?

Comment 33

[Kai Liu]

(In reply to comment #32)
> Do you think that this bug should be fixed?
> 

Well, seeing as how the bookmark tooltip has been cutting off since as early as Firefox 1, I don't think we need to be in a hurry about this.  This is something for 3.1.  Or maybe 3.0.1.

Comment 34

[Gabriele Best [:Gabri]]

But now max-width is not being respected, isn't it a bug?

Comment 35

[Gabriele Best [:Gabri]]

I created this addon https://addons.mozilla.org/it/firefox/addon/7314 that fixes this bug. It fixes also bug 237592.

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 319830 [details]
screenshots

Center-crop, I think, is best. For people who want the full monty, there's always properties.

Comment 37

[Gabriele Best [:Gabri]]

checkin-needed for patch "Option 1: wrap title & center-crop URL"

Comment 38

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/367dd7952f34

Comment 39

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20081006 Minefield/3.1b1pre.

Comment 40

[Tony Chung [:tchung]]

Also verified on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20081006 Minefield/3.1b1pre

Comment 41

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv