420499 - Caret browsing mode wrongly engaged

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P2)

Description

[Ria Klaassen (not reading all bugmail)]

Steps to Reproduce:

- Go to a page with a text input field like this bug
- Click in that field
- Click Bookmarks(menu), right click of one of your bookmarks, choose Delete
- Click somewhere in text outside the input field to dismiss the menu

Actual result: a blinking cursor in the page outside text input field
Expected result: when caret browsing not enabled, no cursor should be shown
Regression window is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1204075680&maxdate=1204078259
Possibly caused by bug 394473.

I wonder if this is related to Bug 418055.

Comment 1

[Matthew Cline]

It doesn't just create a cursor/caret, it also engages caret browsing mode (which is really annoying if you use the PgUp and pgDwn keys for scrolling).

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #1)
> It doesn't just create a cursor/caret, it also engages caret browsing mode
> (which is really annoying if you use the PgUp and pgDwn keys for scrolling).
> 

The STR don't engage browse-with-caret mode, as if you press F7 to toggle browse-with-caret the caret is still there. Pressing it again toggles it off.

Also if you follow the STR, and then click elsewhere in the document, the caret goes away.

I suspect this is a problem with the focus code is deciding to turn on the caret incorrectly for some reason...

Bug appears in [2008022714] but not in [2008022504], so it's pretty much guaranteed that it was caused by my patch for bug 394473 which was checked in on 26 Feb.

Comment 3

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #2)
>
> Also if you follow the STR, and then click elsewhere in the document, the caret
> goes away.

The caret only disappears here if I click in the input field.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 - don't re-enable caret if focus changed

Ok, so nsMenuBarFrame disables/reenables the caret when it pops up a menu. It remembers the caret-enabled state of the content that is focused when it pops up the menu, and restores that state in the focused content when it hides a menu. 

So if you:

1. Focus a textfield.
2. Activate a menu.
3. Focus something other than the previously focused textfield in the webpage.

The menu will re-enable the caret, even if the newly focused content shouldn't display a caret.

This wasn't actually a bug introduced by the patch for bug 394473 per se, it was always there, it's just that the state of nsCaret::mIgnoreUserModify stopped the enabled caret from being drawn when you did step 3 of the STR above. The patch for bug 394473 changed the state of mIgnoreUserModify, so this bug showed up.

So the fix is to remember the content that was focused when you activate a menu as well as the caret-enabled state, and when you deactivate the menu, if the focus has changed, don't restore the caret-enabled state in the new focus. We rely on the newly focused content's focus listener to re-enable the caret if it requires it.

Comment 5

[Aaron Leventhal]

Other steps that reproduce this:
1. Go to google.com
2. Click on I'm feeling luck
3. Hit back
4. Tab to the links, you'll see the caret blinking

Does the proposed patch fix that case?

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #5)
> Other steps that reproduce this:
> 1. Go to google.com
> 2. Click on I'm feeling luck
> 3. Hit back
> 4. Tab to the links, you'll see the caret blinking
> 
> Does the proposed patch fix that case?
> 

No, that looks like a dupe of bug 421169.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd like to try to get away from complicated save/restore of caret-related state, to a more functional approach where we simply tell the caret code that something has changed and the caret code computes whether the caret should be showing or not.

In this case, that would mean the caret code checking whether there is an open popup that does not contain the caret, before deciding to draw the caret.

Does this make sense?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You can check for open popups using http://mxr.mozilla.org/seamonkey/source/layout/xul/base/public/nsXULPopupManager.h, and checking for open popups of the types you care about: http://mxr.mozilla.org/seamonkey/source/widget/public/nsIWidget.h#131

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2 - popup detection in nsCaret

* Adds popup detection to nsCaret::DrawCaret(), which now doesn't draw the caret when there are open XUL menu and panel popups, but none of them contain the caret.
* Removes mCaretWasVisible from nsMenuFrame.
* nsMenuFrame now erases the caret when it opens/closes a popup, this way we don't have the caret linger for up to a second when we open/close popups.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we need to be a bit more careful about testing popups. For example we don't want to hide the caret just because a panel is open. Doesn't this make the caret disappear when the URL autocomplete popup opens?

