159998 - typing named anchor into address bar makes address bar appear to keep focus

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Type http://www.squarefree.com/bookmarklets/pagelinks.html#search_links into
the address bar.
2. Press enter and wait for the page to load.
3. Select the address.
4. Press enter again.

Result: the page scrolls to the anchor again, but the location bar *appears* not
to lose focus.  The selection is still dark blue, but hitting tab goes to the
first item after the named anchor and Ctrl+C doesn't copy.  

If you replace step 3 with "place the caret after the address", a similar bug
occurs: the address bar still has a blinking insertion point but no longer has
focus.

This bug happens in 1.0 and 2002 072908 (~1.1b) but not in 0.9.9.  This bug may
have been caused by the fix for bug 130447, "Clicking on anchor goes to top of
page, entering the URL directly doesn't".

Comment 1

[Radha on family leave (not reading bugmail)]

I am not sure this bug belongs to me. I think its better if Aaron first  took a
 look at this. 

Comment 2

[Aaron Leventhal]

-> Hewitt, who owns location bar focus issues meta bug

Comment 3

[Aaron Leventhal]

Attachment: Null out mCurrent focus in FocusElementButNotDocument() when we're updating the focus appearance of a non-focused document. Otherwise the next blur event to this doc isn't fired.

Comment 4

[Brian Ryner (not reading)]

That's correct that mCurrentFocus should be null when the document doesn't have
focus, but how is it getting set to something non-null to start with?

Comment 5

[Aaron Leventhal]

Brian, I´m cancelling out the side effect of SendFocusBlur(). Should I patch 
that routine instead not to cause the side effect, or find another way to write 
this method? I´m not completely confident about how to do the latter.

Comment 6

[Aaron Leventhal]

Brian, I tried to do this by having SendFocusBlur() not change mCurrentFocus.
However, that created a problem because the focus outline doesn't then appear at
the right time (for example if a link gets selected when you do a in page dialog
find). It turns out that SetContentState() needs to use mCurrentFocus.

To me, it seems easier to use this fix than to change both SendFocusBlur() and
SetContentState() not to use mCurrentFocus. Or would you prefer me to make those
respond to some kind of flag? It's easy with SendFocusBlur(), but
SetContentState() would need to take a new boolean argument or use a global flag
or new member variable.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 127245 [details] [diff] [review]
Null out mCurrent focus in FocusElementButNotDocument() when we're updating the focus appearance of a non-focused document. Otherwise the next blur event to this doc isn't fired.

Hm.  This entire method seems more complicated than I'd think it could be.  Why
can't we just update the focus controller's focused element without ever
calling SendFocusBlur()?

Comment 8

[Aaron Leventhal]

Attachment: As simple as I can make it, without changing esm::GetContentState()

Unfortunately, we have to make sure mCurrentFocus is set and then reset back to
null. Otherwise, esm::GetContentState() doesn't tell layout to use :focus style
rules for the element, and it won't get dotted out line.

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 128684 [details] [diff] [review]
As simple as I can make it, without changing esm::GetContentState()

Thanks, that's a _lot_ clearer.  Hopefully it doesn't regress anything :^/

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 127245 [details] [diff] [review]
Null out mCurrent focus in FocusElementButNotDocument() when we're updating the focus appearance of a non-focused document. Otherwise the next blur event to this doc isn't fired.

-'ing this one since I hope we can go with the simplification patch.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 128684 [details] [diff] [review]
As simple as I can make it, without changing esm::GetContentState()

+  if (!focusController || !aContent)
+      return;
...
+  if (aContent)
+    mDocument->ContentStatesChanged(oldFocusedContent, aContent, 
				     NS_EVENT_STATE_FOCUS);

You return early if !aContent, so you know it's not null here. No need for that
if check.

-  FlushPendingEvents(mPresContext);

Are you sure this isn't needed any more? Do pending events get flushed by some
of the calls you make here, or does it not matter?

sr=jst if you look into those issues.

Comment 12

[Aaron Leventhal]

I don't think FlushPendingEvents() was ever needed. I put it in there originally because I was 
making it look like some other code. However, the way this method is used, it will always come 
after a keypress, and then there will be a pause for the layout system to catch up, unless the user 
does a find again or leaves the form, in either case that's okay.

I can put it back it to be on the safe side, if you think that's wise.

Comment 13

[Aaron Leventhal]

checked into trunk

Comment 14

[Johnny Stenback (:jst)]

I'm fine leaving it out, if you did (I didn't check), please keep your eyes open
for any possible regressions caused by the omission of that call.