The text inside tooltips which appear when hovering the mouse over an object with a TITLE attribute should have the same BiDi directionality as the object which had the TITLE attribute. Can be demonstrated on www.msn.co.il, on any news link which has mixed character (e.g. HEBREW and a QUESTION MARK or HEBREW and ENGLISH).

We test for the direction in |nsTextBoxFrame::PaintTitle|, but it looks like it's never getting set for a popup frame.

hyatt, can you give me some advice here? How can the tooltip inherit the direction style from its parent element? Thanks

The tooltip is in the chrome. It is not a child of the element in the content area, and therefore there's no way you can inherit direction into it. The same tooltip object is reused for all elements in the content area.

I was afraid of that :-/ Does that mean that this bug can't be fixed?

We could make title tips examine the directionality of the target and then dynamically switch directionality for that particular tooltip.

Mass-move all BiDi Hebrew and Arabic qa to me, zach@zachlipton.com. Thank you Gilad for your service to this component, and best of luck to you in the future. Sholom.

Another way to go would be to determine the direction of the title tips from their content. We already have support for doing this (see http://lxr.mozilla.org/mozilla/source/intl/unicharutil/public/nsIBidi.h#270), so it would be a one-line patch. I have done this in my local tree and my own artificial testcase in attachment 42868 [details] doesn't work, but it looks good on real-world examples. I also don't see in the HTML standard any suggestion that the title attribute /should/ inherit the directionality of the parent. Thoughts, anyone?

W3C's standard doesn't specify the rendering of the TITLE attribute at all, but since DIR attribute's documentation says it's inherited, we can assume it'll inherit to the tooltip too. And most importantl, it's for real word compatibility. I feel bad for troubling you guys with such nitpicking bugs :)

Simon, you've figured out why this doesn't work on the artificial example?

I knew why all along :-). The example contains first English then Hebrew, with <dir = rtl> (inherited from the root element), so the English appears to the right of the Hebrew. With the patch to determine the direction from the context, since the English comes first the direction is set to ltr, so the English appears to the left.

Oh, you're determining from contents? Failed to read that, and that's not exactly what's expected, as you probably agree. Along with the bug which makes directionality marks draw garbage characters, this simply makes it impossible to do it right for a web developer.

Returning to this bug after 2 years, fixing it seems pretty trivial, in pure JS code. Works both on the testcase and real life (MSN.co.il). What you say?

