132489 - caret should stop blinking when user enters menus

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P3)

Description

[Aaron Leventhal]

1. Hit Accel+Shift+K to toggle caret browsing on.
2. Notice the caret is blinking in the browser content.
3. Hit Alt+F to enter file menu. Notice caret is still blinking

What should happen - caret should stop blinking temporarily when entering 
menus, and start blinking again when menus are exited

Comment 1

[saari (gone)]

nsbeta1+, blocking a real world user from effectively using ns with a screen
reader. 

Comment 2

[John Sun]

Hi, aaronl
I'm a member of browser-china-atf. I plan to work on the bug.
But I can't see the caret blinking in browser content. Do you refer to the
caret blinking in url input line at location bar? thanks!

Comment 3

[John Sun]

Do you mean the caret blinking in some textaread such as a input control of a
form?

Comment 4

[Aaron Leventhal]

We have a "browse with caret" mode that allows the user to select text with the
keyboard. You can toggle it off and on with Accel+Shift+K.

It gives you a blinking caret that you can move around in the document with, as
if in a read-only editor.

However, when you pull down a menu it should stop blinking. 

I also noticed that if you're in the editor, or in a textfield or textarea, it
also  doesn't stop blinking. Compare this with notepad or another application,
where the caret stops blinking as you enter the menus.

Comment 5

[Aaron Leventhal]

Resummarizing: this is a problem no matter where the caret is - the caret could
be in a form control, or in composer.

This is not a recent regression - this has been a problem for quite a while. Not
sure if we ever had this right.

Having a caret blinking while the user enters menus will mess up screen readers
entirely.

Comment 6

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 7

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 8

[Aaron Leventhal]

Wish I could go back and change old comments -

==== F7 is now the key (not Accel+Shift+K). ====

Comment 9

[Aaron Leventhal]

Attachment: When menu bar gets toggled, so does caret if it was on

Comment 10

[Joe Hewitt (gone)]

Comment on attachment 89376 [details] [diff] [review]
When menu bar gets toggled, so does caret if it was on

You should really initialize mWasCaretVisible to false in the constructor,
looks like there is a case where it could be checked before you're gotten its
value.

Comment 11

[Aaron Leventhal]

Attachment: Initialize mCaretWasVisible(PR_FALSE) in constructor

Comment 12

[Brian Ryner (not reading)]

Comment on attachment 89453 [details] [diff] [review]
Initialize mCaretWasVisible(PR_FALSE) in constructor

