248488 - Improve Tooltip display of long TITLE attribute values

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[precision-image]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040623 Camino/0.8
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040623 Camino/0.8

This bug seems very similar to PC-bug 247815... however I did not find any
Camino bugs when I searched for the string "vBulletin" so I thought it worth
posting. 

Reproducible: Always
Steps to Reproduce:
1. Go to a vBulletin 3.0.x site that has the thread preview feature turned on by
defult.
2. Run your curosor over a thread title and wait a second or two
3. Camino will show the preview but in such a way as to use only a single row of
text. If the preview is set to display enough characters, the beginning and end
of the preview will get truncated at the screen edges.



Expected Results:  
As a comparison, Safari will show the thread previews in a "box" rather than a
single row, and will use ellipses to show that there is more to be viewed when
the character limit has been reached. Camino should also handle vBulletin Thread
Previews in this way IMO.

Comment 1

[Stuart Morgan]

Confirmed, but I'm not sure if it's a dup or not. Is our tooltip system distinct
enough that we would have a separate fix?

Comment 2

[Mike Pinkerton (not reading bugmail)]

yeah, this bugs me as well. we should fix it.

Comment 3

[Mike Pinkerton (not reading bugmail)]

our toolips are custom. gecko tells us what to show, but now how to do it. we
also don't use the os tooltips for other reasons (bugs which i no longer recall).

so something isn't wrapping correctly. shouldn't be tough to fix.

Comment 4

[Josh Aas]

Attachment: test case

Use this little test page to test the patch.

Comment 5

[Josh Aas]

Attachment: patch that almost works

This patch almost works, but with one annoying bug I can't seem to fix. Apply,
compile, then try the test page I just posted.

1. Move mouse over short string until tooltip pops up
2. Move mouse over long string until tooltip pops up
3. Move mouse off of long string onto blank space
4. Move mouse back over long string until tooltip pops up

The first time you move the mouse over the long string it is incorrectly sized
(right after moving mouse over the short string). The second time it is
correctly sized. Arg.

If someone else could fix this, do a thorough review, and land it before I get
back to it that would be great :) Otherwise I'm just posting this patch here so
I can keep track of it myself.

Comment 6

[Josh Aas]

Take a look pink? Probably something obvious I'm missing because I've been
staring at this for way too long.

Comment 7

[Josh Aas]

Attachment: patch that really does work

This updated patch works in all cases I could think of. Here is what it does:

- smarter sizing. sizes tooltips correctly, wraps them at a certain width.
allows multiple lines of text
- smarter about what it does when a tooltip would display at least partially
offscreen

Comment 8

[Maximilian Ruderburg [:max] (Arctic Revolution)]

Latest patch looks good and works but might be "too" smart.

Example case of http://www.zeldman.com, see the side "Departments":
1) colophon, contrast, externals, and rss feed all render fine and as they should (Yay, the patch 
worked).
2) contrast-o-meter, essentials, goodies, and pleasure cut their line short. As an example, goodies title 
says "Toys for" on one line and puts "you." on another. The source of the page doesn't show any sign of 
this being a purposeful thing.

Besides the minor cutting of some tooltips/titles short, this patch works.

Comment 9

[Josh Aas]

Nice find. That does need to be fixed.

Comment 10

[Ludovic Hirlimann [:Usul]]

Comment on attachment 153364 [details] [diff] [review]
patch that really does work

>Index: src/browser/ToolTip.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/ToolTip.h,v
>retrieving revision 1.1
>diff -u -p -r1.1 ToolTip.h
>--- src/browser/ToolTip.h	8 Aug 2002 19:36:10 -0000	1.1
>+++ src/browser/ToolTip.h	16 Jul 2004 03:23:44 -0000
>@@ -20,6 +20,7 @@
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s): Richard Schreyer
>+ *                 Josh Aas <[e-mail]>
I thought you where using gmail for Camino related stuff ?

If you need a review set me for reviewer, I'll do it on Monday. I can't before
that date.

Comment 11

[Josh Aas]

I do use gmail for moz related stuff, but for licensing/copyright things I use
my more permanent address.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Josh, did the patch for this bug work and never get checked in, or something else?

Also, this appears to be a duplicate of bug 167435--which has a much better
summary of the underlying problem than this one, but no patch and a completely
different target milestone....

I made a comment in 167435 not too long ago and found this today while looking
for something else in your blog; if these two are duplicates and this one stays
because of the patch, I recommend at least adding "TITLE" and "tooltip" to this
bug's summary.

Comment 13

[Jasper]

*** Bug 167435 has been marked as a duplicate of this bug. ***

Comment 14

[Jasper]

Thank you Smokey you are right, did the paperwork to resolve the other one and
chnages this bug title to be like Bug 167435.

Comment 15

[Josh Aas]

The patch was not checked in because of comment #8. It isn't ready.

Comment 16

[Julian Pellico]

Attachment: reimplement |sizeToFit| -- still needs a tweak

Hi,

I came to therealization that we just need to not use sizeToFit -- other people
have complianed about it on cocoa-dev.