Also, we might conceivably have the caret inside a popup menu when another popup menu opens, and we should hide the caret in that case.

I think the rule should be "hide the caret if there is a menu popup above the caret in the popup chain". You can implement that by walking the popup chain and setting or clearing a flag when you see a menu popup or you see a popup containing the caret.

Also instead of explicitly calling EraseCaret you probably should just call something which rechecks whether a caret should be shown and hides/shows it as needed. I can't remember whether an API for that exists already, but it should!

OpenPopupsDontContainCaretAt is a bad name, maybe IsMenuPopupHidingCaret?

Comment 11

[Neil Deakin]

I think you want to hide the caret whenever a menu is opened, but not any other type of popup. Panels (unless they have noautofocus="true") remove the focus when opened anyway.

Note that OSX just stops the caret from blinking when a menu is opened, but doesn't hide it. Maybe we should handle this as well?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> I think you want to hide the caret whenever a menu is opened, but not any other
> type of popup. Panels (unless they have noautofocus="true") remove the focus
> when opened anyway.

That's basically the same as what I suggested. But I want to get away from this imperative idea that we "show" or "hide" the caret and instead simply have rules for when the caret should be showing and when it shouldn't. The former is getting much too complicated when we have callers trying to show or hide the caret and they interfere with each other.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #12)
> That's basically the same as what I suggested. 

It's slightly different. With Roc's approach you can have a caret in a panel which is in front of a menu, which could be needed if in future we want to make funky custom menus, like Vista's start menu search box for example.

So perhaps we should hide the caret if:
1. An open popup contains the caret, but a menu popup exists before the caret-owning popup in the popup list (i.e. a menu is in front of the popup with the caret). If the menu itself contains the caret we don't hide it.
2. A menu popup is open.

So we can only have a caret visible when we have menu popups open if the caret exists in a popup before the first menu popup.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So when is that different to what I suggested?

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #10)
> I think the rule should be "hide the caret if there is a menu popup above the
> caret in the popup chain".

When there is no caret in the popup chain, for example when you just open up a menu on the menu bar.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, I sort of assumed "above the caret in the popup chain" included all popups if the caret isn't in a popup. But never mind, we agree :-)

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3 - nsCaret::EnsureDrawnState()

* Added nsCaret::EnsureDrawnState() which will erase the caret if it's drawn when it shouldn't be. I didn't want to have it do the opposite, as all the "turn on caret" logic is spread out, and I didn't want to refactor it all in at this stage.
* nsXULPopupManager::ShowPopupCallback() calls nsCaret::EnsureDrawnState() to ensure the caret's not visible when it shouldn't be.
* nsCaret::IsMenuPopupHidingCaret() checks if the caret should be drawn, as discussed.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (popupFrame->PopupType() == ePopupTypeMenu)
+      menuPopupOpen = PR_TRUE;
+
+    if (nsLayoutUtils::IsProperAncestorFrame(popupFrame, caretFrame)) {
+      // The caret is in this popup, if a popup before this one is
+      // a menu popup, hide the caret, else show it.
+      return menuPopupOpen;
+    }

Seems like this would hide the caret if it's in the topmost menupopup, would it not? Did you test that?

+    NS_IMETHOD EnsureDrawnState();

Not a great name for a method that can only hide the caret. How about "CheckCaretDrawingState"?

+     PRBool        MustDrawCaret(PRBool aIgnoreDrawnState=PR_FALSE);

Document aIgnoreDrawnState and don't use a default for it, that hurts readability.

