Closed Bug 455217 Opened 16 years ago Closed 16 years ago

Selection colors for plain text files is wrong (white background, blue text instead of blue background, white text)

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.1b1

People

(Reporter: martijn.martijn, Assigned: zwol)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(2 files)

(rev 0) proposed patch, needs test cases [Checkin: Comment 33]
3.57 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
(rev 0) test cases, don't work
84.95 KB, application/x-gzip
Details 
I'm using WindowsXP, classic theme, in case if that matters.

Looking at https://bugzilla.mozilla.org/attachment.cgi?id=338506 or
http://martijn.martijn.googlepages.com/builderror.txt and then when I select some text, the selection colors are off.

I get a white background color with blue text color for the selection, while it should be blue background color with white text color.

This regressed between 2008-09-12 and 2008-09-13:
http://hg.mozilla.org/mozilla-central/log/c833b8069d4d
http://hg.mozilla.org/mozilla-central/log/ade776b76598
Backing out the fix for bug 453566 didn't help.
Observed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080913070841 Shredder/3.0b1pre ID:20080913070841
As well as SeaMonkey nightly.
Flags: wanted1.9.1?
Yes, backing out the fix for bug 453916 seems to fix this bug.
Blocks: 453916
Seems to be windows-specific; I won't have time to investigate further until tomorrow.
This is not Windows-specific, I'm seeing the same effect on
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080915153118 SeaMonkey/2.0a1pre
in the plaintext mail message pane.
OS: Windows XP → All
Hardware: PC → All
I built a copy of Seamonkey with no special options and I still don't see blue-on-white selected text.  I tried:

 - a local .txt file loaded in the web browser interface
 - a local copy of an email message loaded via File->Open File... in the
   email client interface
 - the message composition window of the email client, set to both HTML and
   plain text.

Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b1pre) Gecko/20080915103716 SeaMonkey/2.0a1pre
I even can see it in
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080915153524 Minefield/3.1b1pre
when I load a .txt file, but I noticed that once I focus the location bar, the unfocused selection is correct again.

In SeaMonkey, I see it both with the default theme that uses system colors and with my custom theme that uses hardcoded colors.
Nope, doesn't happen here.  At all.  I have, however, found some suspicious code - EnsureSufficientContrast() in nsTextFrameThebes.cpp, which deliberately swaps the text highlight's foreground and background if it thinks that will be more legible, and which does not pay any attention to color alpha.  So it may be confusing our canonical representation of transparent (rgba(0, 0, 0, 0.0)) with black (rgba(0, 0, 0, 1.0)).  But that doesn't explain why it's not happening for me.

Would it be possible for you to attach a tarball of your custom SeaMonkey theme to this bug?

dbaron: Thoughts on how we could make EnsureSufficientContrast aware of color alpha?  The "entirely correct" thing to do would be use the composition of all colors in the z-stack between the canvas and the current frame, but I don't know whether we have that information at this point, and it might be unacceptably high overhead.

