Closed
Bug 158754
Opened 22 years ago
Closed 22 years ago
Change selection color and show caret when user is in type ahead find mode
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Core
DOM: UI Events & Focus Handling
Tracking
()
VERIFIED
FIXED
People
(Reporter: aaronlev, Assigned: aaronlev)
Details
Attachments
(1 file, 1 obsolete file)
11.51 KB,
patch
|
akkzilla
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Updated•22 years ago
|
OS: Windows 2000 → All
Hardware: PC → All
Assignee | ||
Updated•22 years ago
|
Assignee | ||
Comment 1•22 years ago
|
||
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.
Summary: Draw box/border around selection when user is in type ahead find mode → Change selection color and show caret when user is in type ahead find mode
Assignee | ||
Comment 2•22 years ago
|
||
Comment 3•22 years ago
|
||
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•22 years ago
|
||
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.
Attachment #93434 -
Flags: review+
Assignee | ||
Comment 6•22 years ago
|
||
Akkana, I just tested this on a fresh tree and it compiles and works.
Comment 7•22 years ago
|
||
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.
Attachment #93637 -
Flags: review+
Comment 8•22 years ago
|
||
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.
Attachment #93637 -
Flags: superreview+
Assignee | ||
Comment 9•22 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 10•22 years ago
|
||
vrfy'd fixed with 2002.09.24.08 mozilla trunk. background color is green (default) and the caret is displayed.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Component: Keyboard: Navigation → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•