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

RESOLVED FIXED in mozilla1.8.1

Status

()

RESOLVED FIXED
18 years ago
11 years ago

People

(Reporter: ilya.konstantinov+future, Assigned: ilya.konstantinov+future)

Tracking

Trunk
mozilla1.8.1
Points:
---
Bug Flags:
blocking1.8.1 -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

18 years ago
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
We test for the direction in |nsTextBoxFrame::PaintTitle|, but it looks like
it's never getting set for a popup frame.
Created attachment 42868 [details]
Testcase
hyatt, can you give me some advice here? How can the tooltip inherit the
direction style from its parent element?

Thanks

Comment 4

18 years ago
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? 

Comment 6

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

Comment 7

18 years ago
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. 
QA Contact: giladehven → zach
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?
(Assignee)

Comment 9

17 years ago
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 :)

Updated

17 years ago
Blocks: 115713
(Assignee)

Comment 10

17 years ago
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.
(Assignee)

Comment 12

17 years ago
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.
(Assignee)

Comment 13

15 years ago
Created attachment 151098 [details] [diff] [review]
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?
Assignee: mkaply → mozilla-bugzilla
Status: NEW → ASSIGNED
(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.
(Assignee)

Comment 17

15 years ago
Created attachment 151112 [details] [diff] [review]
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.
Attachment #151098 - Attachment is obsolete: true
(Assignee)

Comment 18

15 years ago
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.
Attachment #151112 - Flags: superreview?(jag)
Attachment #151112 - Flags: review?(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.
Attachment #151112 - Flags: review?(smontagu) → review+
(Assignee)

Comment 20

15 years ago
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.
(Assignee)

Updated

15 years ago
Attachment #151112 - Flags: superreview?(jag) → superreview?(firefox)
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.
(Assignee)

Comment 24

15 years ago
Created attachment 152685 [details] [diff] [review]
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

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

a=me for firefox, and marking roc's sr+
Attachment #151112 - Flags: superreview?(firefox)
Attachment #151112 - Flags: superreview+
Attachment #151112 - Flags: approval1.8a2?

Comment 26

15 years ago
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 28

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

a=asa (on behalf of drivers) for checkin to 1.8a2
Attachment #151112 - Flags: approval1.8a2? → approval1.8a2+

Updated

15 years ago
Attachment #152685 - Flags: review+
(Assignee)

Comment 29

15 years ago
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.
Attachment #152685 - Attachment is obsolete: true
Created attachment 161651 [details] [diff] [review]
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.
(Assignee)

Comment 32

14 years ago
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?
Created attachment 233751 [details] [diff] [review]
Updated patch for browser
Attachment #151112 - Attachment is obsolete: true
Attachment #161651 - Attachment is obsolete: true
Attachment #233751 - Flags: review?(bugs.mano)
Created attachment 233752 [details] [diff] [review]
Updated patch for xpfe
Attachment #233752 - Flags: superreview?(neil)
Attachment #233752 - Flags: review?(neil)
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 ?
Attachment #233751 - Flags: review?(bugs.mano) → review-
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.
Created attachment 234202 [details]
Fancier testcase
Attachment #42868 - Attachment is obsolete: true
Created attachment 234203 [details] [diff] [review]
Patch addressing Mano's comments (browser and xpfe)
Attachment #233751 - Attachment is obsolete: true
Attachment #233752 - Attachment is obsolete: true
Attachment #233752 - Flags: superreview?(neil)
Attachment #233752 - Flags: review?(neil)
Attachment #234203 - Flags: superreview?(neil)
Attachment #234203 - Flags: review?(bugs.mano)

Comment 40

12 years ago
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.
Attachment #234203 - Flags: superreview?(neil) → superreview+
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.
Attachment #234203 - Flags: review?(bugs.mano) → review+
Created attachment 236054 [details] [diff] [review]
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;
Attachment #234203 - Attachment is obsolete: true
Attachment #236054 - Flags: superreview+
Attachment #236054 - Flags: review+

Updated

12 years ago
Flags: blocking1.8.1?
Target Milestone: --- → mozilla1.8.1
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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.
Flags: blocking1.8.1? → blocking1.8.1-

Updated

12 years ago
Attachment #42868 - Attachment mime type: text/html → text/html; charset=windows-1255

Updated

11 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.