Closed
Bug 91312
Opened 24 years ago
Closed 18 years ago
TITLE texts should follow the same BiDi directionality as the source object
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla1.8.1
People
(Reporter: ilya.konstantinov+future, Assigned: ilya.konstantinov+future)
References
()
Details
Attachments
(2 files, 8 obsolete files)
1.48 KB,
text/html
|
Details | |
3.42 KB,
patch
|
smontagu
:
review+
smontagu
:
superreview+
|
Details | Diff | Splinter Review |
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).
Updated•24 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Comment 1•24 years ago
|
||
We test for the direction in |nsTextBoxFrame::PaintTitle|, but it looks like
it's never getting set for a popup frame.
Comment 2•24 years ago
|
||
Comment 3•24 years ago
|
||
hyatt, can you give me some advice here? How can the tooltip inherit the
direction style from its parent element?
Thanks
Comment 4•24 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.
Comment 5•24 years ago
|
||
I was afraid of that :-/
Does that mean that this bug can't be fixed?
Comment 6•24 years ago
|
||
We could make title tips examine the directionality of the target and then
dynamically switch directionality for that particular tooltip.
Comment 7•24 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
Comment 8•23 years ago
|
||
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•23 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 :)
Assignee | ||
Comment 10•23 years ago
|
||
Simon, you've figured out why this doesn't work on the artificial example?
Comment 11•23 years ago
|
||
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•23 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•21 years ago
|
||
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
Comment 14•21 years ago
|
||
(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•21 years ago
|
||
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•21 years ago
|
||
... 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•21 years ago
|
||
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•21 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 19•21 years ago
|
||
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•21 years ago
|
||
Embedders can get the direction the same way I do it from the JS code: by using
the computed style.
Comment 21•21 years ago
|
||
(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•21 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•21 years ago
|
||
Separate patch, containing only the XPFE part of the previous patch.
smontagu, please mark reviewed again.
roc, please sr as promised.
Comment 25•21 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•21 years ago
|
||
Comment on attachment 151112 [details] [diff] [review]
Updated patch (Mozilla and Firefox)
Firefox half checked-in to trunk.
Comment 27•21 years ago
|
||
(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?
Updated•21 years ago
|
Flags: blocking-aviary1.0?
Comment 28•21 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•21 years ago
|
Attachment #152685 -
Flags: review+
Assignee | ||
Comment 29•21 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
I backed this out of the trunk to fix bug 250825 so we can progress towards
shipping 1.8a2.
Updated•21 years ago
|
Flags: blocking-aviary1.0?
Comment 31•20 years ago
|
||
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•20 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?
Comment 33•18 years ago
|
||
Attachment #151112 -
Attachment is obsolete: true
Attachment #161651 -
Attachment is obsolete: true
Attachment #233751 -
Flags: review?(bugs.mano)
Comment 34•18 years ago
|
||
Attachment #233752 -
Flags: superreview?(neil)
Attachment #233752 -
Flags: review?(neil)
Comment 35•18 years ago
|
||
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-
Comment 36•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
Attachment #42868 -
Attachment is obsolete: true
Comment 39•18 years ago
|
||
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•18 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 41•18 years ago
|
||
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+
Comment 42•18 years ago
|
||
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•18 years ago
|
Flags: blocking1.8.1?
Target Milestone: --- → mozilla1.8.1
Comment 43•18 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 44•18 years ago
|
||
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•18 years ago
|
Attachment #42868 -
Attachment mime type: text/html → text/html; charset=windows-1255
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.
Description
•