Bug 24187 – RTL: tooltip does not get its directionality from its element's directionality

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Xiaomei Ji]

1. Open the attached test file

2. Hover over the red span. 
the tooltip reads (visually) "שלום!". What expected is "!שלום"

[Xiaomei Ji]

Created attachment 28009 [details]
tooltip directionality test case

[Xiaomei Ji]

Related Chrome bug:
http://code.google.com/p/chromium/issues/detail?id=5996

Note that the issue is not as severe in Mac OS X because AppKit uses the "natural writing direction" for help tags (so it actually behaves correctly with the attached test case).

[Xiaomei Ji]

The following patch in HitTestResult::title() seems fixes the bug, but I am not sure whether this is the right place to fix it and whether this is the right fix.

Please advice.


Index: HitTestResult.cpp
===================================================================
--- HitTestResult.cpp   (revision 43938)
+++ HitTestResult.cpp   (working copy)
@@ -21,6 +21,7 @@
 #include "config.h"
 #include "HitTestResult.h"

+#include "CharacterNames.h"
 #include "Frame.h"
 #include "FrameTree.h"
 #include "HTMLAnchorElement.h"
@@ -180,8 +181,24 @@
     for (Node* titleNode = m_innerNode.get(); titleNode; titleNode = titleNode-
>parentNode()) {
         if (titleNode->isElementNode()) {
             String title = static_cast<Element*>(titleNode)->title();
-            if (!title.isEmpty())
-                return title;
+            if (!title.isEmpty()) {
+                String displayTitle;
+                const RenderObject* renderer = titleNode->renderer();
+                if (!renderer)
+                    return title;
+                if (renderer->style()->direction() == LTR) {
+                    // This node is a LTR node.
+                    displayTitle.append(leftToRightEmbed);
+                    displayTitle.append(title);
+                    displayTitle.append(popDirectionalFormatting);
+                } else {
+                    // This node is an RTL node.
+                    displayTitle.append(rightToLeftEmbed);
+                    displayTitle.append(title);
+                    displayTitle.append(popDirectionalFormatting);
+                }
+                return displayTitle;
+            }
         }
     }
     return String();

[Darin Adler]

What’s our usual approach for RTL text in other functions that return plain text? Do we use LTR/RTL override characters in other functions? Do we need a raw version of the title too, in addition to the “display” version of the title?

I don’t think we handle directionality in any other API right now. I am concerned that in-band signaling using Unicode control characters may not work best for all clients. An alternative approach would be to retain the plain-text title API and add an API for returning the directionality. Clients could call that if needed and use whatever mechanism they want (e.g. if their text system supports attributes, use attributes instead of control characters).

[Darin Adler]

