Last Comment Bug 356900 - Quiet place to work on SeaMonkey multiline tooltip stuff.
: Quiet place to work on SeaMonkey multiline tooltip stuff.
Status: RESOLVED FIXED
: fixed-seamonkey1.1b, relnote
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: 1.8 Branch
: x86 All
: -- enhancement (vote)
: ---
Assigned To: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
:
Mentors:
Depends on: 313266
Blocks: 357337 45375 67127
  Show dependency treegraph
 
Reported: 2006-10-16 18:19 PDT by Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
Modified: 2008-07-31 04:23 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (4.16 KB, patch)
2006-10-16 18:35 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: review-
neil: superreview-
Details | Diff | Review
patch v2 (4.42 KB, patch)
2006-10-17 06:24 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: review-
neil: superreview-
Details | Diff | Review
patch v3 (4.38 KB, patch)
2006-10-17 16:36 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: review+
neil: superreview+
kairo: approval‑seamonkey1.1b+
Details | Diff | Review
patch for 1.8 branch (929 bytes, patch)
2006-10-21 18:10 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: review+
neil: superreview+
cbiesinger: approval‑seamonkey1.1b+
Details | Diff | Review

Description Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-16 18:19:04 PDT
Patch in a minute.
Comment 1 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-16 18:35:01 PDT
Created attachment 242458 [details] [diff] [review]
patch

flex did not matter.  crop does (removing it would require yet more width hacks I have not yet figured out).  I took care of all the other review comments you had that are still relevant to this.  As I said before, my interpretation of the spec says: 

foo = bar.replace(/\n/g,"").replace(/\r/g," ");

is correct.  You wanted to preserve them, so I left them ;)
Comment 2 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-16 20:40:11 PDT
            else // if there's only one tab, it's selected anyway
              this.selectedTab = t;