If this is really the problem, we may need to audit all other uses of NS_LUMINOSITY_DIFFERENCE and NS_GetLuminosity - mxr doesn't think there are many, fortunately.
(In reply to comment #9)
> Would it be possible for you to attach a tarball of your custom SeaMonkey theme
> to this bug?

I just have extensions/EarlyBlue@kairo.at being a symlink to a copy of http://git-public.kairo.at/?p=themes.git;a=tree;f=EarlyBlue;hb=HEAD - you can download a snapshot from there or git clone http://git-public.kairo.at/themes.git easily if you want to test with it. The theme is in need of a few minor updates, but as I can see the same with the default theme, those needed updates are not the problem - I guess it's actually the colors, my system theme uses similar colors as my custom theme, and the actual colors being the problem would probably support your analysis.
I'm sorry, could you please give me step-by-step instructions for how to make that theme available in SeaMonkey starting from a downloaded snapshot directory?  I am not at all familiar with themes.
Codewise, a place to start looking could be the various selection color stuff in nsTextFrameThebes.cpp.
don't need special theme - see this with current thunderbird nightly default theme ftp://ftp.mozilla.org/pub/thunderbird/nightly/latest-trunk/
dbaron: see comment 9; I found stuff that looks suspicious, but I don't know quite what to do about it.

Wayne: I have yet to be able to reproduce this under any circumstances at all, including default Tb and Ff themes.  KaiRo's custom theme might do it but I'm not really expecting it to.
Zack:
Actually, I just realized, even in a custom theme, the content areas this is happening in are using the system colors, so trying the custom SeaMonkey theme probably doesn't help.
My OS theme is set to have selected test as #FFFFFF on #336699 and there I see the bug - it reverses those colors, making selected text be #336699 on #FFFFFF which is not much different from the unselected #000000 on #FFFFFF style.
When I set the OS colors to the KDE default colors of #FFFFFF on #678DB2 and restart SeaMonkey, it actually works correctly.
Sorry for one more comment, but just to confirm: If I set the "KDE  default" color scheme on the OS, and then start SeaMonkey, it works as I said. If I just change the selected text background color to #336699 there, I trigger the bug on a SeaMonkey restart. Same if I select one of the "Redmond 2000", "Redmon 95", or "Redmond XP" color schemes, which might explain why Windows users are seeing this.
As text background and foreground colors are swapped, the code you pointed to really sound suspicious. (I'm actually a bit worried that we even have code not to follow text background/foreground colors that were explicitely set, e.g. through OS theming.)
(In reply to comment #17)
> really sound suspicious. (I'm actually a bit worried that we even have code not
> to follow text background/foreground colors that were explicitely set, e.g.
> through OS theming.)

There are good reasons for this.  In particular, suppose that:
 * the foreground/background colors specified by a Web page, and
 * the foreground/background OS colors for selection
are exactly the same.  In that case, we still want the selection to show up, which means we have to change something.
(In reply to comment #17)
> If I just
> change the selected text background color to #336699 there, I trigger the bug
> on a SeaMonkey restart.

Aha! My system was set to use #FFFFFF on #86ABD9.  Changing the background color to #336699 lets me see the effect in both Seamonkey and Firefox trunk builds.  This is consistent with the hypothesis that EnsureSufficientContrast() is misinterpreting a transparent background as a black one; #336699 is much closer to black than what I had.

Now to see what I can do about it.
You can get back to the old behavior by replacing any occurrence of transparent with the default background color, I'd think, since that's how mBackgroundColor used to be initialized.

Figuring out what background is really behind something can be hard.  nsCSSRendering::FindNonTransparentBackground approximates it.  That's probably somewhat better.

An alternative might be checking to see if only the text has sufficient contrast.  However, that's likely to lead to the reversing more often than necessary, and it's probably the background that we care about more.
thinking out loud: mFrameBackgroundColor is already being initialized via FindNonTransparentBackground (in InitCommonColors).  Of course, that only returns the nearest ancestor background that's not *entirely* transparent.  Perhaps we should do the same thing we now do in PaintBackground and compose that color on top of the pres context's default background color; I think that would address this bug in all but obscure cases.

It occurs to me that there's no guarantee that the colors that we request from the look-and-feel service and use to set mSufficientContrast (still in InitCommonColors) will be opaque.  I can imagine, for instance, eColor_TextSelectBackground being a semitransparent yellow, with the expectation being that that will get composited onto whatever background was already there (like highlighter markers).  Maybe shouldn't worry about that now.
Here's a patch which implements the simple scheme I described in the previous comment.  Since we now have two places relying on the pres context's ->DefaultBackgroundColor() as a backstop for a color that might be transparent, I added code to the pres context to ensure that color is opaque, rather than do it in the places that rely on it.  They now have assertions.  Also, NS_GetLuminosity now asserts its argument is opaque.

I manually tested this as follows: 

  - with KaiRo's choice of selection background color, in a plain .txt file,
    with the window background set to white, Firefox latest + this patch draws
    selected text in white on blue.
  - ditto, but with the window background set to black, selected text is blue
    on white.

I shall attempt to write test cases but I'm not sure whether it's possible to set a text selection (and the selection colors!) programmatically.  This may need to be kicked over to QA to add to their manual test suite ("select text on lots of different backgrounds, make sure the highlight is always clearly visible")

p.s. Me and my old eyes are going to keep this selection color; thanks :-)
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Here are some incomplete test cases - patch applies on top of the previous one.  I got stuck, because:

 - The renders are too big; they get cut off on the right.  (Easily fixed.)

 - It doesn't seem to be possible to enable chrome privileges in a reftest
   without popping up a confirmation dialog box, which halts the entire test
   run.

 - More seriously, support.js attempts to ensure that the render is the same
   whether or not the window is active, by setting all three variants of the
   ui.textSelectBackground preference (this is why I don't just use
   ::-moz-selection, and why I need chrome privs in the first place).  But that
   doesn't work, because nsTextFrameThebes.cpp forces them not to be the same!

 - That problem means I didn't bother writing the references, since they would
   be different if the colors were correct.

 - On a more philosophical level, these tests don't actually test for
   this bug; they are exhaustive tests of selection color as a function of
   page-specified background color, but they are *not* tests of selection
   color on top of an arbitrary user-specified default background color.

I'd love to hear ideas for these, but I really don't want a test that requires the reftest runner window to be visible.  I like the way you can minimize it and go do something else and nothing breaks.  (Yes, I wrote logic to run mochitests in a separate X server, but it's not wired up for reftests and it's nice not to have to do anything so heavyweight.)

(The tests are too big to submit uncompressed, sorry.)
Could we get the patch reviewed and checked in? This is blocking the SeaMonkey 2 Alpha 1 release at the moment, which else is frozen and waiting for fixes to the last couple of blockers.
Attachment #338746 - Flags: superreview?(dbaron)
Attachment #338746 - Flags: review?(dbaron)
Comment on attachment 338746 [details] [diff] [review]
(rev 0) proposed patch, needs test cases
[Checkin: Comment 33]

ok, flagging the patch (sans testcases) for review.
Comment on attachment 338746 [details] [diff] [review]
(rev 0) proposed patch, needs test cases
[Checkin: Comment 33]

r+sr=dbaron, although I think you should probably run the idea of forcing the prescontext's default background color to opaque past roc.
Attachment #338746 - Flags: superreview?(dbaron)
Attachment #338746 - Flags: superreview+
Attachment #338746 - Flags: review?(dbaron)
Attachment #338746 - Flags: review+
I like it.

I hope this might fix a problem I've been seeing on Linux with flashes of garbage in window backgrounds.
David, can we get away without automated tests for this?
Yeah; I'd mark it is in-testsuite?, though.
Flagging, and tagging for checkin.  Checkers-in: only the patch that's r+, not the other one, please.
Flags: in-testsuite?
Keywords: checkin-needed
>It doesn't seem to be possible to enable chrome privileges in a reftest
>without popping up a confirmation dialog box, which halts the entire test
>run.

Zack, Boris Zbarsky turned me onto his incredibly useful http://mxr-stage.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/WindowSnapshot.js, and have used it to write mochitests that are secretly reftests. See bug 451286, comments 8 & 9.
Comment on attachment 338746 [details] [diff] [review]
(rev 0) proposed patch, needs test cases
[Checkin: Comment 33]

http://hg.mozilla.org/mozilla-central/rev/64844a4c0a6e
Attachment #338746 - Attachment description: (rev 0) proposed patch, needs test cases → (rev 0) proposed patch, needs test cases [Checkin: Comment 33]
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: wanted1.9.1?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080919103915 SeaMonkey/2.0a1pre

Verifying the fix on Linux: this tinderbox-build is free from the problem. Earlier builds (including the 20080919100708 build) still have it.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: