158754 - Change selection color and show caret when user is in type ahead find mode

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Aaron Leventhal]

Type ahead find is being implemented in bug 30088.

When type ahead find mode is on, there needs to be a better visual indication
that something special is happening. Right now, we rely on the status bar, which
isn't very noticable. Since type ahead find can be used in embedding, we need
something that doesn't rely on the full chrome being there.

Type ahead find starts when:
  //   1. The user just begins typing (except in mailnews or composer)
  //   2. Manually via menu items or accelerators when we implement bug 158752.

Type ahead find mode ends when:
  //   1. Escape pressed
  //   2. Window switched
  //   3. User clicks in window 
  //   4. Selection is moved/changed
  //   5. Window scrolls
  //   6. Focus is moved

I suggest we draw a box around the selection. When type ahead find is finished
the box will disappear, but not the menus.

The question is, how do we do this? It was suggested that we modify
nsISelectionDisplay to have another flag that indicates when a box should be
drawn around the selection, and then modify nsTextFrame.cpp, etc. to draw the box.

However, the problem with this is that the box will actually be outside the
current frame - so what is to prevent turds from contaminating the page layout?

Comment 1

[Aaron Leventhal]

Changing summary - this is a better solution than drawing a box. The selection
color change and visible caret make it apparent something live is happening.
Drawing a box around the selection would be much more difficult, and this might
even be better.

Comment 2

[Aaron Leventhal]

Attachment: Adds SELECTION_ATTENTION const to nsISelectionController, as opposed to SELECTION_ON or SELECTION_DISABLED. Then changes nsTextFrame.cpp to change color when that is set.

Comment 3

[Akkana Peck]

I like the behavior!  Code review coming shortly, but I like the behavior of the
selection changing color.

Please make it stop spewing the following warning, though:

nsTypeAheadFind.cpp: In method `void nsTypeAheadFind::SetCaretEnabled 
(nsIPresShell *, int)':
nsTypeAheadFind.cpp:1170: warning: enumeral mismatch in conditional 
expression: `nsISelectionController::{anonymous enum}' vs 
`nsISelectionController::{anonymous enum}'
nsTypeAheadFindRegistration.cpp

Comment 4

[Akkana Peck]

Comment on attachment 93434 [details] [diff] [review]
Adds SELECTION_ATTENTION const to nsISelectionController, as opposed to SELECTION_ON or SELECTION_DISABLED. Then changes nsTextFrame.cpp to change color when that is set.

>+++ mozilla/layout/html/base/src/nsTextFrame.cpp	31 Jul 2002 17:30:33 -0000
>@@ -970,6 +970,7 @@
> class DrawSelectionIterator
> {
>   enum {DISABLED_COLOR = NS_RGB(176,176,176)};
>+  enum {ATTENTION_COLOR = NS_RGB(55,220,120)};

This is my only problem with this patch.  I'd be happier if it was added to the
look & feel, or otherwise made variable, rather than being compiled in.  (In
fact, I could make a case for moving DISABLED_COLOR there as well, though
that's probably less important.)  Hardwiring a particular shade of green seems
wrong, especially a shade which doesn't contrast well at all with the white
foreground used by selection.

I'll mark this r=akkana anyway, since it's easy enough to fix the look and feel
part in a separate patch and I don't want to block the other changes just for
that.  Either leave this bug open to track that issue, or open a separate bug,
whichever you prefer.

For myself, I really like the different-colored selection (though I'd prefer
more contrasty colors) but I'm indifferent to the caret blinking when in
typeahead-select mode.	  But if it helps some people notice it then it's
probably a good thing.

Comment 5

[Aaron Leventhal]

Attachment: Uses nsILookAndFeel to get colors

Comment 6

[Aaron Leventhal]

Akkana, I just tested this on a fresh tree and it compiles and works.

Comment 7

[Akkana Peck]

Comment on attachment 93637 [details] [diff] [review]
Uses nsILookAndFeel to get colors

r=akkana, assuming you run it by a layout person and they're okay with the
changes.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 93637 [details] [diff] [review]
Uses nsILookAndFeel to get colors

- In nsTextFrame.cpp:

 DrawSelectionIterator::DrawSelectionIterator(const SelectionDetails
*aSelDetails, PRUnichar *aText, 
-							PRUint32 aTextLength,
nsTextFrame::TextStyle &aTextStyle, PRInt16 aSelectionStatus)
+							PRUint32 aTextLength,
nsTextFrame::TextStyle &aTextStyle, PRInt16 aSelectionStatus, nsIPresContext
*aPresContext)

Please fix the messed up next-line argument indentation while you're at it.

- Also in nsTextFrame.cpp:

+    nsCOMPtr<nsILookAndFeel> look;
+	if (NS_SUCCEEDED(aPresContext->GetLookAndFeel(getter_AddRefs(look))) &&
look) {
+	   
look->GetColor(nsILookAndFeel::eColor_TextSelectBackgroundAttention,
mAttentionColor);
+	    look->GetColor(nsILookAndFeel::eColor_TextSelectBackgroundDisabled,
mDisabledColor);
+      mDisabledColor  = EnsureDifferentColors(mDisabledColor,
...

Messed up indentation, I smell tabs.

sr=jst if you fix those nits.

Comment 9

[Aaron Leventhal]

checked in

Comment 10

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2002.09.24.08 mozilla trunk. background color is green
(default) and the caret is displayed.