+
+  // Caret visibility may have been affected, ensure that
+  // the caret isn't now drawn when it shouldn't be.
+  nsIDocument* doc = aPopup->GetCurrentDoc();
+  if (doc) {
+    nsIPresShell* presShell = doc->GetPrimaryShell();
+    EnsureCaretDrawnState(presShell);

You're getting the presshell, and then EnsureCaretDrawnState just gets the document from it. Simplify.

Comment 19

[Neil Deakin]

I would assume that only the frontmost menu would matter for whether the caret should be displayed.

nsIFrame* popupFrame = pm->GetTopPopup(ePopupTypeMenu);
return !popupFrame || nsLayoutUtils::IsProperAncestorFrame(popupFrame, caretFrame);

That is, a caret wouldn't be displayed in a popup if another popup was open above it.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What if the topmost menu opened another popup (a panel?) that contains the caret?

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Testcase - caret in xul menu popups

(In reply to comment #18)
> +    if (popupFrame->PopupType() == ePopupTypeMenu)
> +      menuPopupOpen = PR_TRUE;
> +
> +    if (nsLayoutUtils::IsProperAncestorFrame(popupFrame, caretFrame)) {
> +      // The caret is in this popup, if a popup before this one is
> +      // a menu popup, hide the caret, else show it.
> +      return menuPopupOpen;
> +    }
> 
> Seems like this would hide the caret if it's in the topmost menupopup, would it
> not? Did you test that?

Here's a testcase. It fails some assertions (with and without my patch), and the textboxes aren't editable, so I'll file bugs to get those things fixed.

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4

(In reply to comment #18)
> Seems like this would hide the caret if it's in the topmost menupopup, would it
> not? Did you test that?

Yeah, this patch now handles that.

Also I now get the caret frame using a slightly different mechanism. I was using nsCaret::GetCaretFrame(), but this returns null if the caret hasn't been setup yet. So when its null I figure out what will be the caret frame, and use that instead. This situation happens when SetCaretVisible() calls StartBlinking() which calls DrawCaret() which calls MustDrawCaret() which calls IsMenuPopupHidingCaret().

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (nsLayoutUtils::IsProperAncestorFrame(popupFrame, caretFrame)) {
+      // The caret is in this popup, if a popup before this one is
+      // a menu popup, hide the caret, else show it.
+      return firstMenuPopupFrame && (firstMenuPopupFrame != popupFrame);
+    }

Er, why don't you just move this earlier in the 'for'? Then you don't need this inequality check and firstMenuPopupFrame can go back to being a PRBool.

There must be a way to avoid creating nsCaret::GetCaretFrameForSelection. Can we reorder some code in DrawCaret or somewhere else so that the caret is set up before we decide whether to draw it? Or maybe we can change our strategy a bit so instead of getting the caret frame, we just get the selection focus node and see if that's inside the popup?

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v5

(In reply to comment #23)

> There must be a way to avoid creating nsCaret::GetCaretFrameForSelection. Can
> we reorder some code in DrawCaret or somewhere else so that the caret is set up
> before we decide whether to draw it? Or maybe we can change our strategy a bit
> so instead of getting the caret frame, we just get the selection focus node and
> see if that's inside the popup?
> 

Ok, this one gets the focused content, and checks if that's in the popup. I also simplified the loop in nsCaret::IsMenuPopupHidingCaret() a bit.

Comment 25

[Reed Loden [:reed]]

Can we get a test for this, please?

Checking in layout/base/nsCaret.cpp;
/cvsroot/mozilla/layout/base/nsCaret.cpp,v  <--  nsCaret.cpp
new revision: 1.188; previous revision: 1.187
done
Checking in layout/base/nsCaret.h;
/cvsroot/mozilla/layout/base/nsCaret.h,v  <--  nsCaret.h
new revision: 1.57; previous revision: 1.56
done
Checking in layout/base/nsICaret.h;
/cvsroot/mozilla/layout/base/nsICaret.h,v  <--  nsICaret.h
new revision: 1.45; previous revision: 1.44
done
Checking in layout/xul/base/src/nsMenuBarFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsMenuBarFrame.cpp,v  <--  nsMenuBarFrame.cpp
new revision: 1.171; previous revision: 1.170
done
Checking in layout/xul/base/src/nsMenuBarFrame.h;
/cvsroot/mozilla/layout/xul/base/src/nsMenuBarFrame.h,v  <--  nsMenuBarFrame.h
new revision: 1.72; previous revision: 1.71
done
Checking in layout/xul/base/src/nsXULPopupManager.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsXULPopupManager.cpp,v  <--  nsXULPopupManager.cpp
new revision: 1.57; previous revision: 1.56
done

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Mochitest

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 313246 [details] [diff] [review]
Mochitest

Mochitest which focuses a text input, ensures caret it on, opens a menu, ensures, caret is off.

I had to add a function to nsISelectionController to determine if the caret was visible. I can't just check if the caret is enabled, because now that we're starting to move towards "centralizing" the rendering logic of the caret, the caret can be enabled but not actually drawing.

Comment 28

[Reed Loden [:reed]]

Comment on attachment 313246 [details] [diff] [review]
Mochitest

>+NS_IMETHODIMP PresShell::GetCaretVisible(PRBool *aOutIsVisile)
>+{
>+  *aOutIsVisile = PR_FALSE;

s/Visile/Visible/

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Mochitest with fewer spelling errors...

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    * Returns true if the caret is enabled, visible, and currently blinking.

This doesn't make it clear whether this returns true when the caret would be visible except it's in the off-cycle of a blink.

+    boolean getCaretVisible();

Might make more sense as a readonly attribute "caretVisible". The C++ method name would be the same...

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Mochitest v2

With Roc's changes.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      setTimeout("checkCaretWithMenuOpen()", 2000);

Why is this timeout 2000 instead of 500?

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #32)
> +      setTimeout("checkCaretWithMenuOpen()", 2000);
> 
> Why is this timeout 2000 instead of 500?
> 

500ms wasn't long enough for my Windows Vista machine to popup the menu and start displaying the caret. The menu pops up, but the caret isn't registering as visible, and isn't blinking yet. I chose 2000ms to ensure that it's unlikely that this test would fail due to a machine popping menus up too slow.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What makes you think 2000 is going to be enough, then?

Comment 35

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #34)
> What makes you think 2000 is going to be enough, then?
> 

1000 worked for me, so I doubled it. Seriously, if someone checks in a patch which causes popups to not display their caret within 2 seconds, do we *not* want their patch to fail mochitests?

I could change this to check for caret visibility every 500ms, and then consider it a failure if the caret hasn't come on when 5 seconds has passed. I'd say anything longer than 5 seconds is bad news, and should cause a test failure.

Comment 36

[Neil Deakin]

You would want to wait until the menu actually opens then (menus are opened asynchronously) by continuing during a popupshown event which will fire after the menu is visible.

Also, you should write setTimeout(checkCaretWithMenuOpen, 2000); without the quotes to call the function directly rather than needing an eval.

Comment 37

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Mochitest v3

(In reply to comment #36)
> You would want to wait until the menu actually opens then (menus are opened
> asynchronously) by continuing during a popupshown event which will fire after
> the menu is visible.

Ok, here we go. That's a much nicer way of doing it, shorter and faster too. Thanks Neil!

Comment 38

[Chris Pearce [:cpearce (Not reading bugmail)]]

Could someone please checkin the mochitest v3?

Comment 39

[Reed Loden [:reed]]

Checking in content/base/public/nsISelectionController.idl;
/cvsroot/mozilla/content/base/public/nsISelectionController.idl,v  <--  nsISelectionController.idl
new revision: 3.32; previous revision: 3.31
done
Checking in layout/base/nsCaret.cpp;
/cvsroot/mozilla/layout/base/nsCaret.cpp,v  <--  nsCaret.cpp
new revision: 1.189; previous revision: 1.188
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.1107; previous revision: 3.1106
done
Checking in layout/base/tests/Makefile.in;
/cvsroot/mozilla/layout/base/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.62; previous revision: 1.61
done
RCS file: /cvsroot/mozilla/layout/base/tests/test_bug420499.xul,v
done
Checking in layout/base/tests/test_bug420499.xul;
/cvsroot/mozilla/layout/base/tests/test_bug420499.xul,v  <--  test_bug420499.xul
initial revision: 1.1
done
Checking in layout/forms/nsTextControlFrame.cpp;
/cvsroot/mozilla/layout/forms/nsTextControlFrame.cpp,v  <--  nsTextControlFrame.cpp
new revision: 3.290; previous revision: 3.289
done