Hmm... why did adding this.mStrip.collapsed = false; there not work? :-\
Comment 3 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-16 20:40:45 PDT
(In reply to comment #2)
>             else // if there's only one tab, it's selected anyway
>               this.selectedTab = t;
> 
> Hmm... why did adding this.mStrip.collapsed = false; there not work? :-\
> 

Oops, wrong bug.
Comment 4 neil@parkwaycc.co.uk 2006-10-17 04:36:01 PDT
Comment on attachment 242458 [details] [diff] [review]
patch

>+/* For multi-line tooltips, we need our own style */
>+.tooltip-label {
>+  max-width: 40em;
>+}
This should go in the theme, and it should have its own class or id, to avoid confusion with anonymous tooltip labels.

>+      var hbox = document.getElementById("tooltip-hbox");
>+      var label = hbox.firstChild;
Confusion of styles here - either use var hbox = tipNode.firstChild; or give the label an id. Does it help to move the direction style to the hbox?

>+      label.textContent = t; //NEIL: using the .replace() I removed would just result in wrapping
That's fine, see the multiline tests in bug 67127.

>+      hbox.removeAttribute("width"); //NEIL: .height=0 doesnt work
Does .height = "" work?

>+      var top = parseInt(window.getComputedStyle(label, null).marginTop);
The label shouldn't have a margin (the one in regular tooltips gets its margin style by virtue of being anonymous content).

I notice that in the testcase for bug 45375 the label's boxObject.width is reported incorrectly (i.e. not its actual width!).
Comment 5 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-17 06:22:17 PDT
(In reply to comment #4)
> (From update of attachment 242458 [details] [diff] [review] [edit])
> >+/* For multi-line tooltips, we need our own style */
> >+.tooltip-label {
> >+  max-width: 40em;
> >+}
> This should go in the theme, and it should have its own class or id, to avoid
> confusion with anonymous tooltip labels.

Ok.

> >+      var hbox = document.getElementById("tooltip-hbox");
> >+      var label = hbox.firstChild;
> Confusion of styles here - either use var hbox = tipNode.firstChild; or give
> the label an id. Does it help to move the direction style to the hbox?

Done (used firstChild).

> 
> >+      label.textContent = t; //NEIL: using the .replace() I removed would just result in wrapping
> That's fine, see the multiline tests in bug 67127.

Does that mean "what you have now is fine", or "put the old regex back"?

> >+      hbox.removeAttribute("width"); //NEIL: .height=0 doesnt work
> Does .height = "" work?

Yup.  Turns out I had to swap the ordering of setting them to new values  at the end thanks to a case I missed before (I was setting height first, but need to set width first - might be wrapping-related).

> >+      var top = parseInt(window.getComputedStyle(label, null).marginTop);
> The label shouldn't have a margin (the one in regular tooltips gets its margin
> style by virtue of being anonymous content).

But the amount this returns gives me the right amount of space to add.  If I don't add it, the tooltip's size is the label's size, and since the label's content doesn't start at 0,0 it results in a too-small tooltip.

> I notice that in the testcase for bug 45375 the label's boxObject.width is
> reported incorrectly (i.e. not its actual width!).

Lots of stuff is incorrect in lots of cases... the way I have it ordered now seems to result in things being reported such that it renders correctly.

The testcases I looked at were http://www.webdevout.net/testcases/title_tooltips/ and http://www.petesguide.com/WebStandards/tests/tooltips.html but in both cases I ignored the reference rendering (since the patch doesn't treat newlines exactly the way either of the testcases expect) and just used the testcase to verify that the tooltip was sizing itself properly to fit the content.
Comment 6 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-17 06:24:28 PDT
Created attachment 242495 [details] [diff] [review]
patch v2
Comment 7 neil@parkwaycc.co.uk 2006-10-17 06:32:34 PDT
(In reply to comment #5)
>(In reply to comment #4)
>>>+      label.textContent = t; //NEIL: using the .replace() I removed would just result in wrapping
>>That's fine, see the multiline tests in bug 67127.
>Does that mean "what you have now is fine", or "put the old regex back"?
The former.

>>>+      var top = parseInt(window.getComputedStyle(label, null).marginTop);
>>The label shouldn't have a margin (the one in regular tooltips gets its margin
>>style by virtue of being anonymous content).
>But the amount this returns gives me the right amount of space to add.  If I
>don't add it, the tooltip's size is the label's size, and since the label's
>content doesn't start at 0,0 it results in a too-small tooltip.
Sorry if I was unclear; regular tooltip labels have a zero margin, but class="tooltip-label" won't do this for your non-anonymous label.
Comment 8 neil@parkwaycc.co.uk 2006-10-17 07:38:28 PDT
Comment on attachment 242495 [details] [diff] [review]
patch v2

Apart from the margin issue, a couple of nits:

>+      var label = hbox.firstChild;
>+      label.textContent = t;
This variable is only used once... if moving the style from tipNode to hbox doesn't break anything then that saves another variable.

>+      hbox.width="";
>+      hbox.height="";
Spaces around =
Comment 9 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-17 16:36:44 PDT
Created attachment 242579 [details] [diff] [review]
patch v3

As discussed on IRC.
Comment 10 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-19 06:49:56 PDT
Comment on attachment 242579 [details] [diff] [review]
patch v3

This patch is primarily intended for branch, as on trunk it's fixed on reflow branch.
Comment 11 Robert Kaiser (not working on stability any more) 2006-10-19 06:55:42 PDT
Comment on attachment 242579 [details] [diff] [review]
patch v3

a=me for 1.1b

As a note, it would help if the bug summary and/or comment 0 would tell what the bug actually is. Only telling "Minor bug" without mentioning how it does manifest itself is quite bad for someone else to find out what this is all about.
Comment 12 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-19 16:25:26 PDT
Checked in on trunk.  I'll have to test it on branch before landing it there (*probably* today).

> As a note, it would help if the bug summary and/or comment 0 would tell what
> the bug actually is. Only telling "Minor bug" without mentioning how it does
> manifest itself is quite bad for someone else to find out what this is all
> about.

KaiRo, this bug is technically a duplicate of 45375 / 67127 which I filed because doing anything on those bugs takes forever due to the CC lists.  The patch fixes those bugs.
Comment 13 Krang 2006-10-20 09:59:06 PDT
It looks like this patch accidentally breaks tooltips on the branch (tested using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061020 SeaMonkey/1.1b).  Here is the error message:

Error: direction is not defined
Source File: chrome://navigator/content/browser.js
Line: 269

This appears to be caused by the fact that the patch for Bug 91312 has not yet been checked into the branch for SeaMonkey.
Comment 14 Steuard Jensen 2006-10-20 11:56:30 PDT
(In reply to comment #12)
> ...this bug is technically a duplicate of 45375 / 67127 which I filed
> because doing anything on those bugs takes forever due to the CC lists.  The
> patch fixes those bugs.

First, a side observation: until your change to it yesterday, bug 67127 was just as much a Firefox bug as it was a SeaMonkey bug.  I believe that a substantial number of people with interest in that bug are Firefox users.  So it's a bit disingenuous to say that your patch has "fixed" bug 67127.  But moving on...

One disadvantage (at least to outside observers) of doing this work in a separate bug is that other folks who might be interested in your what exactly your patch does (and how it does it) don't get a chance to ask questions or give feedback.  So, for example, how close does your patch come to the behavior outlined in bug 67127 comment 131?  Or is this patch something that will match that behavior only after bug 322270 is fixed (assuming it gets fixed in the optimal way)?  If the latter (which seems likely, from my brief look at your code), do you think it's wise to check in such a patch now, since web authors might conclude that SeaMonkey is endorsing the IE behavior?  Is it a problem that SeaMonkey's treatment of tooltips will change dramatically once bug 322270 is fixed?

And from another direction, just for my information, how specific is your code here to SeaMonkey?  That is, could I (or someone else) just copy your changes over to the corresponding places for Firefox and implement the same thing, or are there pitfalls that we should know about?  It looks pretty promising at first glance!
Comment 15 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-20 13:16:49 PDT
(In reply to comment #13)
> It looks like this patch accidentally breaks tooltips on the branch (tested
> using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061020
> SeaMonkey/1.1b).  Here is the error message:
> 
> Error: direction is not defined
> Source File: chrome://navigator/content/browser.js
> Line: 269
> 
> This appears to be caused by the fact that the patch for Bug 91312 has not yet
> been checked into the branch for SeaMonkey.
> 

Sander tried fixing the "direction" mistake I made, but reported that he still didn't get working tooltips.  I will figure it out tonight.

(In reply to comment #14)
> (In reply to comment #12)
> One disadvantage (at least to outside observers) of doing this work in a
> separate bug is that other folks who might be interested in your what exactly
> your patch does (and how it does it) don't get a chance to ask questions or
> give feedback.

I filed this bug specifically to avoid such feedback from the masses; anyone whose opinion matters (for example, you) can find this bug easily enough.

>  So, for example, how close does your patch come to the behavior
> outlined in bug 67127 comment 131?  Or is this patch something that will match
> that behavior only after bug 322270 is fixed (assuming it gets fixed in the
> optimal way)?  If the latter (which seems likely, from my brief look at your
> code), do you think it's wise to check in such a patch now, since web authors
> might conclude that SeaMonkey is endorsing the IE behavior?  Is it a problem
> that SeaMonkey's treatment of tooltips will change dramatically once bug 322270
> is fixed?

My interpretation of the spec says to do the following:
1) Replace entities with their values
2) Remove all \n
3) Replace all \r with " ".

That's not a very nice behavior, and does not match bug 67127 comment 131 (people want this feature to do things like a tooltip for an image that has the title on line 1, the size on line 2, etc - the spec would only allow for ordinary text which wraps on its own and give users no hard breaks), and my current patch does not implement that (partly because I don't like it much, partly because Neil doesn't seem to).  My most recent attachment on bug 45375 does implement the behavior according to the spec; it's a one-line change to "fix" it here.

I don't think comment 131 will ever be attainable (currently, we can't distinguis &#foo; from \n and in the future newlines will be removed by the parser.

If bug 322270 gets fixed in accordance with the spec, the behavior here will (automatically) change to match the spec.  Hixie seems to be saying that we might expect a more useful spec for HTML5 though; in that case, tooltips would presumably retain/regain a nice behavior.

I am aware that having the behavior change will displease users, but I don't think it's a problem worth worrying about much (they'll be pissed off no matter which decision we make...so I might as well decide to do it the way I like it).

> And from another direction, just for my information, how specific is your code
> here to SeaMonkey?  That is, could I (or someone else) just copy your changes
> over to the corresponding places for Firefox and implement the same thing, or
> are there pitfalls that we should know about?  It looks pretty promising at
> first glance!

I talked to some Firefox devs yesterday and most seemed interested in having it; some thought it might even be acceptable for 2.0.0.1 after it bakes in SeaMonkey for a while.  I can port it easily if they tell me it will get approval for FF2.x.  Reflow branch is supposed to fix it anyway for FF3.x.
Comment 16 Steuard Jensen 2006-10-20 14:58:21 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > So, for example, how close does your patch come to the behavior
> > outlined in bug 67127 comment 131?  Or is this patch something that
> > will match that behavior only after bug 322270 is fixed (assuming
> > it gets fixed in the optimal way)?

> My interpretation of the spec says to do the following:
> 1) Replace entities with their values
> 2) Remove all \n
> 3) Replace all \r with " ".

I've discussed this in too many places to be able to find the details, but my memory was that the spec doesn't explicitly specify the order in which one should do those things (it's an UL in the HTML 4.01 spec, not an OL).  One might normally assume that one should do them in the order listed, but I'd be willing to bend that assumption if doing so allowed us to enable a much-requested feature.  The behavior in bug 67127 comment 131 would result if we handled explicit whitespace (your #2 and 3) before entity substitution.  I'll admit that it's unlikely that the spec writers had this in mind, but it would work beautifully if we could make it happen.

I'll admit that my interim patch to bug 67127 went beyond the spec by collapsing whitespace entirely.  I just couldn't bring myself to delete \n without putting a space in its place (the only reason I can imagine for that is to avoid double spaces for CRLF platforms, but as it stands the spec means that the "same" document will display differently depending on which platform you save it on).  Apart from that, I figure that after we've properly parsed the CDATA we are free to choose how we want to display it, and "collapse whitespace" seemed like an attractive and reasonable choice to me.

> ...my current patch does not implement that (partly because I don't like
> it much, partly because Neil doesn't seem to).  My most recent attachment
> on bug 45375 does implement the behavior according to the spec; it's a
> one-line change to "fix" it here.

You've left out tab replacement there (which can also lead to a "black bars" problem), but I understand what you're doing.  I'd still lean toward my solution, I think, if only because the character replacements end up looking ugly at times if we don't collapse whitespace for display (which, as I said, I think we're free to do).  But my impression is that implementing my solution would also be just a one-line change to your patch as well.  (Okay, a two-line change, since I'd keep the comment as well.)

> I don't think comment 131 will ever be attainable (currently, we can't
> distinguis &#foo; from \n and in the future newlines will be removed by the
> parser.

Blake Kaplan's comment in bug 322270 comment 8 (and 5) made it sound like there were other places where "too early" entity replacement was painful as well, so I retained some small amount of hope that someone would eventually look for a way to delay it to a later stage.

> > If the latter (which seems likely, from my brief look at your code),
> > do you think it's wise to check in such a patch now, since web authors
> > might conclude that SeaMonkey is endorsing the IE behavior?  Is it a
> > problem that SeaMonkey's treatment of tooltips will change
> > dramatically once bug 322270 is fixed?

> I am aware that having the behavior change will displease users, but I don't
> think it's a problem worth worrying about much

My concern was more that the change you've implemented here (which will shift to match the IE behavior, as far as I can tell) will PLEASE a substantial community of users to no end, and they'll then be doubly angry when the fix to bug 322270 shifts the behavior back again.  Sending mixed messages could be worse than just sticking with a single less popular position.

> > ...could I (or someone else) just copy your changes over to the
> > corresponding places for Firefox and implement the same thing

> I talked to some Firefox devs yesterday and most seemed interested in having
> it; some thought it might even be acceptable for 2.0.0.1 after it bakes in
> SeaMonkey for a while.  I can port it easily if they tell me it will get
> approval for FF2.x.  Reflow branch is supposed to fix it anyway for FF3.x.

I don't know enough about what the reflow branch is all about to be able to comment on it: how does it handle tooltips compared to what we've been doing here?  If the job's already done for FF3, then I'm glad to know it (and it might be worth putting comments to that effect in some of the all-too-popular bugs out there).  Meanwhile, if there's a chance of getting this accepted for FF2.x, I'm sure that would be much appreciated by the community (I'd still advocate for my own take on the whitespace at this point, though).  Thanks for your work on this, in any case.
Comment 17 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-20 15:50:46 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > So, for example, how close does your patch come to the behavior
> > > outlined in bug 67127 comment 131?  Or is this patch something that
> > > will match that behavior only after bug 322270 is fixed (assuming
> > > it gets fixed in the optimal way)?
> 
> > My interpretation of the spec says to do the following:
> > 1) Replace entities with their values
> > 2) Remove all \n
> > 3) Replace all \r with " ".
> 
> I've discussed this in too many places to be able to find the details, but my
> memory was that the spec doesn't explicitly specify the order in which one
> should do those things (it's an UL in the HTML 4.01 spec, not an OL).  One
> might normally assume that one should do them in the order listed, but I'd be
> willing to bend that assumption if doing so allowed us to enable a
> much-requested feature.  The behavior in bug 67127 comment 131 would result if
> we handled explicit whitespace (your #2 and 3) before entity substitution. 
> I'll admit that it's unlikely that the spec writers had this in mind, but it
> would work beautifully if we could make it happen.

I don't disagree with anything you said here.  I would be satisfied with the ability to implement bug 67127 comment 131.  I would not be bothered by implementing the spec as I interpreted it.

> > ...my current patch does not implement that (partly because I don't like
> > it much, partly because Neil doesn't seem to).  My most recent attachment
> > on bug 45375 does implement the behavior according to the spec; it's a
> > one-line change to "fix" it here.
> 
> You've left out tab replacement there (which can also lead to a "black bars"
> problem), but I understand what you're doing.  I'd still lean toward my
> solution, I think, if only because the character replacements end up looking
> ugly at times if we don't collapse whitespace for display (which, as I said, I
> think we're free to do).  But my impression is that implementing my solution
> would also be just a one-line change to your patch as well.  (Okay, a two-line
> change, since I'd keep the comment as well.)

Yup.  I forgot about tabs... I don't really care either way about collapsing them.  If someone has a decent argument for why we should, feel free to attach a patch (or I can).

> > I don't think comment 131 will ever be attainable (currently, we can't
> > distinguis &#foo; from \n and in the future newlines will be removed by the
> > parser.
> 
> Blake Kaplan's comment in bug 322270 comment 8 (and 5) made it sound like there
> were other places where "too early" entity replacement was painful as well, so
> I retained some small amount of hope that someone would eventually look for a
> way to delay it to a later stage.

Yeah, it would be nice if the spec clearly said *exactly* what to do with whitespace in each situation, and someone implemented that.

> > > If the latter (which seems likely, from my brief look at your code),
> > > do you think it's wise to check in such a patch now, since web authors
> > > might conclude that SeaMonkey is endorsing the IE behavior?  Is it a
> > > problem that SeaMonkey's treatment of tooltips will change
> > > dramatically once bug 322270 is fixed?
> 
> > I am aware that having the behavior change will displease users, but I don't
> > think it's a problem worth worrying about much
> 
> My concern was more that the change you've implemented here (which will shift
> to match the IE behavior, as far as I can tell) will PLEASE a substantial
> community of users to no end, and they'll then be doubly angry when the fix to
> bug 322270 shifts the behavior back again.  Sending mixed messages could be
> worse than just sticking with a single less popular position.

True.  I guess I'm assuming bug 322270 won't actually happen for a long time since I don't think it's a high priority on anyone's radar.  Neil: want to put my regex back that implements "the spec" (with something to handle tab collapsing)?

> > > ...could I (or someone else) just copy your changes over to the
> > > corresponding places for Firefox and implement the same thing
> 
> > I talked to some Firefox devs yesterday and most seemed interested in having
> > it; some thought it might even be acceptable for 2.0.0.1 after it bakes in
> > SeaMonkey for a while.  I can port it easily if they tell me it will get
> > approval for FF2.x.  Reflow branch is supposed to fix it anyway for FF3.x.
> 
> I don't know enough about what the reflow branch is all about to be able to
> comment on it: how does it handle tooltips compared to what we've been doing
> here?  If the job's already done for FF3, then I'm glad to know it (and it
> might be worth putting comments to that effect in some of the all-too-popular
> bugs out there).  Meanwhile, if there's a chance of getting this accepted for
> FF2.x, I'm sure that would be much appreciated by the community (I'd still
> advocate for my own take on the whitespace at this point, though).  Thanks for
> your work on this, in any case.

The issue isn't really specific to tooltips - it's related to ambiguities about how wide a block of text should be, and how tall it should be.  They're supposed to have all of these things defined precisely for reflow branch.  Tooltips just happen to be the most-visible impact, but there are ways to cause the same ugliness with XUL outside of tooltips.
Comment 18 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-20 16:06:11 PDT
FWIW, Neil said on IRC he thinks we should show what the parser gives us.
Comment 19 Steuard Jensen 2006-10-20 16:24:28 PDT
(In reply to comment #18)
> FWIW, Neil said on IRC he thinks we should show what the parser gives us.

Rather than collapsing whitespace, you mean?  I'm okay with that, though I think that it will lead to some mild weirdness on a number of pages (doubled spaces will turn up randomly if you compose your HTML in an editor with word wrap, for example, any time a title attribute gets split across lines).  Personally, I feel like the collapsing whitespace makes the tooltip behave more like ordinary HTML text, which is what I've expected in the past.

I assume he's still okay with the idea of replacing \n, etc. according to the spec until bug 322270 is fixed?  Or do you mean that he's in favor of showing the linebreaks like IE does (until the parser's behavior changes: bug 322270 is targeted at 1.9alpha)?  I've already explained my concerns about that approach; are there strong counter-arguments?  (Also, is the exact format of the parser output particularly meaningful?  I've usually thought of parser output as something that we are free to format any way we'd like, just as we do with plain HTML text and a CSS "white-space: normal" rule.)
Comment 20 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-20 16:37:57 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > FWIW, Neil said on IRC he thinks we should show what the parser gives us.
> 
> Rather than collapsing whitespace, you mean?  I'm okay with that, though I
> think that it will lead to some mild weirdness on a number of pages (doubled
> spaces will turn up randomly if you compose your HTML in an editor with word
> wrap, for example, any time a title attribute gets split across lines). 
> Personally, I feel like the collapsing whitespace makes the tooltip behave more
> like ordinary HTML text, which is what I've expected in the past.
> 
> I assume he's still okay with the idea of replacing \n, etc. according to the
> spec until bug 322270 is fixed?  Or do you mean that he's in favor of showing
> the linebreaks like IE does (until the parser's behavior changes: bug 322270 is
> targeted at 1.9alpha)?  I've already explained my concerns about that approach;
> are there strong counter-arguments?  (Also, is the exact format of the parser
> output particularly meaningful?  I've usually thought of parser output as
> something that we are free to format any way we'd like, just as we do with
> plain HTML text and a CSS "white-space: normal" rule.)

I don't know if he has a strong counter argument.  Get on IRC and chat ;).

BTW, theme changes are required, which Mano pointed out they can't do in FF2.0.0.x.
Comment 21 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-21 16:48:56 PDT
Neil, on 1.8 branch, it looks like setting the label's textContent causes the tooltip not to show up.  Using

  <tooltip id="aHTMLTooltip" onpopupshowing="return FillInHTMLTooltip(document.tooltipNode);">
    <hbox>
      <label flex="1" class="htmltooltip-label"/><label>ASDF</label>
    </hbox>
  </tooltip>

gives "ASDF" tooltips if I don't set the textContent, but doesn't show anything if I do.  There are no errors in the JS console; this is not a debug build, so I can't see if there are any asserts or other backend-type issues going on when I do set textContent.
Comment 22 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-21 18:10:12 PDT
Created attachment 243059 [details] [diff] [review]
patch for 1.8 branch

1. Remove the "direction" line (improper merge to branch).
2. Work around bug 313266 which is branch-only (fun fun fun).

Sander tested it too.
Comment 23 neil@parkwaycc.co.uk 2006-10-22 03:44:48 PDT
Comment on attachment 243059 [details] [diff] [review]
patch for 1.8 branch

>+      label.setAttribute("flex", 1);
>+      label.setAttribute("class", "htmltooltip-label");
Nit: use .flex and .className here.

I thought it might be easier to cloneNode the label but it turns out you have to remove the label from the box before cloning it otherwise it doesn't like you replacing the existing label for some reason (I get an assertion).

var label = hbox.firstChild;
hbox.removeChild(label);
label = label.cloneNode(false);
label.textContent = t;
hbox.appendChild(label);
Comment 24 Christian :Biesinger (don't email me, ping me on IRC) 2006-10-22 11:26:31 PDT
Comment on attachment 243059 [details] [diff] [review]
patch for 1.8 branch

a=biesi for sm 1.1b
Comment 25 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-22 12:00:14 PDT
Fixed branch.
Comment 26 Serge Gautherie (:sgautherie) 2006-10-24 04:21:59 PDT
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1) Gecko/20061023 SeaMonkey/1.1b] (nightly) (W98SE)

Comment 13 regression V.Fixed on MOZILLA_1_8_BRANCH.
Comment 27 Felix Miata 2006-10-24 09:47:00 PDT
Comment 13 should have been filed as a new bug. I just wasted 2 hours hunting the regression range for new bugs filed since it began, only to find out it's already fixed, which I couldn't even do on Linux because the nightlies quit on the 21st. Bugzilla doesn't find this bug in a created since tooltip search that begins the day before this regression occurred.

Note You need to log in before you can comment on or make changes to this bug.