1219833 - Composition underline color no longer shown

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Screenshot of underline color

This is a regression from the 2015-08-20 nightly. Range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f384789a29dcfd514d25d4a16a97ec5309612d78&tochange=29b2df16e961fbe9a379362ecba6f888d1754bc3

We used to support underline color in composition (see attached screenshot), but currently all composition underlines are black.

Masayuki-san, do you think this is a regression from bug 299603?

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Could be. I guess that the IME doesn't specify foreground color. If so, current nsTextFrame always use foreground color or background color for underline color:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.cpp?rev=9a5903768780#5369

> 5369         // If underline color is defined and that doesn't depend on the
> 5370         // foreground color, we should use the color directly.
> 5371         if (aRangeStyle.IsUnderlineColorDefined() &&
> 5372             aRangeStyle.IsForegroundColorDefined() &&
> 5373             aRangeStyle.mUnderlineColor != aRangeStyle.mForegroundColor) {

It seems that this should be:

if (aRangeStyle.IsUnderlineColorDefined() &&
    (!aRangeStyle.IsForegroundColorDefined() ||
     aRangeStyle.mUnderlineColor != aRangeStyle.mForegroundColor)) {

I'm not so active today, if you'd like to try to fix with this information, feel free to ask review to me. Otherwise, I'll work on this late next week. (I'll be offline until next Tue.)

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Respect composition underline color (v1)

Thanks! That change worked.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8681513 [details] [diff] [review]
Respect composition underline color (v1)

Although, this has an issue of contrast between its background color and underline color. But it must be rare case and we need bigger change to fix it. So, for fixing this regression, this simpler patch is better.

Thank you for the fix! Please request approvals for the branches.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8681513 [details] [diff] [review]
Respect composition underline color (v1)

Approval Request Comment

[Feature/regressing bug #]: Bug 299603

[User impact if declined]: Visual artifact; composition underline color not shown

[Describe test coverage new/current, TreeHerder]: Locally

[Risks and why]: Almost none; patch only affects the way we draw composition text.

[String/UUID change made/needed]: None

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec846e144024

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec846e144024

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ec846e144024

Comment 8

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8681513 [details] [diff] [review]
Respect composition underline color (v1)

Recent regression and time to fix potential fallout, taking it.
Should be in 43 beta 2.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3841d7954630

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cd32550ec7fe

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3841d7954630