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


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-03-27 02:16:57 PST -------
Related Chrome bug: http://code.google.com/p/chromium/issues/detail?id=27094
------- Comment #2 From 2011-03-28 08:26:42 PST -------
Created an attachment (id=87148) [details]
Patch
------- Comment #3 From 2011-03-28 08:42:15 PST -------
(From update of 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 From 2011-03-28 08:43:21 PST -------
Attachment 87148 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8269969
------- Comment #5 From 2011-03-28 08:58:46 PST -------
Attachment 87148 [details] did not build on win:
Build output: http://queues.webkit.org/results/8276142
------- Comment #6 From 2011-03-28 12:51:36 PST -------
Attachment 87148 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8279122
------- Comment #7 From 2011-03-28 23:43:12 PST -------
Attachment 87148 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8282094
------- Comment #8 From 2011-03-29 03:38:42 PST -------
Created an attachment (id=87293) [details]
Patch
------- Comment #9 From 2011-03-29 03:53:37 PST -------
Attachment 87293 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8280208
------- Comment #10 From 2011-03-29 03:53:59 PST -------
(From update of 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 From 2011-03-29 04:52:15 PST -------
Attachment 87293 [details] did not build on win:
Build output: http://queues.webkit.org/results/8280245
------- Comment #12 From 2011-03-29 07:12:11 PST -------
Created an attachment (id=87312) [details]
Patch
------- Comment #13 From 2011-03-29 07:15:46 PST -------
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 From 2011-03-29 07:30:16 PST -------
Attachment 87312 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8281327
------- Comment #15 From 2011-03-29 07:41:09 PST -------
(From update of 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 From 2011-03-29 08:23:17 PST -------
Attachment 87312 [details] did not build on win:
Build output: http://queues.webkit.org/results/8284345
------- Comment #17 From 2011-03-29 08:45:23 PST -------
Attachment 87312 [details] did not build on win:
Build output: http://queues.webkit.org/results/8283337
------- Comment #18 From 2011-03-29 12:02:37 PST -------
Attachment 87312 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8281423
------- Comment #19 From 2011-03-29 12:09:16 PST -------
Attachment 87312 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8285414
------- Comment #20 From 2011-03-30 06:05:26 PST -------
Created an attachment (id=87512) [details]
Patch
------- Comment #21 From 2011-03-30 06:31:05 PST -------
Attachment 87512 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8282861
------- Comment #22 From 2011-03-30 06:52:12 PST -------
Attachment 87512 [details] did not build on win:
Build output: http://queues.webkit.org/results/8285826
------- Comment #23 From 2011-03-30 09:07:56 PST -------
Attachment 87512 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8305066
------- Comment #24 From 2011-03-31 03:12:34 PST -------
Created an attachment (id=87687) [details]
Patch
------- Comment #25 From 2011-03-31 03:16:38 PST -------
(From update of 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 From 2011-03-31 03:30:03 PST -------
Attachment 87687 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8315083
------- Comment #27 From 2011-03-31 03:42:32 PST -------
Attachment 87687 [details] did not build on win:
Build output: http://queues.webkit.org/results/8314211
------- Comment #28 From 2011-03-31 04:24:37 PST -------
Created an attachment (id=87695) [details]
Mac build fix
------- Comment #29 From 2011-03-31 04:43:25 PST -------
Created an attachment (id=87699) [details]
Patch
------- Comment #30 From 2011-03-31 05:09:30 PST -------
Attachment 87699 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8309306
------- Comment #31 From 2011-03-31 05:24:41 PST -------
Attachment 87699 [details] did not build on win:
Build output: http://queues.webkit.org/results/8306310
------- Comment #32 From 2011-03-31 05:33:21 PST -------
Created an attachment (id=87704) [details]
Patch
------- Comment #33 From 2011-03-31 05:41:09 PST -------
Created an attachment (id=87705) [details]
Patch
------- Comment #34 From 2011-03-31 06:13:39 PST -------
Attachment 87705 [details] did not build on win:
Build output: http://queues.webkit.org/results/8306320
------- Comment #35 From 2011-03-31 07:23:42 PST -------
Created an attachment (id=87722) [details]
Patch
------- Comment #36 From 2011-03-31 07:37:08 PST -------
(From update of attachment 87722 [details])
Still looks fine.
------- Comment #37 From 2011-03-31 08:17:24 PST -------
Committed r82580: <http://trac.webkit.org/changeset/82580>
------- Comment #38 From 2011-03-31 08:28:10 PST -------
http://trac.webkit.org/changeset/82580 might have broken GTK Linux 32-bit Release and Qt Linux Release minimal