Bug 50961 – <title> should support dir attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Xiaomei Ji]

Following is from the spec:

User agents should use the document's title when referring to the document in their user interface. When the contents of a title element are used in this way, the directionality of that title element should be used to set the directionality of the document's title in the user interface.

In addition, http://dev.w3.org/html5/spec/Overview.html#text-rendered-in-native-user-interfaces  requires text from elements generally to be rendered in native user interfaces in a manner that honors the directionality of the element from which the text was obtained, mentioning dialogs, title bars, pop-up menus, and tooltips as particular examples.

[Evan Martin]

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

[Evan Martin]

Created attachment 87148 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 87148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87148&action=review

> Source/WebCore/ChangeLog:11
> +        Introduce a new StringWithDirection object that carries a String along
> +        with the TextDirection associated with the String.  Use this object for
> +        document titles used within WebCore.  Put FIXMEs at the WebKit level to
> +        expose the new direction information to clients.

You might want to explain here that we need to do this so that titles can be stored in History as well as for display of the current page.

> Source/WebCore/dom/Document.cpp:392
> +    , m_title("", LTR)

Just have a default constructor here and then you don't even need these lines.

> Source/WebCore/dom/Document.cpp:1328
> +    m_title = StringWithDirection(canonicalizedTitle(this, m_rawTitle.m_string), m_rawTitle.m_direction);

Why not just pass the StringWithDirection to canonicalizedTitle() and then return one?

> Source/WebCore/dom/Document.cpp:1335
> +    setTitle(StringWithDirection(title, LTR), 0);

You should comment here that this is called by the JavaScript document.title = "" setter and thus we always assume an LTR context.

> Source/WebCore/dom/Document.cpp:1367
> +        static_cast<HTMLTitleElement*>(m_titleElement.get())->setText(m_title.m_string);

You should just add a string() accessor instead of grabbing at m_string.

> Source/WebCore/dom/Document.h:816
> +    void setTitle(const String&);

You should comment here that these two are only used by teh DOM bindings.

> Source/WebCore/dom/Document.h:1300
> +    StringWithDirection m_title;
> +    StringWithDirection m_rawTitle;

It seems that only one of these really needs the direction, or?

> Source/WebCore/html/HTMLTitleElement.cpp:37
> +    , m_title("", LTR)

Default constructor and this goes away. :)

> Source/WebCore/html/HTMLTitleElement.cpp:79
> +StringWithDirection HTMLTitleElement::textWithDirection()

Should this be a const method?

> Source/WebCore/html/HTMLTitleElement.cpp:81
> +    RenderStyle* style = computedStyle();

I believe this can be NULL, do we need to check that?

> Source/WebCore/loader/DocumentLoader.cpp:654
> +    if (title.m_string.isEmpty())

You might add a isEmpty() accessor to StringWithDirection.  Certainly string() woudl be nicer than m_string here. :)

> Source/WebCore/loader/FrameLoader.cpp:615
> +        if (!ptitle.m_string.isNull())

consider adding isNull().

> Source/WebCore/platform/text/StringWithDirection.h:55
> +    TextDirection m_direction;

I think you need to make it clear that this is the directional context of the string.  I worry folks will get confused that this the "direction" of the string.  It could be an LTR context but all hebrew and thus render as RTL, no?

> Source/WebCore/svg/SVGTitleElement.cpp:44
> +        // FIXME: does SVG have a concept of text direction?
> +        document()->setTitle(StringWithDirection(textContent(), LTR), this);

There is "direction", but I don't think it applies to title elements.  http://www.w3.org/TR/SVG/text.html#DirectionProperty

[Early Warning System Bot]

Attachment 87148 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8269969

[Build Bot]

Attachment 87148 [details] did not build on win:
Build output: http://queues.webkit.org/results/8276142

[WebKit Review Bot]

Attachment 87148 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8279122

[Gustavo Noronha (kov)]

Attachment 87148 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8282094

[Evan Martin]

Created attachment 87293 [details]
Patch

[Early Warning System Bot]