This patch tries to reimplement it, and is pretty close to fully working, in my
testing. The main problem (check on vBulletin) is that on some multiline
tooltips, the text will actually be cut off at the top by either part of a line
or several lines -- it's as if the text view is moved up so it starts above the
tooltip frame. The real wierd thing here is that usually by moving the mouse to
make the tip reappear, the text will be located correctly.

I hope this helps and I'd like people to try it out for a spin. If I have time
I'll try to squash the remaining issue :)

Comment 17

[Josh Aas]

Attachment: updated version of "patch that really does work"

Still has bugs, not asking for review. Just updating it so it applies if anyone
wants to mess with it. I'm working on it at the moment but I suspect I won't
get far.

Comment 18

[Mike Pinkerton (not reading bugmail)]

i was playing with "patch that really does work" and it with the exception of it
wrapping a few tooltips that should be a single-line, it works SO MUCH BETTER
than what we have now we should really just go ahead and land it and move on
with our lives.

Comment 19

[Mike Pinkerton (not reading bugmail)]

i landed the "updated version of patched that works" on the trunk and i think we
should ship it in 084. Like I said, it's really a LOT better than what we have
now. If we want to work to improve sizeToFit (which after playing with it I
agree has serious issues)) on the trunk, we can, though the i18n issues might be
hairy. 

I'll deal with landing the patch on the 17branch.

Comment 20

[Josh Aas]

I doubt we're going to fix the problem any time in the near future as sizeToFit
is busted or has unhelpful characteristics, and re-implementing sizeToFit such
that it works with all character sets, text directions and whatnot pretty much
isn't going to happen. The only way this will get fixed any time soon is with an
OS update.

Comment 21

[Mike Pinkerton (not reading bugmail)]

landed on branch. should we just future this?

Comment 22

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 178542 [details] [diff] [review]
updated version of "patch that really does work"

landed on trunk and branch, obsoleting and plussing.

Comment 23

[Wevah (Nate Weaver)]

Looks good so far, aside from some of the margins being a few px too large.

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #18)
> i was playing with "patch that really does work" and it with the exception of it
> wrapping a few tooltips that should be a single-line, 

Wow, I see how messed up that underlying function is!  :-(

Have a look at tooltips on Wikipedia articles 
http://en.wikipedia.org/wiki/Camino
is good, as is 
http://en.wikipedia.org/wiki/Template:Europe 

And notice the number of short tootips wrapping strangely: Mozil la,  2 0 0 5, etc.

Long ones do seem to be fixed nicely, except for some wierdness in the second
testcase from bug 167435, attachment id=123085, going upwards offscreen.

Comment 25

[Mike Pinkerton (not reading bugmail)]

yeah |sizeToFit| must have internal rounding errors. It would wrap "Latest
Firefox Nightly Build" but keep "Latest Firefox NigltlY Build" as a single line
-- the only difference being the capital "Y" in Nightly, an even wider string to
boot!

regardless, there's no point pointing out all the places where this falls down,
we know it does, we know there's not a whole heck of a lot we can do about it. 

Comment 26

[Greg K.]

I'm happy to see multiline tooltips, but it is also worth pointing out (for
future reference, if anything) that HTML 4 actually specifies a different behavior.

Specifically, since TITLE attribute values are like any other attribute value
merely CDATA, <http://www.w3.org/TR/html4/types.html#type-cdata> says UAs are
supposed to replace "...each carriage return or tab with a single space."

Of course, if I'm wrong, I'd be glad to hear about it.

Comment 27

[Simon Fraser [no longer active]]

I think tooltips are worse now than they were before. I just got one like:

F
U
D

Comment 28

[Mike Pinkerton (not reading bugmail)]

for every one that's worse, there are 50 that no longer take up the full screen
width. Have fun with sizeToFit. I think we're in a better place than we were.

Comment 29

[Simon Fraser [no longer active]]

Attachment: Richer testcase

Comment 30

[Simon Fraser [no longer active]]

I checked in some code that measure the string width by hand, adds some magic
numbers, uses that as a min width, and then calls -sizeToFit. It fixes the
examples in the testcase.

Comment 31

[Mike Pinkerton (not reading bugmail)]

won't setting that minWidth then cause long tooltips to be the whole screen
again?  What am i missing?

Comment 32

[Mike Pinkerton (not reading bugmail)]

ah, i see now. the max width takes care of that. looks better, fixes a lot of
the places where i noticed problems (mozillazine.org, for example). 

Comment 33

[Ashley Bischoff (blog at handcoding.com)]

(In reply to comment #30)
> I checked in some code that measure the string width by hand, adds some magic
> numbers, uses that as a min width, and then calls -sizeToFit. It fixes the
> examples in the testcase.

Is the fix here of any use towards Firefox bug 45375?

Comment 34

[Simon Fraser [no longer active]]

No, this is all Cocoa code.

Comment 35

[Julian Pellico]

Comment on attachment 173591 [details] [diff] [review]
reimplement |sizeToFit| -- still needs a tweak

If no one's gonna kill my patch, I'll kill it myself :P

Comment 36

[Julian Pellico]

Simon, is there a reason the code you checked in does not appear here as a patch?

Comment 37

[Simon Fraser [no longer active]]

Yes. a) I'm lazy b) I'm a senior camino developer and can do such things, and c)
you can see the patch in bonsai anyway  :)