(In reply to comment #13) > Created an attachment (id=151098) > Sets tooltip direction to element direction > > Returning to this bug after 2 years, fixing it seems pretty trivial, in pure JS > code. Works both on the testcase and real life (MSN.co.il). What you say? please request review from smontagu. 10x

That looks great to me, but note the comment at http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/browser.js#339 339 * NOTE: Any changes to this routine need to be mirrored in ChromeListener::FindTitleText() 340 * (located in mozilla/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp) 341 * which performs the same function, but for embedded clients that 342 * don't use a XUL/JS layer. It is important that the logic of 343 * these two routines be kept more or less in sync. Also, you should probably set a default value before the |if|.

... but having said that I find that there is no ChromeListener::FindTitleText(), and hasn't been for almost three years. It was replaced by DefaultTooltipTextProvider::GetNodeText() in bug 90195. The parallel firefox file xpfe/browser/resources/content/browser.js will also need patching.

Same code for Mozilla and Firefox. 'direction' is set to 'inherit' by default, which is the CSS default. (Not setting direction at all would be the wrong solution, since that'll cause the direction of the previous TITLE tooltip to linger.) As to nsDocShellTreeOwner.cpp, I checked it and indeed didn't find the function. Someone with cvs write permissions please fix this comment. I don't fix it since it's outside of the scope of this bug.

Comment on attachment 151112 [details] [diff] [review] Updated patch (Mozilla and Firefox) Oh, and almost forgot: The changes aren't relevant to the embedding' version of this code, since GetNodeText simply returns the title. We don't even know what kind of tooltip control the embedder will use.

Comment on attachment 151112 [details] [diff] [review] Updated patch (Mozilla and Firefox) We could still do with some mechanism for embedders to get the direction, but I don't mind that being a separate bug. r=smontagu.

Embedders can get the direction the same way I do it from the JS code: by using the computed style.

(In reply to comment #20) > Embedders can get the direction the same way I do it from the JS code: by using > the computed style. First of all I agree this can be a separate bug, but the point is that embedders should get this working out-of-the-box (Meaning we should patch PaintTitle somewhen). Anyway, this is far better from nothing, and it won't cause regressions when the layout code will be patched, so i guess this OK for now.

Someone needs to kick some Firefox butt and land this.

Actually, I give you sr+ for Mozilla, so you can go ahead and check this into xpfe.

Separate patch, containing only the XPFE part of the previous patch. smontagu, please mark reviewed again. roc, please sr as promised.

Comment on attachment 151112 [details] [diff] [review] Updated patch (Mozilla and Firefox) a=me for firefox, and marking roc's sr+

Comment on attachment 151112 [details] [diff] [review] Updated patch (Mozilla and Firefox) Firefox half checked-in to trunk.

(In reply to comment #26) > (From update of attachment 151112 [details] [diff] [review]) > Firefox half checked-in to trunk. > What about AVIARY 1.0 Branch?

Comment on attachment 151112 [details] [diff] [review] Updated patch (Mozilla and Firefox) a=asa (on behalf of drivers) for checkin to 1.8a2

Comment on attachment 152685 [details] [diff] [review] Same as previous patch, for XPFE only The xpfe patch is already in. So is the firefox one. Currently I'm waiting a week before I check it into aviary as well.

I backed this out of the trunk to fix bug 250825 so we can progress towards shipping 1.8a2.

I haven't been able to reproduce bug 250825, so I can't be sure that this fixes it, but it removes an error from the JS console in some cases.

I was never able to reproduce bug 250825 either. Let's check it in. If there's a bug in some other piece of code which this trivial code exposed, we shall fix the bug in the other piece of code. We'll still be in the 1.8 cycle for quite a while, won't we?

Comment on attachment 233751 [details] [diff] [review] Updated patch for browser I couldn't figure why do we need to null-check defView. But if we do, defualting to "inherit" is wrong because tipNode is a child of the browser window, how about tipElement.ownerDocument.dir ?

Not null-checking defView gives an error in console in the following scenario: 1) click on a link 2) hover over an element in the current page with a title. If you are quick enough, the tooltip will appear before the new page loads 3) when the new page loads, the tooltip remains for a few seconds, and console shows Error: tipElement.ownerDocument.defaultView has no properties

So, after I adopted Mano's suggestion from comment 35, if I follow the scenario in comment 36 and the hovered element is LTR and the new page is RTL or vice versa, the tooltip that appears for a few seconds has the wrong directionality. Even assuming that we care about this case, I'm not sure there's much that we can do about it except by preventing a tooltip from appearing at all in those circumstances, which would be a different bug.

Comment on attachment 234203 [details] [diff] [review] Patch addressing Mano's comments (browser and xpfe) >+ if (defView) { I still don't think we should test this. If (and I can't reproduce the bug) we are firing tooltips on documents without a view (or in other edge cases) then that's a core bug which should be filed separately. sr=me with this fixed.

Comment on attachment 234203 [details] [diff] [review] Patch addressing Mano's comments (browser and xpfe) I would keep the check on defView, esp. if we want this on the branch (I think we do, though I'm not sure how we can be sure it doesn't regress 250825). r=mano, please file a bug on comment 36.

Transferring r+ and sr+ based on comments here and IRC <smontagu> Neil, Mano: any chance I can get a consensus from you on bug 91312, or shall I just check the version Mano likes into toolkit and the version Neil likes into xpfe? <Mano> smontagu: branch version should have this. <Mano> smontagu: i don't mind leaving the error on trunk if you get someone to care about the core bug ;) <smontagu> pace Neil, I'm not so sure it's a bug, in that the view has to die at some point <Mano> smontagu: the issue isn't that defaultView is null. <Mano> smontagu: we shouldn't show the tooltip at this point. <smontagu>Mano: in that case, why not just |if (!defView) return;| ? <Mano> smontagu: maybe, FillInHTMLTooltip != CreateTooltip. <Neil> smontagu: file a bug on the fact that the tooltip can get fired when the view is null, and add an XXX comment to the code referencing the bug before your if (!defView) return;

Checked in.

Not going to block on this release, but once it bakes and is determined to be safe, please nominate as a non-blocker and give us some assessment of risk (what code touches these methods? any edge cases?). It looks solid.