Bug 50961

Summary:

<title> should support dir attribute

Product:

WebKit

Reporter:

Xiaomei Ji <xji>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, aharon, buildbot, eric, evan, gustavo.noronha, gustavo, mitz, playmobil, rniwa, webkit-ews, webkit.review.bot, xan.lopez

Priority:

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

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Mac build fix	none
Patch	none
Patch	none
Patch	none
Patch	eric: review+

Xiaomei Ji

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

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
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Evan Martin

Comment 1 2011-03-27 02:16:57 PDT

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

Evan Martin

Comment 2 2011-03-28 08:26:42 PDT

Created attachment 87148 [details] Patch

Eric Seidel (no email)

Comment 3 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

Early Warning System Bot

Comment 4 2011-03-28 08:43:21 PDT

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

Build Bot

Comment 5 2011-03-28 08:58:46 PDT

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

WebKit Review Bot

Comment 6 2011-03-28 12:51:36 PDT

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

Gustavo Noronha (kov)

Comment 7 2011-03-28 23:43:12 PDT

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

Evan Martin

Comment 8 2011-03-29 03:38:42 PDT

Created attachment 87293 [details] Patch

Early Warning System Bot

Comment 9 2011-03-29 03:53:37 PDT

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

Eric Seidel (no email)

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

Build Bot

Comment 11 2011-03-29 04:52:15 PDT

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

Evan Martin

Comment 12 2011-03-29 07:12:11 PDT

Created attachment 87312 [details] Patch

Evan Martin

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

Early Warning System Bot

Comment 14 2011-03-29 07:30:16 PDT

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

Ryosuke Niwa

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

Build Bot

Comment 16 2011-03-29 08:23:17 PDT

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

Build Bot

Comment 17 2011-03-29 08:45:23 PDT

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

Collabora GTK+ EWS bot

Comment 18 2011-03-29 12:02:37 PDT

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

WebKit Review Bot

Comment 19 2011-03-29 12:09:16 PDT

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

Evan Martin

Comment 20 2011-03-30 06:05:26 PDT

Created attachment 87512 [details] Patch

Early Warning System Bot

Comment 21 2011-03-30 06:31:05 PDT

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

Build Bot

Comment 22 2011-03-30 06:52:12 PDT

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

WebKit Review Bot

Comment 23 2011-03-30 09:07:56 PDT

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

Evan Martin

Comment 24 2011-03-31 03:12:34 PDT

Created attachment 87687 [details] Patch

Eric Seidel (no email)

Comment 25 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).

Early Warning System Bot

Comment 26 2011-03-31 03:30:03 PDT

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

Build Bot

Comment 27 2011-03-31 03:42:32 PDT

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

Ryosuke Niwa

Comment 28 2011-03-31 04:24:37 PDT

Created attachment 87695 [details] Mac build fix

Evan Martin

Comment 29 2011-03-31 04:43:25 PDT

Created attachment 87699 [details] Patch