Attachment 87293 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8280208

[Eric Seidel (no email)]

Comment on attachment 87293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87293&action=review

Seems OK.

> Source/WebCore/dom/Document.h:816
> +    String title() const { return m_title.string(); }
> +    void setTitle(const String&); // Used by DOM bindings; no direction known.

These two are used by teh DOM bindings, and you could separate them by a newline to indicate that.  But this is also OK.

> Source/WebCore/html/HTMLTitleElement.cpp:37
> +    , m_title("", LTR)

This is no longer needed, right?

> Source/WebCore/loader/FrameLoader.cpp:615
> -        if (!ptitle.isNull())
> +        if (!ptitle.isEmpty())

We decided that the old isNull behavior was in error?

> Source/WebCore/platform/text/StringWithDirection.h:50
> +    StringWithDirection(const String& string, TextDirection dir) : m_string(string), m_direction(dir) {}

Should this have a default parameter for dir?  Then this could be implicitly constructed from string.  i'm not sure if that's good or bad.

[Build Bot]

Attachment 87293 [details] did not build on win:
Build output: http://queues.webkit.org/results/8280245

[Evan Martin]

Created attachment 87312 [details]
Patch

[Evan Martin]

This patch addresses Eric's comments, but I'll probably wait for some of the bots to pass it before I consider committing it.

[Early Warning System Bot]

Attachment 87312 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8281327

[Ryosuke Niwa]

Comment on attachment 87312 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87312&action=review

> Source/WebCore/dom/Document.cpp:1338
> +void Document::setTitle(const StringWithDirection& title, Element* titleElement)

I think this function is a little confusing.  I feel as if this function will modify DOM to have the specified direction in the title.

[Build Bot]

Attachment 87312 [details] did not build on win:
Build output: http://queues.webkit.org/results/8284345

[Build Bot]

Attachment 87312 [details] did not build on win:
Build output: http://queues.webkit.org/results/8283337

[Collabora GTK+ EWS bot]

Attachment 87312 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8281423

[WebKit Review Bot]

Attachment 87312 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8285414

[Evan Martin]

Created attachment 87512 [details]
Patch

[Early Warning System Bot]

Attachment 87512 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8282861

[Build Bot]

Attachment 87512 [details] did not build on win:
Build output: http://queues.webkit.org/results/8285826

[WebKit Review Bot]

Attachment 87512 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8305066

[Evan Martin]

Created attachment 87687 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 87687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87687&action=review

This looks good to me.  I suspect WebKit-level folks may have opinions.  You should consider emailing webkit-dev once this lands and encouraging WebKit-level hackers to respect title directionality on their ports.

> Source/WebCore/html/HTMLTitleElement.cpp:84
> +    if (RenderStyle* style = computedStyle())
> +        direction = style->direction();
> +    else if (RefPtr<RenderStyle> style = styleForRenderer())
> +        direction = style->direction();

Sad that we don't have a reliable way to get a style, always.  (Not your fault of course).

[Early Warning System Bot]

Attachment 87687 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8315083

[Build Bot]

Attachment 87687 [details] did not build on win:
Build output: http://queues.webkit.org/results/8314211

[Ryosuke Niwa]

Created attachment 87695 [details]
Mac build fix

[Evan Martin]

Created attachment 87699 [details]
Patch

[Early Warning System Bot]

Attachment 87699 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8309306

[Build Bot]

Attachment 87699 [details] did not build on win:
Build output: http://queues.webkit.org/results/8306310

[Evan Martin]

Created attachment 87704 [details]
Patch

[Evan Martin]

Created attachment 87705 [details]
Patch

[Build Bot]

Attachment 87705 [details] did not build on win:
Build output: http://queues.webkit.org/results/8306320

[Evan Martin]

Created attachment 87722 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 87722 [details]
Patch

Still looks fine.

[Evan Martin]

Committed r82580: <http://trac.webkit.org/changeset/82580>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82580 might have broken GTK Linux 32-bit Release and Qt Linux Release minimal