Bug 50961 - <title> should support dir attribute
: <title> should support dir attribute
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
http://dev.w3.org/html5/spec/Overview...
:
Depends on:
Blocks: 50910
  Show dependency treegraph
 
Reported: 2010-12-13 13:01 PST by Xiaomei Ji
Modified: 2011-03-31 08:28 PDT (History)
13 users (show)

See Also:


Attachments
Patch (60.56 KB, patch)
2011-03-28 08:26 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (62.94 KB, patch)
2011-03-29 03:38 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (63.36 KB, patch)
2011-03-29 07:12 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (63.60 KB, patch)
2011-03-30 06:05 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (63.70 KB, patch)
2011-03-31 03:12 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Mac build fix (67.10 KB, patch)
2011-03-31 04:24 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (71.62 KB, patch)
2011-03-31 04:43 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (73.34 KB, patch)
2011-03-31 05:33 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (74.91 KB, patch)
2011-03-31 05:41 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (75.55 KB, patch)
2011-03-31 07:23 PDT, Evan Martin
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Patch
Comment 3 Eric Seidel 2011-03-28 08:42:15 PDT
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
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]
Patch
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 2011-03-29 03:53:59 PDT
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.
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]
Patch
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]
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.
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]
Patch
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]
Patch
Comment 25 Eric Seidel 2011-03-31 03:16:38 PDT
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).
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]
Patch
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]
Patch
Comment 33 Evan Martin 2011-03-31 05:41:09 PDT
Created attachment 87705 [details]
Patch
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]
Patch
Comment 36 Eric Seidel 2011-03-31 07:37:08 PDT
Comment on attachment 87722 [details]
Patch

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