[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


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #31868|review?                     |review-
               Flag|                            |




------- Comment #10 from darin at apple.com  2009-06-25 11:53 PDT -------
(From update of attachment 31868)
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.


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