(In reply to comment #6)
> I don’t think we handle directionality in any other API right now. I am
> concerned that in-band signaling using Unicode control characters may not work
> best for all clients. An alternative approach would be to retain the plain-text
> title API and add an API for returning the directionality. Clients could call
> that if needed and use whatever mechanism they want (e.g. if their text system
> supports attributes, use attributes instead of control characters).

That approach sounds good to me. Where are the other APIs that currently return only text but should perhaps have a way to return the directionality too?

[Xiaomei Ji]

Thanks Mitz and Darin for your quick and helpful comments. I am fixing it as what Mitz suggested in comment #6.

[Xiaomei Ji]

Created attachment 31868 [details]
patch w/o layout test

I do not know how to create a layout test for the patch.
Since the patch does not change any layout explicitly, the test should be testing HitTestResult::spellingToolTipDirection() and HitTestResult::titleDirection().

[Darin Adler]

Comment on attachment 31868 [details]
patch w/o layout test

Looks like a step in the right "direction". I see a few problems.

> +    TextDirection toolTipDirection = LTR;
>      // First priority is a potential toolTip representing a spelling or grammar error
>      String toolTip = result.spellingToolTip();
> +    toolTipDirection = result.spellingToolTipDirection();

There's no need to initialize toolTipDirection twice here. You should just define it where it's initialized instead of first setting it LTR two lines before.

>                          toolTip = form->action();
> +                        if (form->renderer())
> +                            toolTipDirection = form->renderer()->style()->direction();

It seems strange to leave toolTipDirection set to result.spellingToolTipDirection() if there is a form with no renderer. I suggest setting toolTipDirection to LTR in that case, or not setting the toolTip at all in that case.

> -        if (toolTip.isEmpty())
> +        if (toolTip.isEmpty()) {
>              // FIXME: Need to pass this URL through userVisibleString once that's in WebCore
>              toolTip = result.absoluteLinkURL().string();
> +            // URL always display as LTR, no need to reset toolTipDirectionality.
> +        }

Comment is fine, but code is wrong. toolTipDirection will be result.spellingToolTipDirection(), not LTR.

>                          toolTip = String::adopt(names);
> +                        // FIXME: is filename always LTR?

Again, this leaves toolTipDirection set to result.spellingToolTipDirection(), not LTR.

> +    return (m_innerNonSharedNode->renderer()) ?
> +               (m_innerNonSharedNode->renderer()->style()->direction()) : LTR;

The formatting here is a little messy. We don't usually use extra parentheses like the ones here, nor do we indent the way you're indenting here. Maybe use an if statement instead of ? : and a local variable to more consistent with the other early exist in this function.

> +TextDirection HitTestResult::titleDirection() const
> +{
> +    for (Node* titleNode = m_innerNode.get(); titleNode; titleNode = titleNode->parentNode()) {
> +        if (titleNode->isElementNode()) {
> +            String title = static_cast<Element*>(titleNode)->title();
> +            if (!title.isEmpty())
> +                return (titleNode->renderer()) ? (titleNode->renderer()->style()->direction()) : LTR;
> +        }
> +    }
> +    return LTR;
> +}

I don't like the fact that this function repeats the logic about finding title elements; it's really unclear to me that the first element with a non-empty title is the right one to return. Can this share code with HitTestResult::title in some way?

Also, the extra parentheses in the ? : line don't seem good. Maybe put the renderer into a local variable.

> -void WebChromeClient::setToolTip(const String& toolTip)
> +void WebChromeClient::setToolTip(const String& toolTip, WebCore::TextDirection dir)

You need to leave out the name for the TextDirection argument, because otherwise we'll get an unused parameter warning and so this will not compile.

Also, I don't think you need the WebCore:: prefix inside the file because it has using namespace WebCore at the top.

What about other platforms with setToolTip functions? Won't this break the build for those platforms? Please include fixes for that in the patch.

[Xiaomei Ji]

Comment on attachment 31868 [details]
patch w/o layout test

I am removing the review flag since this patch changes the signature of 
virtual void ChromeClient::setToolTip(const String&, TextDirection) = 0,
which requires all client uses this method to change and recompile.

I will upload another patch to keep the signature the same and added another virtual API instead.

[Xiaomei Ji]

Hi Darin,

Thanks a lot for your quick response!
Please see my comments inline.


(In reply to comment #10)
> (From update of attachment 31868 [details] [review])
> Looks like a step in the right "direction". I see a few problems.
> 
> > +    TextDirection toolTipDirection = LTR;
> >      // First priority is a potential toolTip representing a spelling or grammar error
> >      String toolTip = result.spellingToolTip();
> > +    toolTipDirection = result.spellingToolTipDirection();
> 
> There's no need to initialize toolTipDirection twice here. You should just
> define it where it's initialized instead of first setting it LTR two lines
> before.

Yes, you are right.
I will remove the 1st assignment.

> 
> >                          toolTip = form->action();
> > +                        if (form->renderer())
> > +                            toolTipDirection = form->renderer()->style()->direction();
> 
> It seems strange to leave toolTipDirection set to
> result.spellingToolTipDirection() if there is a form with no renderer. I
> suggest setting toolTipDirection to LTR in that case, or not setting the
> toolTip at all in that case.

This is only called when toolTip.isEmpty() after result.spellingToolTipDirection(), in which case, the toolTipDirection should be set to LTR in result.spellingToolTipDirection(). But setting it explicitly as LTR is a good idea.

> 
> > -        if (toolTip.isEmpty())
> > +        if (toolTip.isEmpty()) {
> >              // FIXME: Need to pass this URL through userVisibleString once that's in WebCore
> >              toolTip = result.absoluteLinkURL().string();
> > +            // URL always display as LTR, no need to reset toolTipDirectionality.
> > +        }
> 
> Comment is fine, but code is wrong. toolTipDirection will be
> result.spellingToolTipDirection(), not LTR.

This is only called when the toolTip.isEmpty() after result.spellingToolTipDirection() and the "form", in which case, I think the direction should be LTR. But I will set the direction explicitly as LTR.

> 
> >                          toolTip = String::adopt(names);
> > +                        // FIXME: is filename always LTR?
> 
> Again, this leaves toolTipDirection set to result.spellingToolTipDirection(),
> not LTR.

Same as above.

> 
> > +    return (m_innerNonSharedNode->renderer()) ?
> > +               (m_innerNonSharedNode->renderer()->style()->direction()) : LTR;
> 
> The formatting here is a little messy. We don't usually use extra parentheses
> like the ones here, nor do we indent the way you're indenting here. Maybe use
> an if statement instead of ? : and a local variable to more consistent with the
> other early exist in this function.

I will change it to local variable + "if" statement.

> 
> > +TextDirection HitTestResult::titleDirection() const
> > +{
> > +    for (Node* titleNode = m_innerNode.get(); titleNode; titleNode = titleNode->parentNode()) {
> > +        if (titleNode->isElementNode()) {
> > +            String title = static_cast<Element*>(titleNode)->title();
> > +            if (!title.isEmpty())
> > +                return (titleNode->renderer()) ? (titleNode->renderer()->style()->direction()) : LTR;
> > +        }
> > +    }
> > +    return LTR;
> > +}
> 
> I don't like the fact that this function repeats the logic about finding title
> elements; it's really unclear to me that the first element with a non-empty
> title is the right one to return. 

I do not know this part, I am following the logic in title().

> Can this share code with HitTestResult::title
> in some way?

I will merge these 2 functions into 1 and use "direction" as OUT parameter.
String HitTestResult::title(TextDirection&) const;
Same for spellingToolTip().



> 
> Also, the extra parentheses in the ? : line don't seem good. Maybe put the
> renderer into a local variable.

I will do that.

> 
> > -void WebChromeClient::setToolTip(const String& toolTip)
> > +void WebChromeClient::setToolTip(const String& toolTip, WebCore::TextDirection dir)
> 
> You need to leave out the name for the TextDirection argument, because
> otherwise we'll get an unused parameter warning and so this will not compile.

I will do that.

> 
> Also, I don't think you need the WebCore:: prefix inside the file because it
> has using namespace WebCore at the top.

This one is under Webkit/mac/WebCoreSupport. 
Seems there is no "using WebCore" at the top. I will double check and remove it if correct.


> 
> What about other platforms with setToolTip functions? Won't this break the
> build for those platforms? Please include fixes for that in the patch.
> 

I did not realize you are this quick!
I tried the approach in Chrome before uploading the patch, and it worked. But I neglected the fact that the Chrome side code *has to* be changed/re-compiled after pick up this webkit patch. I was removing the review flag almost at the same time when you submitted the review feedback.

Looks like with this patch, there is no way for webkit to avoid the recompilation of other platforms.

So, I am thinking, instead of changing the signature of WebCore::ChromeClient::setToolTip(), I need to leave it as is and add a non-pure empty virtual function setToolTipDirection() in  WebCore::ChromeClient for tooltip direction, so, other  platforms do not have to recompile after pick up the webkit change. But the client wanting to handle the direction needs to get the direction inside its setToolTip() to display it correctly.

So, the change would be:
1. in ChromeClent.h
virtual void setToolTipDirection(TextDirection dir) { };

2. in Chrome.cpp, at the very end:
m_client->setToolTipDirection(toolTipDirection);
m_client->setToolTip();

3. changes in EmptyClient and WebChromeClient (because of ChromeClient::setToolTip() signagure change) wont be needed anymore

4. In client side, for example Chrome, maybe need to have a private data member to save the tooltip direction, and a corresponding getter. (I am not sure whether such data member and getter function should be in WebCore::ChromeClient or in client.)

Or maybe some other better ways. What do you think?

Thanks,
Xiaomei

[Darin Adler]

(In reply to comment #12)
> I tried the approach in Chrome before uploading the patch, and it worked. But I
> neglected the fact that the Chrome side code *has to* be changed/re-compiled
> after pick up this webkit patch. I was removing the review flag almost at the
> same time when you submitted the review feedback.
> 
> Looks like with this patch, there is no way for webkit to avoid the
> recompilation of other platforms.

There's no need to avoid this. All these platforms are inside WebKit for a reason. We can change them all at once. It does make it harder to test the fix, but as long as your patch changes all platforms it's completely legitimate to change this internal interface to add the direction argument.

You just need to actually change them all.

[Xiaomei Ji]

Yes, you are right.
I forgot the overload part in each client.

Thanks,
Xiaomei 

(In reply to comment #13)
> (In reply to comment #12)
> > I tried the approach in Chrome before uploading the patch, and it worked. But I
> > neglected the fact that the Chrome side code *has to* be changed/re-compiled
> > after pick up this webkit patch. I was removing the review flag almost at the
> > same time when you submitted the review feedback.
> > 
> > Looks like with this patch, there is no way for webkit to avoid the
> > recompilation of other platforms.
> 
> There's no need to avoid this. All these platforms are inside WebKit for a
> reason. We can change them all at once. It does make it harder to test the fix,
> but as long as your patch changes all platforms it's completely legitimate to
> change this internal interface to add the direction argument.
> 
> You just need to actually change them all.
> 

[Xiaomei Ji]

Created attachment 31953 [details]
patch w/o layout test (version 2)

[Darin Adler]

Comment on attachment 31953 [details]
patch w/o layout test (version 2)

> +    RenderObject* renderer = m_innerNonSharedNode->renderer();
> +    if (renderer)
> +        dir = renderer->style()->direction();

Could have defined the variable inside the if statement.

> +                RenderObject* renderer = titleNode->renderer();
> +                if (renderer)
> +                    dir = renderer->style()->direction();

Ditto.

> +    }
> +    else if (isEqual(WebElementTitleKey, key)) {

In WebKit coding style, the else goes on the same line as the brace.

> +void WebChromeClient::setToolTip(const String& toolTip, TextDirection /* dir */)

We normally omit the name entirely rather than having it included but commented out.

r=me

[Xiaomei Ji]

Created attachment 32727 [details]
updated patch for landing

I am uploading this updated patch for the convenience of landing.
The only differences between this updated patch and the patch Darin approved are those style changes per Darin's suggestion in comment #16.
Build succeed and test without extra diff.

[Xiaomei Ji]

Hi Darin,

Thanks for your review and your comments, appreciated!

Xiaomei

(In reply to comment #16)
> (From update of attachment 31953 [details])
> > +    RenderObject* renderer = m_innerNonSharedNode->renderer();
> > +    if (renderer)
> > +        dir = renderer->style()->direction();
> 
> Could have defined the variable inside the if statement.
> 
> > +                RenderObject* renderer = titleNode->renderer();
> > +                if (renderer)
> > +                    dir = renderer->style()->direction();
> 
> Ditto.
> 
> > +    }
> > +    else if (isEqual(WebElementTitleKey, key)) {
> 
> In WebKit coding style, the else goes on the same line as the brace.
> 
> > +void WebChromeClient::setToolTip(const String& toolTip, TextDirection /* dir */)
> 
> We normally omit the name entirely rather than having it included but commented
> out.
> 
> r=me

[Xiaomei Ji]

Hi Darin,

A question about the testing:

I do not know how to create a layout test for the patch.
Since the patch does not change any layout explicitly, the test should be
testing HitTestResult::spellingToolTip() and
HitTestResult::title() or ... ?

Thanks,
Xiaomei

[Darin Adler]

(In reply to comment #19)
> I do not know how to create a layout test for the patch.
> Since the patch does not change any layout explicitly, the test should be
> testing HitTestResult::spellingToolTip() and
> HitTestResult::title() or ... ?

To create a regression test for this, we need to somehow expose the information to the DOM; that's not terribly difficult to do, but it would require adding code to DumpRenderTree. In some cases like this, though, we don't include a regression test.

And you'd want to test all of the code paths, both title and spelling tool tip.

[Adam Barth]

Comment on attachment 31953 [details]
patch w/o layout test (version 2)

The real patch is the other one.

[Adam Barth]

Will land.

[Adam Barth]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.base.exp
	M	WebCore/loader/EmptyClients.h
	M	WebCore/page/Chrome.cpp
	M	WebCore/page/ChromeClient.h
	M	WebCore/page/chromium/ChromeClientChromium.h
	M	WebCore/rendering/HitTestResult.cpp
	M	WebCore/rendering/HitTestResult.h
	M	WebKit/gtk/ChangeLog
	M	WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp
	M	WebKit/gtk/WebCoreSupport/ChromeClientGtk.h
	M	WebKit/mac/ChangeLog
	M	WebKit/mac/Misc/WebElementDictionary.mm
	M	WebKit/mac/WebCoreSupport/WebChromeClient.h
	M	WebKit/mac/WebCoreSupport/WebChromeClient.mm
	M	WebKit/qt/ChangeLog
	M	WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
	M	WebKit/qt/WebCoreSupport/ChromeClientQt.h
	M	WebKit/win/ChangeLog
	M	WebKit/win/WebCoreSupport/WebChromeClient.cpp
	M	WebKit/win/WebCoreSupport/WebChromeClient.h
	M	WebKit/win/WebElementPropertyBag.cpp
	M	WebKit/wx/ChangeLog
	M	WebKit/wx/WebKitSupport/ChromeClientWx.cpp
	M	WebKit/wx/WebKitSupport/ChromeClientWx.h
Committed r45991
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/Configurations/FeatureDefines.xcconfig
r45990 = 6904d0a9e48800e162225ff7715d2fc1828d0a54 (trunk)
	M	WebKit/mac/WebCoreSupport/WebChromeClient.mm
	M	WebKit/mac/WebCoreSupport/WebChromeClient.h
	M	WebKit/mac/ChangeLog
	M	WebKit/mac/Misc/WebElementDictionary.mm
	M	WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
	M	WebKit/qt/WebCoreSupport/ChromeClientQt.h
	M	WebKit/qt/ChangeLog
	M	WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp
	M	WebKit/gtk/WebCoreSupport/ChromeClientGtk.h
	M	WebKit/gtk/ChangeLog
	M	WebKit/win/ChangeLog
	M	WebKit/win/WebCoreSupport/WebChromeClient.cpp
	M	WebKit/win/WebCoreSupport/WebChromeClient.h
	M	WebKit/win/WebElementPropertyBag.cpp
	M	WebKit/wx/WebKitSupport/ChromeClientWx.h
	M	WebKit/wx/WebKitSupport/ChromeClientWx.cpp
	M	WebKit/wx/ChangeLog
	M	WebCore/ChangeLog
	M	WebCore/WebCore.base.exp
	M	WebCore/page/chromium/ChromeClientChromium.h
	M	WebCore/page/ChromeClient.h
	M	WebCore/page/Chrome.cpp
	M	WebCore/rendering/HitTestResult.h
	M	WebCore/rendering/HitTestResult.cpp
	M	WebCore/loader/EmptyClients.h
r45991 = 9b343eeb6e42f7d0e50dbbfccd49ede75f215f37 (trunk)
First, rewinding head to replay your work on top of it...
Nothing to do.
http://trac.webkit.org/changeset/45991

[Xiaomei Ji]

Thanks for landing!


(In reply to comment #23)
> Committing to http://svn.webkit.org/repository/webkit/trunk ...
>     M    WebCore/ChangeLog
>     M    WebCore/WebCore.base.exp
>     M    WebCore/loader/EmptyClients.h
>     M    WebCore/page/Chrome.cpp
>     M    WebCore/page/ChromeClient.h
>     M    WebCore/page/chromium/ChromeClientChromium.h
>     M    WebCore/rendering/HitTestResult.cpp
>     M    WebCore/rendering/HitTestResult.h
>     M    WebKit/gtk/ChangeLog
>     M    WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp
>     M    WebKit/gtk/WebCoreSupport/ChromeClientGtk.h
>     M    WebKit/mac/ChangeLog
>     M    WebKit/mac/Misc/WebElementDictionary.mm
>     M    WebKit/mac/WebCoreSupport/WebChromeClient.h
>     M    WebKit/mac/WebCoreSupport/WebChromeClient.mm
>     M    WebKit/qt/ChangeLog
>     M    WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
>     M    WebKit/qt/WebCoreSupport/ChromeClientQt.h
>     M    WebKit/win/ChangeLog
>     M    WebKit/win/WebCoreSupport/WebChromeClient.cpp
>     M    WebKit/win/WebCoreSupport/WebChromeClient.h
>     M    WebKit/win/WebElementPropertyBag.cpp
>     M    WebKit/wx/ChangeLog
>     M    WebKit/wx/WebKitSupport/ChromeClientWx.cpp
>     M    WebKit/wx/WebKitSupport/ChromeClientWx.h
> Committed r45991
>     M    JavaScriptCore/ChangeLog
>     M    JavaScriptCore/Configurations/FeatureDefines.xcconfig
> r45990 = 6904d0a9e48800e162225ff7715d2fc1828d0a54 (trunk)
>     M    WebKit/mac/WebCoreSupport/WebChromeClient.mm
>     M    WebKit/mac/WebCoreSupport/WebChromeClient.h
>     M    WebKit/mac/ChangeLog
>     M    WebKit/mac/Misc/WebElementDictionary.mm
>     M    WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
>     M    WebKit/qt/WebCoreSupport/ChromeClientQt.h
>     M    WebKit/qt/ChangeLog
>     M    WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp
>     M    WebKit/gtk/WebCoreSupport/ChromeClientGtk.h
>     M    WebKit/gtk/ChangeLog
>     M    WebKit/win/ChangeLog
>     M    WebKit/win/WebCoreSupport/WebChromeClient.cpp
>     M    WebKit/win/WebCoreSupport/WebChromeClient.h
>     M    WebKit/win/WebElementPropertyBag.cpp
>     M    WebKit/wx/WebKitSupport/ChromeClientWx.h
>     M    WebKit/wx/WebKitSupport/ChromeClientWx.cpp
>     M    WebKit/wx/ChangeLog
>     M    WebCore/ChangeLog
>     M    WebCore/WebCore.base.exp
>     M    WebCore/page/chromium/ChromeClientChromium.h
>     M    WebCore/page/ChromeClient.h
>     M    WebCore/page/Chrome.cpp
>     M    WebCore/rendering/HitTestResult.h
>     M    WebCore/rendering/HitTestResult.cpp
>     M    WebCore/loader/EmptyClients.h
> r45991 = 9b343eeb6e42f7d0e50dbbfccd49ede75f215f37 (trunk)
> First, rewinding head to replay your work on top of it...
> Nothing to do.
> http://trac.webkit.org/changeset/45991

[Xiaomei Ji]

Created attachment 32906 [details]
patch to fix compilation error in gtk and qt

Comment on attachment 32906 [details]
patch to fix compilation error in gtk and qt

Build fixes like this do not require a review (although it is okay to request a review).

Build fix committed as <http://trac.webkit.org/changeset/46000>.

[Aharon (Vladimir) Lanin]

I am not sure that I have seen the final version of the change, but I have a concern, if the latest logic is still to always wrap in either LRE or RLE (and then PDF). It would be best not to wrap when the intended directionality of the title is the same as the browser's directionality (and the wrapping simply isn't needed). My concern is that the LRE and PDF characters could be displayed as funny little graphics characters on Windows systems without RTL script support enabled (i.e. 99% of the Windows systems in the world - but probably not the systems of the developers on this thread! :-) for your normal garden variety purely LTR pages.