Skip to content

GitLab

T
tangled
Commit eafb309c authored by avi@chromium.org's avatar avi@chromium.org
Browse files
Options

Revert 112983 - Improve RenderTextWin font fallback. 

Breaks GraphemePositions and EditString_ComplexScript on Win XP.

BUG=90426

Review URL: http://codereview.chromium.org/8575020

TBR=asvitkine@chromium.org
Review URL: http://codereview.chromium.org/8802011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@112994 0039d316-1c4b-4281-b951-d872f2087c98
parent 6588d67c
Hide whitespace changes
Inline Side-by-side
Showing with 52 additions and 189 deletions
ui/gfx/render_text.cc
View file @ eafb309c
......@@ -67,9 +67,8 @@ void ApplyStyleRangeImpl(gfx::StyleRanges* style_ranges,
} else if (i->range.end() > new_range.end()) {
i->range.set_start(new_range.end());
break;
} else {
} else
NOTREACHED();
}
}
// Add the new range in its sorted location.
style_ranges->insert(i, style_range);
......@@ -189,8 +188,8 @@ void RenderText::MoveCursorRight(BreakType break_type, bool select) {
MoveCursorTo(position);
}
bool RenderText::MoveCursorTo(const SelectionModel& model) {
SelectionModel sel(model);
bool RenderText::MoveCursorTo(const SelectionModel& selection_model) {
SelectionModel sel(selection_model);
size_t text_length = text().length();
// Enforce valid selection model components.
if (sel.selection_start() > text_length)
......@@ -369,7 +368,7 @@ SelectionModel RenderText::FindCursorPosition(const Point& point) {
// binary searching the cursor position.
// TODO(oshima): use the center of character instead of edge.
// Binary search may not work for language like Arabic.
while (std::abs(right_pos - left_pos) > 1) {
while (std::abs(static_cast<long>(right_pos - left_pos)) > 1) {
int pivot_pos = left_pos + (right_pos - left_pos) / 2;
int pivot = font.GetStringWidth(text().substr(0, pivot_pos));
if (pivot < x) {
......@@ -431,7 +430,8 @@ SelectionModel RenderText::GetLeftSelectionModel(const SelectionModel& current,
BreakType break_type) {
if (break_type == LINE_BREAK)
return LeftEndSelectionModel();
size_t pos = std::max<int>(current.selection_end() - 1, 0);
size_t pos = std::max(static_cast<long>(current.selection_end() - 1),
static_cast<long>(0));
if (break_type == CHARACTER_BREAK)
return SelectionModel(pos, pos, SelectionModel::LEADING);
......@@ -505,7 +505,8 @@ void RenderText::SetSelectionModel(const SelectionModel& model) {
selection_model_.set_selection_start(model.selection_start());
DCHECK_LE(model.selection_end(), text().length());
selection_model_.set_selection_end(model.selection_end());
DCHECK_LT(model.caret_pos(), std::max<size_t>(text().length(), 1));
DCHECK_LT(model.caret_pos(),
std::max(text().length(), static_cast<size_t>(1)));
selection_model_.set_caret_pos(model.caret_pos());
selection_model_.set_caret_placement(model.caret_placement());
......
ui/gfx/render_text.h
View file @ eafb309c
......@@ -234,9 +234,6 @@ class UI_EXPORT RenderText {
virtual void DrawVisualText(Canvas* canvas) = 0;
// Get the logical index of the grapheme preceding the argument |position|.
// If |IsCursorablePosition(position)| is true, the result will be the start
// of the previous grapheme, if any. Otherwise, the result will be the start
// of the grapheme containing |position|.
size_t GetIndexOfPreviousGrapheme(size_t position);
// Apply composition style (underline) to composition range and selection
......@@ -255,13 +252,8 @@ class UI_EXPORT RenderText {
FRIEND_TEST_ALL_PREFIXES(RenderTextTest, CustomDefaultStyle);
FRIEND_TEST_ALL_PREFIXES(RenderTextTest, ApplyStyleRange);
FRIEND_TEST_ALL_PREFIXES(RenderTextTest, StyleRangesAdjust);
FRIEND_TEST_ALL_PREFIXES(RenderTextTest, GraphemePositions);
FRIEND_TEST_ALL_PREFIXES(RenderTextTest, SelectionModels);
// Return an index belonging to the |next| or previous logical grapheme.
// If |next| is false and |IsCursorablePosition(index)| is true, the result
// will be the start of the previous grapheme, if any. Otherwise, the result
// will be the start of the grapheme containing |index|.
// The return value is bounded by 0 and the text length, inclusive.
virtual size_t IndexOfAdjacentGrapheme(size_t index, bool next) = 0;
......
ui/gfx/render_text_linux.cc
View file @ eafb309c
......@@ -284,8 +284,6 @@ void RenderTextLinux::DrawVisualText(Canvas* canvas) {
}
size_t RenderTextLinux::IndexOfAdjacentGrapheme(size_t index, bool next) {
if (index > text().length())
return text().length();
EnsureLayout();
return Utf16IndexOfAdjacentGrapheme(Utf16IndexToUtf8Index(index), next);
}
......
ui/gfx/render_text_unittest.cc
View file @ eafb309c
......@@ -543,105 +543,6 @@ TEST_F(RenderTextTest, MoveCursorLeftRight_ComplexScript) {
}
#endif
TEST_F(RenderTextTest, GraphemePositions) {
// LTR 2-character grapheme, LTR abc, LTR 2-character grapheme.
const string16 kText1 = WideToUTF16(L"\x0915\x093f"L"abc"L"\x0915\x093f");
// LTR ab, LTR 2-character grapheme, LTR cd.
const string16 kText2 = WideToUTF16(L"ab"L"\x0915\x093f"L"cd");
struct {
string16 text;
size_t index;
size_t expected_previous;
size_t expected_next;
} cases[] = {
{ string16(), 0, 0, 0 },
{ string16(), 1, 0, 0 },
{ string16(), 50, 0, 0 },
{ kText1, 0, 0, 2 },
{ kText1, 1, 0, 2 },
{ kText1, 2, 0, 3 },
{ kText1, 3, 2, 4 },
{ kText1, 4, 3, 5 },
{ kText1, 5, 4, 7 },
{ kText1, 6, 5, 7 },
{ kText1, 7, 5, 7 },
{ kText1, 8, 7, 7 },
{ kText1, 50, 7, 7 },
{ kText2, 0, 0, 1 },
{ kText2, 1, 0, 2 },
{ kText2, 2, 1, 4 },
{ kText2, 3, 2, 4 },
{ kText2, 4, 2, 5 },
{ kText2, 5, 4, 6 },
{ kText2, 6, 5, 6 },
{ kText2, 7, 6, 6 },
{ kText2, 50, 6, 6 },
};
scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {
render_text->SetText(cases[i].text);
size_t next = render_text->GetIndexOfNextGrapheme(cases[i].index);
EXPECT_EQ(cases[i].expected_next, next);
EXPECT_TRUE(render_text->IsCursorablePosition(next));
size_t previous = render_text->GetIndexOfPreviousGrapheme(cases[i].index);
EXPECT_EQ(cases[i].expected_previous, previous);
EXPECT_TRUE(render_text->IsCursorablePosition(previous));
}
}
TEST_F(RenderTextTest, SelectionModels) {
// Simple Latin text.
const string16 kLatin = WideToUTF16(L"abc");
// LTR 2-character grapheme.
const string16 kLTRGrapheme = WideToUTF16(L"\x0915\x093f");
// LTR 2-character grapheme, LTR a, LTR 2-character grapheme.
const string16 kHindiLatin = WideToUTF16(L"\x0915\x093f"L"a"L"\x0915\x093f");
// RTL 2-character grapheme.
const string16 kRTLGrapheme = WideToUTF16(L"\x05e0\x05b8");
// RTL 2-character grapheme, LTR a, RTL 2-character grapheme.
const string16 kHebrewLatin = WideToUTF16(L"\x05e0\x05b8"L"a"L"\x05e0\x05b8");
struct {
string16 text;
size_t expected_left_end_caret;
SelectionModel::CaretPlacement expected_left_end_placement;
size_t expected_right_end_caret;
SelectionModel::CaretPlacement expected_right_end_placement;
} cases[] = {
{ string16(), 0, SelectionModel::LEADING, 0, SelectionModel::LEADING },
{ kLatin, 0, SelectionModel::LEADING, 2, SelectionModel::TRAILING },
{ kLTRGrapheme, 0, SelectionModel::LEADING, 0, SelectionModel::TRAILING },
{ kHindiLatin, 0, SelectionModel::LEADING, 3, SelectionModel::TRAILING },
{ kRTLGrapheme, 0, SelectionModel::TRAILING, 0, SelectionModel::LEADING },
#if defined(OS_LINUX)
// On Linux, the whole string is displayed RTL, rather than individual runs.
{ kHebrewLatin, 3, SelectionModel::TRAILING, 0, SelectionModel::LEADING },
#else
{ kHebrewLatin, 0, SelectionModel::TRAILING, 3, SelectionModel::LEADING },
#endif
};
scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {
render_text->SetText(cases[i].text);
SelectionModel model = render_text->LeftEndSelectionModel();
EXPECT_EQ(cases[i].expected_left_end_caret, model.caret_pos());
EXPECT_TRUE(render_text->IsCursorablePosition(model.caret_pos()));
EXPECT_EQ(cases[i].expected_left_end_placement, model.caret_placement());
model = render_text->RightEndSelectionModel();
EXPECT_EQ(cases[i].expected_right_end_caret, model.caret_pos());
EXPECT_TRUE(render_text->IsCursorablePosition(model.caret_pos()));
EXPECT_EQ(cases[i].expected_right_end_placement, model.caret_placement());
}
}
TEST_F(RenderTextTest, MoveCursorLeftRightWithSelection) {
scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
render_text->SetText(WideToUTF16(L"abc\x05d0\x05d1\x05d2"));
......
ui/gfx/render_text_win.cc
View file @ eafb309c
......@@ -291,15 +291,10 @@ SelectionModel RenderTextWin::LeftEndSelectionModel() {
EnsureLayout();
size_t cursor = base::i18n::IsRTL() ? text().length() : 0;
internal::TextRun* run = runs_[visual_to_logical_[0]];
size_t caret;
SelectionModel::CaretPlacement placement;
if (run->script_analysis.fRTL) {
caret = IndexOfAdjacentGrapheme(run->range.end(), false);
placement = SelectionModel::TRAILING;
} else {
caret = run->range.start();
placement = SelectionModel::LEADING;
}
bool rtl = run->script_analysis.fRTL;
size_t caret = rtl ? run->range.end() - 1 : run->range.start();
SelectionModel::CaretPlacement placement =
rtl ? SelectionModel::TRAILING : SelectionModel::LEADING;
return SelectionModel(cursor, caret, placement);
}
......@@ -310,15 +305,10 @@ SelectionModel RenderTextWin::RightEndSelectionModel() {
EnsureLayout();
size_t cursor = base::i18n::IsRTL() ? 0 : text().length();
internal::TextRun* run = runs_[visual_to_logical_[runs_.size() - 1]];
size_t caret;
SelectionModel::CaretPlacement placement;
if (run->script_analysis.fRTL) {
caret = run->range.start();
placement = SelectionModel::LEADING;
} else {
caret = IndexOfAdjacentGrapheme(run->range.end(), false);
placement = SelectionModel::TRAILING;
}
bool rtl = run->script_analysis.fRTL;
size_t caret = rtl ? run->range.start() : run->range.end() - 1;
SelectionModel::CaretPlacement placement =
rtl ? SelectionModel::LEADING : SelectionModel::TRAILING;
return SelectionModel(cursor, caret, placement);
}
......@@ -471,50 +461,22 @@ void RenderTextWin::DrawVisualText(Canvas* canvas) {
size_t RenderTextWin::IndexOfAdjacentGrapheme(size_t index, bool next) {
EnsureLayout();
if (text().empty())
return 0;
if (index >= text().length()) {
if (next || index > text().length()) {
return text().length();
} else {
// The requested |index| is at the end of the text. Use the index of the
// last character to find the grapheme.
index = text().length() - 1;
if (IsCursorablePosition(index))
return index;
}
}
size_t run_index = GetRunContainingPosition(index);
DCHECK(run_index < runs_.size());
internal::TextRun* run = runs_[run_index];
size_t start = run->range.start();
size_t ch = index - start;
internal::TextRun* run = run_index < runs_.size() ? runs_[run_index] : NULL;
int start = run ? run->range.start() : 0;
int length = run ? run->range.length() : text().length();
int ch = index - start;
WORD cluster = run ? run->logical_clusters[ch] : 0;
if (!next) {
// If |ch| is the start of the run, use the preceding run, if any.
if (ch == 0) {
if (run_index == 0)
return 0;
run = runs_[run_index - 1];
start = run->range.start();
ch = run->range.length();
}
// Loop to find the start of the grapheme.
WORD cluster = run->logical_clusters[ch - 1];
do {
ch--;
} while (ch > 0 && run->logical_clusters[ch - 1] == cluster);
} while (ch >= 0 && run && run->logical_clusters[ch] == cluster);
} else {
WORD cluster = run->logical_clusters[ch];
while (ch < run->range.length() && run->logical_clusters[ch] == cluster)
while (ch < length && run && run->logical_clusters[ch] == cluster)
ch++;
}
return start + ch;
return std::max(std::min(ch, length) + start, 0);
}
void RenderTextWin::ItemizeLogicalText() {
......@@ -583,7 +545,6 @@ void RenderTextWin::LayoutVisualText() {
internal::TextRun* run = *run_iter;
size_t run_length = run->range.length();
const wchar_t* run_text = &(text()[run->range.start()]);
bool tried_fallback = false;
// Select the font desired for glyph generation.
SelectObject(hdc, run->font.GetNativeFont());
......@@ -608,15 +569,12 @@ void RenderTextWin::LayoutVisualText() {
if (hr == E_OUTOFMEMORY) {
max_glyphs *= 2;
} else if (hr == USP_E_SCRIPT_NOT_IN_FONT) {
// Only try font fallback if it hasn't yet been attempted for this run.
if (tried_fallback) {
// TODO(msw): Don't use SCRIPT_UNDEFINED. Apparently Uniscribe can
// crash on certain surrogate pairs with SCRIPT_UNDEFINED.
// See https://bugzilla.mozilla.org/show_bug.cgi?id=341500
// And http://maxradi.us/documents/uniscribe/
run->script_analysis.eScript = SCRIPT_UNDEFINED;
break;
}
// TODO(msw): Don't use SCRIPT_UNDEFINED. Apparently Uniscribe can crash
// on certain surrogate pairs with SCRIPT_UNDEFINED.
// See https://bugzilla.mozilla.org/show_bug.cgi?id=341500
// And http://maxradi.us/documents/uniscribe/
if (run->script_analysis.eScript == SCRIPT_UNDEFINED)
break;
// The run's font doesn't contain the required glyphs, use an alternate.
if (ChooseFallbackFont(hdc, run->font, run_text, run_length,
......@@ -625,7 +583,7 @@ void RenderTextWin::LayoutVisualText() {
SelectObject(hdc, run->font.GetNativeFont());
}
tried_fallback = true;
run->script_analysis.eScript = SCRIPT_UNDEFINED;
} else {
break;
}
......
ui/views/controls/textfield/textfield_views_model_unittest.cc
View file @ eafb309c
......@@ -138,7 +138,6 @@ TEST_F(TextfieldViewsModelTest, EditString_SimpleRTL) {
TEST_F(TextfieldViewsModelTest, EditString_ComplexScript) {
TextfieldViewsModel model(NULL);
// Append two Hindi strings.
model.Append(WideToUTF16(L"\x0915\x093f\x0915\x094d\x0915"));
EXPECT_EQ(WideToUTF16(L"\x0915\x093f\x0915\x094d\x0915"),
......@@ -149,8 +148,13 @@ TEST_F(TextfieldViewsModelTest, EditString_ComplexScript) {
model.GetText());
// Check it is not able to place cursor in middle of a grapheme.
// TODO(xji): temporarily disable in platform Win since the complex script
// characters turned into empty square due to font regression. So, not able
// to test 2 characters belong to the same grapheme.
#if defined(OS_LINUX)
model.MoveCursorTo(gfx::SelectionModel(1U));
EXPECT_EQ(0U, model.GetCursorPosition());
#endif
model.MoveCursorTo(gfx::SelectionModel(2U));
EXPECT_EQ(2U, model.GetCursorPosition());
......@@ -181,7 +185,6 @@ TEST_F(TextfieldViewsModelTest, EditString_ComplexScript) {
EXPECT_EQ(WideToUTF16(L"\x0061\x0062\x0915\x0915\x094d\x092e\x094d"),
model.GetText());
model.MoveCursorTo(gfx::SelectionModel(model.GetText().length()));
EXPECT_EQ(model.GetText().length(), model.GetCursorPosition());
EXPECT_TRUE(model.Backspace());
EXPECT_EQ(WideToUTF16(L"\x0061\x0062\x0915\x0915\x094d\x092e"),
model.GetText());
......@@ -192,21 +195,30 @@ TEST_F(TextfieldViewsModelTest, EditString_ComplexScript) {
model.MoveCursorTo(gfx::SelectionModel(0));
EXPECT_EQ(0U, model.GetCursorPosition());
// TODO(xji): temporarily disable in platform Win since the complex script
// characters turned into empty square due to font regression. So, not able
// to test 2 characters belong to the same grapheme.
#if defined(OS_LINUX)
model.MoveCursorTo(gfx::SelectionModel(1));
EXPECT_EQ(0U, model.GetCursorPosition());
#endif
model.MoveCursorTo(gfx::SelectionModel(2));
EXPECT_EQ(2U, model.GetCursorPosition());
model.MoveCursorTo(gfx::SelectionModel(3));
EXPECT_EQ(3U, model.GetCursorPosition());
// TODO(asvitkine): Temporarily disable the following check on Windows. It
// seems Windows treats "\x0D38\x0D4D\x0D15" as a single grapheme.
#if !defined(OS_WIN)
model.MoveCursorTo(gfx::SelectionModel(2));
EXPECT_EQ(2U, model.GetCursorPosition());
EXPECT_TRUE(model.Backspace());
EXPECT_EQ(WideToUTF16(L"\x0D38\x0D15\x0D16\x0D2E"), model.GetText());
#endif
// Test Delete/Backspace on Hebrew with non-spacing marks.
// TODO(xji): temporarily disable in platform Win since the complex script
// characters turned into empty square due to font regression. So, not able
// to test 2 characters belong to the same grapheme.
#if defined(OS_LINUX)
model.SetText(WideToUTF16(L"\x05d5\x05b7\x05D9\x05B0\x05D4\x05B4\x05D9"));
model.MoveCursorTo(gfx::SelectionModel(0));
EXPECT_TRUE(model.Delete());
......@@ -214,6 +226,7 @@ TEST_F(TextfieldViewsModelTest, EditString_ComplexScript) {
EXPECT_TRUE(model.Delete());
EXPECT_TRUE(model.Delete());
EXPECT_EQ(WideToUTF16(L""), model.GetText());
#endif
// The first 2 characters are not strong directionality characters.
model.SetText(WideToUTF16(L"\x002C\x0020\x05D1\x05BC\x05B7\x05E9\x05BC"));
......@@ -548,7 +561,7 @@ TEST_F(TextfieldViewsModelTest, MAYBE_Clipboard) {
EXPECT_EQ(29U, model.GetCursorPosition());
}
static void SelectWordTestVerifier(const TextfieldViewsModel& model,
void SelectWordTestVerifier(TextfieldViewsModel &model,
const string16 &expected_selected_string, size_t expected_cursor_pos) {
EXPECT_EQ(expected_selected_string, model.GetSelectedText());
EXPECT_EQ(expected_cursor_pos, model.GetCursorPosition());
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment