Bug 50961

Summary: <title> should support dir attribute
Product: WebKit Reporter: Xiaomei Ji <xji>
Component: DOMAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: abarth, aharon, buildbot, eric, evan, gustavo.noronha, gustavo, mitz, playmobil, rniwa, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://dev.w3.org/html5/spec/Overview.html#the-title-element-0
Bug Depends on:    
Bug Blocks: 50910    
Description Flags
Mac build fix
Patch eric: review+

Description Xiaomei Ji 2010-12-13 13:01:45 PST
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.
Comment 1 Evan Martin 2011-03-27 02:16:57 PDT
Related Chrome bug: http://code.google.com/p/chromium/issues/detail?id=27094
Comment 2 Evan Martin 2011-03-28 08:26:42 PDT
Created attachment 87148 [details]
Comment 3 Eric Seidel (no email) 2011-03-28 08:42:15 PDT
Comment on attachment 87148 [details]

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
Comment 4 Early Warning System Bot 2011-03-28 08:43:21 PDT
Attachment 87148 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8269969
Comment 5 Build Bot 2011-03-28 08:58:46 PDT
Attachment 87148 [details] did not build on win:
Build output: http://queues.webkit.org/results/8276142
Comment 6 WebKit Review Bot 2011-03-28 12:51:36 PDT
Attachment 87148 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8279122
Comment 7 Gustavo Noronha (kov) 2011-03-28 23:43:12 PDT
Attachment 87148 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8282094
Comment 8 Evan Martin 2011-03-29 03:38:42 PDT
Created attachment 87293 [details]
Comment 9 Early Warning System Bot 2011-03-29 03:53:37 PDT
Attachment 87293 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8280208
Comment 10 Eric Seidel (no email) 2011-03-29 03:53:59 PDT
Comment on attachment 87293 [details]

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.
Comment 11 Build Bot 2011-03-29 04:52:15 PDT
Attachment 87293 [details] did not build on win:
Build output: http://queues.webkit.org/results/8280245
Comment 12 Evan Martin 2011-03-29 07:12:11 PDT
Created attachment 87312 [details]
Comment 13 Evan Martin 2011-03-29 07:15:46 PDT
This patch addresses Eric's comments, but I'll probably wait for some of the bots to pass it before I consider committing it.
Comment 14 Early Warning System Bot 2011-03-29 07:30:16 PDT
Attachment 87312 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8281327
Comment 15 Ryosuke Niwa 2011-03-29 07:41:09 PDT
Comment on attachment 87312 [details]

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.
Comment 16 Build Bot 2011-03-29 08:23:17 PDT
Attachment 87312 [details] did not build on win:
Build output: http://queues.webkit.org/results/8284345
Comment 17 Build Bot 2011-03-29 08:45:23 PDT
Attachment 87312 [details] did not build on win:
Build output: http://queues.webkit.org/results/8283337
Comment 18 Collabora GTK+ EWS bot 2011-03-29 12:02:37 PDT
Attachment 87312 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8281423
Comment 19 WebKit Review Bot 2011-03-29 12:09:16 PDT
Attachment 87312 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8285414
Comment 20 Evan Martin 2011-03-30 06:05:26 PDT
Created attachment 87512 [details]
Comment 21 Early Warning System Bot 2011-03-30 06:31:05 PDT
Attachment 87512 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8282861
Comment 22 Build Bot 2011-03-30 06:52:12 PDT
Attachment 87512 [details] did not build on win:
Build output: http://queues.webkit.org/results/8285826
Comment 23 WebKit Review Bot 2011-03-30 09:07:56 PDT
Attachment 87512 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8305066
Comment 24 Evan Martin 2011-03-31 03:12:34 PDT
Created attachment 87687 [details]
Comment 25 Eric Seidel (no email) 2011-03-31 03:16:38 PDT
Comment on attachment 87687 [details]

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).
Comment 26 Early Warning System Bot 2011-03-31 03:30:03 PDT
Attachment 87687 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8315083
Comment 27 Build Bot 2011-03-31 03:42:32 PDT
Attachment 87687 [details] did not build on win:
Build output: http://queues.webkit.org/results/8314211
Comment 28 Ryosuke Niwa 2011-03-31 04:24:37 PDT
Created attachment 87695 [details]
Mac build fix
Comment 29 Evan Martin 2011-03-31 04:43:25 PDT
Created attachment 87699 [details]
Comment 30 Early Warning System Bot 2011-03-31 05:09:30 PDT
Attachment 87699 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8309306
Comment 31 Build Bot 2011-03-31 05:24:41 PDT
Attachment 87699 [details] did not build on win:
Build output: http://queues.webkit.org/results/8306310
Comment 32 Evan Martin 2011-03-31 05:33:21 PDT
Created attachment 87704 [details]
Comment 33 Evan Martin 2011-03-31 05:41:09 PDT
Created attachment 87705 [details]
Comment 34 Build Bot 2011-03-31 06:13:39 PDT
Attachment 87705 [details] did not build on win:
Build output: http://queues.webkit.org/results/8306320
Comment 35 Evan Martin 2011-03-31 07:23:42 PDT
Created attachment 87722 [details]
Comment 36 Eric Seidel (no email) 2011-03-31 07:37:08 PDT
Comment on attachment 87722 [details]

Still looks fine.
Comment 37 Evan Martin 2011-03-31 08:17:24 PDT
Committed r82580: <http://trac.webkit.org/changeset/82580>
Comment 38 WebKit Review Bot 2011-03-31 08:28:10 PDT
http://trac.webkit.org/changeset/82580 might have broken GTK Linux 32-bit Release and Qt Linux Release minimal