>Index: nsMenuBarFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/xul/base/src/nsMenuBarFrame.cpp,v
>retrieving revision 1.96
>diff -u -r1.96 nsMenuBarFrame.cpp
>--- nsMenuBarFrame.cpp	24 Apr 2002 07:15:33 -0000	1.96
>+++ nsMenuBarFrame.cpp	27 Jun 2002 21:23:09 -0000
>@@ -191,6 +195,37 @@
>     mTarget->RemoveEventListener(NS_LITERAL_STRING("keyup"), (nsIDOMKeyListener*)mKeyboardNavigator, PR_TRUE);
>   
>     NS_IF_RELEASE(mKeyboardNavigator);
>+  }
>+  
>+  // We don't want the caret to blink while the menus are active
>+  // The caret distracts screen readers and other assistive technologies from the menu selection
>+  // There is 1 caret per document, we need to find the focused document and toggle its caret 
>+  nsCOMPtr<nsIDocument> document;
>+  nsCOMPtr<nsIFocusController> focusController;
>+  nsCOMPtr<nsIPresShell> presShell;
>+  mPresContext->GetShell(getter_AddRefs(presShell));
>+  if (presShell) {
>+    presShell->GetDocument(getter_AddRefs(document));
>+    if (document)
>+      document->GetFocusController(getter_AddRefs(focusController));
>+  }
>+  if (focusController) {
>+    nsCOMPtr<nsIDOMWindowInternal> windowInternal;

I'd just write this so that we return early if presShell, document, or
focusController are null, to avoid over-indenting.  Your call though.

>+    focusController->GetFocusedWindow(getter_AddRefs(windowInternal));
>+    nsCOMPtr<nsIDOMWindow> window(do_QueryInterface(windowInternal));
>+    nsCOMPtr<nsIDOMDocument> domDoc;
>+    window->GetDocument(getter_AddRefs(domDoc));

It may be possible for the focused window to be null here, although that raises
the question of how the user opened the menu...  Probably better to null-check
it though.

Comment 13

[Aaron Leventhal]

Sorry, I didn't want a review of this yet - Hewitt and I decided that nsCaret
needs to listen for menu bar activity.

Comment 14

[Aaron Leventhal]

Attachment: Makes nsCaret listen to 2 new DOM events DOMMenuBarActive and DOMMenuBarInactive

Caret should know about the menu bar, so that it knows when to stop blinking.
Menus didn't fire the right events to notify the caret in all situations, for
example when Alt by itself is pressed to enter the menubar, or then escape to
leave the menubar. So this patch adds DOMMenuBarActive, DOMMenuBarInactive.

I also refactored nsMenuFrame::FireMenuDOMEvent() to FireDOMEvent() and moved
it up to nsBoxFrame, for code reuse purposes.

Comment 15

[kinmoz]

I really don't think the caret should know about app level widgets ... that is I
think the app or menu code should deal with hiding the caret.

Comment 16

[Aaron Leventhal]

Attachment: Returning to original impl at the request of Simon Fraser. Added more null checks for bryner.

Comment 17

[Brian Ryner (not reading)]

Comment on attachment 89596 [details] [diff] [review]
Returning to original impl at the request of Simon Fraser. Added more null checks for bryner.

r=bryner

Comment 18

[Joe Hewitt (gone)]

Comment on attachment 89596 [details] [diff] [review]
Returning to original impl at the request of Simon Fraser. Added more null checks for bryner.

sr=hewitt

Comment 19

[Aaron Leventhal]

forgot to mark fixed.

Comment 20

[Kyle Yuan]

I still can reproduce this bug by these two ways:
1) turn on caret browsing;
2) pull down the "Window" menu, caret stop blinking;
3) click the "Tools" menu, now caret is blinking again.
-or-
1) turn on caret browsing;
2) pull down the "Tools" menu and open the "Form Manager" submenu, caret stop 
blinking;
3) press <esc> to close the "Form Manager" submenu, caret is blinking again.

Version:
Mozilla 1.1b
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1b) Gecko/20020905

Comment 21

[Aaron Leventhal]

We need this, it will mess up screen reader users -- at least on Windows.

Comment 22

[Aaron Leventhal]

The cases mentioned by Kyle are caused by PresShell.cpp
PresShellViewEventListener::DidRefreshRegion() {
  return RestoreCaretVisibility();
}

That code and the caret code have no idea when the menu bar is active.

Comment 23

[Aaron Leventhal]

Attachment: Use nsISelectionController methods instead of nsICaret. This way the presshell can properly keep track of the caret.

Comment 24

[Aaron Leventhal]

Comment on attachment 108849 [details] [diff] [review]
Use nsISelectionController methods instead of nsICaret. This way the presshell can properly keep track of the caret.

Found a problem with the patch. 

Load google and tab to a link. If you alt+tab away and back, the cursor starts
blinking in the text box.

Comment 25

[Aaron Leventhal]

Attachment: Correct patch -- the last patch occasionally made the caret blink when it shouldn't

Comment 26

[Brian Ryner (not reading)]

Comment on attachment 109545 [details] [diff] [review]
Correct patch -- the last patch occasionally made the caret blink when it shouldn't

I'd change the check for selCon to be an assertion rather than a null-check.
There's no reason the QI should ever fail.  Other than that, sr=bryner.

Comment 27

[Aaron Leventhal]

fixed

Comment 28

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381