Closed Bug 1219833 Opened 5 years ago Closed 5 years ago

Composition underline color no longer shown

Categories

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

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox43 --- fixed
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

(Keywords: regression)

Attachments

(2 files)

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?
Flags: needinfo?(masayuki)
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.)
Flags: needinfo?(masayuki)
Thanks! That change worked.
Attachment #8681513 - Flags: review?(masayuki)
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.
Attachment #8681513 - Flags: review?(masayuki) → review+
Assignee: nobody → nchen
Status: NEW → ASSIGNED
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
Attachment #8681513 - Flags: approval-mozilla-beta?
Attachment #8681513 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/ec846e144024
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
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.
Attachment #8681513 - Flags: approval-mozilla-beta?
Attachment #8681513 - Flags: approval-mozilla-beta+
Attachment #8681513 - Flags: approval-mozilla-aurora?
Attachment #8681513 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.