[Webkit-unassigned] [Bug 24187] RTL: tooltip does not get its directionality from its element's directionality

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=24187





------- Comment #12 from xji at chromium.org  2009-06-25 13:13 PDT -------
Hi Darin,

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


(In reply to comment #10)
> (From update of attachment 31868 [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


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.