91312 - TITLE texts should follow the same BiDi directionality as the source object

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Ilya Konstantinov]

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).

Comment 1

[Simon Montagu :smontagu]

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

Comment 2

[Simon Montagu :smontagu]

Attachment: Testcase

Comment 3

[Simon Montagu :smontagu]

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

Thanks

Comment 4

[David Hyatt]

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.

Comment 5

[Simon Montagu :smontagu]

I was afraid of that :-/

Does that mean that this bug can't be fixed? 

Comment 6

[David Hyatt]

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

Comment 7

[Zach Lipton [:zach]]

Mass-move all BiDi Hebrew and Arabic qa to me, [e-mail]. 
Thank you Gilad for your service to this component, and best of luck to you 
in the future.

Sholom. 

Comment 8

[Simon Montagu :smontagu]

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?

Comment 9

[Ilya Konstantinov]

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 :)

Comment 10

[Ilya Konstantinov]

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

Comment 11

[Simon Montagu :smontagu]

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.

Comment 12

[Ilya Konstantinov]

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.

Comment 13

[Ilya Konstantinov]

Attachment: 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?

Comment 14

[Asaf Romano (gone)]

(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

Comment 15

[Simon Montagu :smontagu]

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

Comment 16

[Simon Montagu :smontagu]

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

Comment 17

[Ilya Konstantinov]

Attachment: Updated patch (Mozilla and Firefox)

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 18

[Ilya Konstantinov]

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 19

[Simon Montagu :smontagu]

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.

Comment 20

[Ilya Konstantinov]

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

Comment 21

[Asaf Romano (gone)]

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

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Someone needs to kick some Firefox butt and land this.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 24

[Ilya Konstantinov]

Attachment: Same as previous patch, for XPFE only

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

Comment 25

[Benjamin Smedberg]

Comment on attachment 151112 [details] [diff] [review]
Updated patch (Mozilla and Firefox)

a=me for firefox, and marking roc's sr+

Comment 26

[James Ross]

Comment on attachment 151112 [details] [diff] [review]
Updated patch (Mozilla and Firefox)

Firefox half checked-in to trunk.

Comment 27

[Asaf Romano (gone)]

(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 28

[Asa Dotzler [:asa]]

Comment on attachment 151112 [details] [diff] [review]
Updated patch (Mozilla and Firefox)

a=asa (on behalf of drivers) for checkin to 1.8a2

Comment 29

[Ilya Konstantinov]

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.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 31

[Simon Montagu :smontagu]

Attachment: Possible better patch

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.

Comment 32

[Ilya Konstantinov]

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 33

[Simon Montagu :smontagu]

Attachment: Updated patch for browser

Comment 34

[Simon Montagu :smontagu]

Attachment: Updated patch for xpfe

Comment 35

[Asaf Romano (gone)]

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 ?

Comment 36

[Simon Montagu :smontagu]

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

Comment 37

[Simon Montagu :smontagu]

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 38

[Simon Montagu :smontagu]

Attachment: Fancier testcase

Comment 39

[Simon Montagu :smontagu]

Attachment: Patch addressing Mano's comments (browser and xpfe)

Comment 40

[[e-mail]]

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 41

[Asaf Romano (gone)]

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.

Comment 42

[Simon Montagu :smontagu]

Attachment: Patch to satisfy both reviewers' comments

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;

Comment 43

[Simon Montagu :smontagu]

Checked in.

Comment 44

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

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.