Open Bug 560071 Opened 11 years ago Updated 9 months ago

Improve IME selection painting

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set
normal

Tracking

()

People

(Reporter: masayuki, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: inputmethod)

Attachments

(1 file, 4 obsolete files)

nsTextFrame uses IME specified selection colors/styles. However, there are some a11y problems:

1. If only foreground or background color is specified, nsTextFrame needs to check the contrast with its background color.

- When only foreground color is specified, should check whether the color has enough contrast with the frame background color. If it doesn't, use system color of input field's background color.

- When only background color is specified, should check whether the color has enough contrast with the frame foreground color. If it doesn't, use system color of input field's foreground color.

- After that, if the selection background color and frame's background don't have enough contrast, and if the selection type is "selected clause", we should revert the colors.

2. The underline color selection logic is wrong. Currently, nsTextFrame uses the specified underline color if it's specified. Otherwise, when foreground color is specified, it uses this. Otherwise, it uses its foreground color.

The better logic is: When both underline color and foreground color are specified and they are different color or only underline color is specified, should use the specified underline color. Otherwise, if foreground color or background color is defined, use computed foreground color by #1. Otherwise, use the frame foreground color.

But if only underline color is specified, should check the contrast with the frame background color.
Attached patch Patch v1.0 (obsolete) — Splinter Review
See comment 0 for the detail of this patch.
Attachment #445585 - Flags: review?(roc)
The color selection code is already far too complicated :-(. I think we should factor it out into a separate file with a clean, documented interface.

It seems to me the interface should be:

Input:
-- the frame text color and background colors
-- the prescontext (so we can get access to nsITheme/nsILookAndFeel)
-- the preferred foreground color (if specified), text color (if specified) and decoration color (if specified) for the selection
-- the selection type?

Output: the actual foreground color, text color and decoration color for the selection

Does that sound right? A single function could do this.
OK, I agree. And it's better for me too. IME related code should be separated to other files in any level, it could protect from other developers who don't use IME.
Attached patch Patch v2.0 (obsolete) — Splinter Review
I'm thinking about tests for this patch.
Attachment #445585 - Attachment is obsolete: true
Attachment #445585 - Flags: review?(roc)
FYI: Ignore the widget part.
Attached patch Patch v2.1 (obsolete) — Splinter Review
Hmm... I should still look for better rule of reverting IME selection color...
Attachment #447518 - Attachment is obsolete: true
Attached patch Patch v2.1 (obsolete) — Splinter Review
Attachment #469340 - Attachment is obsolete: true
Attached patch Patch v3.0Splinter Review
First test (prototype) passed. I'll add many test cases, create a test for nsITextRangeStyle, separate the patch to 3 bugs.
Attachment #469341 - Attachment is obsolete: true

Resetting assignee which I don't work on in this several months.

Assignee: masayuki → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.