124789 - focus goes away, never to be found again (client-side image map)

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Linux 2002-02-07-22 nightly and later nightlies

NOT PRESENT IN: Linux 2002-02-07-07 nightly and earlier nightlies

STEPS TO REPRODUCE:
1)  Load http://bugzilla.mozilla.org/showattachment.cgi?attach_id=68824
2)  Click in content area
3)  Hit tab (watch the "mozilla" link focus and status bar say
    "http://mozilla.org")
4)  Hit tab (watch the "mozilla" link lose focus and the status bar say
   "http://slashdot.org/index.pl?op=u&bid=poll")
5)  Hit shift-tab
6)  Hit tab twice
7)  Hit control-n.

EXPECTED RESULTS:
5)  Focus goes back to "mozilla" linkx
6)  Focus goes to different part of imagemap
7)  New window opens

ACTUAL RESULTS:
5) nothing happens
6) nothing happens
7) nothing happens

Clicking in the content area restores shortcut key functionality and the like

Comment 1

[Boris Zbarsky [:bzbarsky]]

Nominating.  This bug completely breaks keyboard access to app functionality in
some cases...

Comment 2

[Peter Trudelle]

nsbeta1- per Nav triage team,  ->1.2

Comment 3

[Brian Ryner (not reading)]

Does Sun's accessibility team have any interest in taking on this bug?

Comment 4

[Gilbert Fang]

Ok , I will deal with it . 
In mozilla 099 for windows, this bug does not exist. It does works just as the 
expected results except the #6. The focus goes to there, but the content area
does not scroll to show the focus part. Maybe this should be another bug.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Gilbert, that other bug is the bug this bug blocks.  I have a fix for the other
bug that should work but I can't test because of this bug (and yes, builds on
Linux without my fix also exhibit this bug).

Comment 6

[Gilbert Fang]

After the step 4 of the test case, the browser actually does not handle any
NS_KEY_PRESS event at all. But It can handle mouse event. I have set a
breakpoint at PresShell::HandleEventInternal. In that situation, any key press
does not make the mozilla get into the PresShell::HandleEventInternal at all,
while handle_gdk_event can get all the gdk key press event . So I think there
must be something wrong with the dispatching-event handlers.

Comment 7

[Gilbert Fang]

I have traced into PresShell::HandleEvent. When running the testcase, mozilla
goes into PresShell::HandleEvent at the following location:

      if (GetCurrentEventFrame()) {
        rv = HandleEventInternal(aEvent, aView, NS_EVENT_FLAG_INIT, aEventStatus);
      }
      NS_RELEASE(manager);
I found that GetCurrentEventFrame() return nsnull instead of a valid nsIFrame.
mozilla never goes into the HandleEventInternal. Is that why all the key event
not handled by mozilla?  Actually, all mouse event is correctly handled.
I traced into  GetCurrentEventFrame and found that PresShell::GetPrimaryFrameFor
return a nsnull nsIFrame. 
nsIFrame*
PresShell::GetCurrentEventFrame()
{
  if (!mCurrentEventFrame && mCurrentEventContent) {
    // Make sure the content still has a document reference. If not,
    // then we assume it is no longer in the content tree and the
    // frame shouldn't get an event, nor should we even assume its
    // safe to try and find the frame.
    nsCOMPtr<nsIDocument> doc;
    nsresult result = mCurrentEventContent->GetDocument(*getter_AddRefs(doc));
    if (NS_SUCCEEDED(result) && doc) {
--->   GetPrimaryFrameFor(mCurrentEventContent, &mCurrentEventFrame);
    }
  }

I am not so famaliar with that part of code that I can not make sure whether it
is correct now. I will study it tomorrow.

By the way, today I examined the testcase with mozilla 099 again and reproduce
it .So I think it is a cross-platform bug.  
Could I have some privilege of bugzilla to change bug's state , obsolete the
patch and etc. 

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There are a bunch of comments on bug 115481 that may belong on this bug or may
belong on their own new bug, but definitely don't belon on bug 115481.

Comment 9

[Gilbert Fang]

I traced the tab NS_KEY_PRESS event and found that the mozilla always want to
find  the primary frame for the MAP content. When the  GetPrimaryFrameFor return
nsnull,  then the frame which should handle the key event is lost.  Now, I have
a question:
Why the Map's style is inline(see the quirk.css)? Actually , the map is just
like a  java script: 
one or more form control can call one java script function ;
one or more image can use one map definition. 
And both map and js are not be shown only by  themself.
the script's style is "none", so I think the map's style should also be defined
as "none".
And then the mozilla should never GetPrimaryFrameFor the MAP content. 

Hi, David! Do you agree with me?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That makes sense to me, although I don't know much about either imagemaps or
event handling in frames.

Comment 11

[Gilbert Fang]

file a new bug 136841 for comment #9

Comment 12

[Boris Zbarsky [:bzbarsky]]

*** Bug 142584 has been marked as a duplicate of this bug. ***

Comment 13

[Jack Tanner]

I don't know if this is the same bug, but seems similar. Using 1.2.1 on WinXP,
start the browser. Ctrl-T does nothing, Ctrl-L does nothing.

Comment 14

[Stephen Ostermiller]

I'm using a build 10 days old now, but I can not confirm this bug.  I load the
page, focus, use tab, use shift-tab,  I get the expected results, not the broken
behavior reported earlier.

Can other folks check to see if this is fixed?  (I clicked to this bug from the
sig of Slashdot user Hobart, I've never actually seen this bug before.)

Works for me:
Linux Build ID: 2002123008, Redhat 7.3

Comment 15

[Boris Zbarsky [:bzbarsky]]

Stephen, you hit tab twice after focusing the page, correct?  Did mozilla scroll
to the imagemap?  I am seeing exactly the behavior I reported with linux trunk
build 2003-01-10-08, so this is as broken as ever.

Comment 16

[Aaron Leventhal]

This is not worksforme. I'm using build 2003012913.

To see the bug, all I have to do is this:
1. Load http://bugzilla.mozilla.org/showattachment.cgi?attach_id=68824
2. Type TAB TAB SHIFT+TAB

Shift+tab will not move back to the previous spot.

Comment 17

[Aaron Leventhal]

I also notice that the entire image is highlighted as I try to tab through an
image map.

Comment 18

[Aaron Leventhal]

Interesting! If the image map is visible, focus does not get lost. If the image
map isn't visible -- then it does get lost. Presumably the frames are created
lazily?

Also, if I try a similar test with an image map using a broken image, focus gets
lost there too.

Comment 19

[Aaron Leventhal]

Attachment: Second test case, using broken image with image map

Comment 20

[Boris Zbarsky [:bzbarsky]]

Frames are not created lazily...

Comment 21

[Aaron Leventhal]

Well, I do know that GetPrimaryFrameFor() is returning null for the HTML area
content nodes when the focus is broken. It no longer returns null once the image
has been painted.

Comment 22

[Aaron Leventhal]

So I think the fix is:
1. Key events should never be lost just because GetPrimaryFrameFor() is
returning null for the current focus. That might fix a lot of other bugs. The
event could be retargeted to the document as a whole, when in doubt. At least
that way global hotkeys woulndn't be hosed.
2. We need to find out why GetPrimaryFrameFor() returns null for <area> when the
image hasn't been displayed yet, or it's a broken image

Comment 23

[Aaron Leventhal]

This might be fixed by jkeiser's patch in bug 148542, which makes the event
system better at dealing with null frames.

Comment 24

[John Keiser (jkeiser)]

That patch does not deal with this issue, although it *does* target the event at
the content and bubble like a good little boy.  The problem is that
PreHandleEvent() and PostHandleEvent() are very unhappy if they don't get a frame.

What would need to happen to fix this bug such that display: none content could
receive events and still be happy would be:
(a) fix bug 148542 such that display: none content gets presshell events fired
at them

(b) call nsEventStateManager::PreHandleEvent and PostHandleEvent even if there
is no frame, and pass in the content (I experimentally had it call these
functions when there is no frame and the browser seemed to work just fine). 
This would make some other things more elegant, such as bug 103055.

(c) Have the focus system *use* this content.  Probably not extremely hard from
what I can read.  Just takes doing it.

(a) is reviewed and will hopefully happen at the beginning of 1.4a; (b) might
happen for bug 103055 the more I think about it; and (c) could happen here. 
I'll look at it.

Still, for this bug I think we want to ask ourselves why we focused something
that has no frame.

Comment 25

[John Keiser (jkeiser)]

Adding dependent on bugs where the fix may help out.  Could be fixed without
them though.

Comment 26

[saari (gone)]

topembed+

Comment 27

[Brian Ryner (not reading)]

I think the premise of having the image frame be the primary frame for the
<area> element is flawed in the first place, because you can have multiple <img>
elements that share the same <map>.  The HTML spec even gives an example that
uses this.  Gecko breaks horribly on that. (the focused-image effect is drawn on
the wrong imagemap, and tab navigation doesn't go through both maps).

You really need two pieces of information to describe the focus location with an
imagemap - the image frame, and the area element.  That's the only way I see to
solve this problem for good.

In addition, if we want to stay compatible with IE, we need to treat the <area>
element as the focused content node, so we need to keep doing what we're doing
with mCurrentFocus in the ESM.  But, when we navigate into an imagemap, we also
need to track the image frame, and _not_ depend on getting it from the area element.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Would it help if <area> elements got frames of their own?  The image frame's
GetFrameForPoint method could return the right thing on clicks and such.  The
click handling would move out of image frame and into the <area> element...

You'd still have multiple frames per content node, with none being clearly
"primary", though... so it may not help this problem at all....

Comment 29

[Brian Ryner (not reading)]

I don't think that would really help - the issue is whether you can get to the
correct frame given an <area> element, and if you can associate multiple images
with the same <map>, there's no good way to do that.  saari and I talked about a
vague idea of implementing imagemaps by inserting anonymous copies of the <area>
elements as children of the image element, so that focus could be on a node that
was tied to a specific frame.  That might be more trouble than it's worth though.

I came up with a patch to fix this specific problem with tabbing, I'll attach it
next.

Comment 30

[Brian Ryner (not reading)]

Attachment: patch

This patch adds a way for a frame to be set as the starting frame for focus
traversal, without assuming that it's the primary frame for mCurrentFocus.  In
non-imagemap cases, I null out mCurrentFocusFrame so it will use the old method
of getting the frame from mCurrentFocus.  I changed PresShell to use this when
finding the target frame for key events.

This certainly doesn't fix all of the callers of GetFocusedContent who then go
ahead and call GetPrimaryFrameFor() on the node.

Comment 31

[Brian Ryner (not reading)]

Comment on attachment 114857 [details] [diff] [review]
patch

John, what do you think?

Comment 32

[Aaron Leventhal]

There might be a lot of unique, hard-to-track-down cases where for some reason
we go keydead. Would it be a good idea to use nsPresShell::RetargetEventToParent
in those cases, so that at least global keys still work in all situations?

Comment 33

[John Keiser (jkeiser)]

Comment on attachment 114857 [details] [diff] [review]
patch

   nsCOMPtr<nsIContent> focusContent;
   if (mCurrentFocus) {
-    focusContent = mCurrentFocus;
+    GetFocusedFrame(&focusFrame);

This works fine for setting the frame, but it looks like you'll return
NS_ERROR_FAILURE right after this since you're not setting focusContent anymore
in this case.  I don't think you meant to not set focusContent.

-  else if (mCurrentFocus && !docHasFocus)   // If selection is not found in
doc, use mCurrentFocus 
-    presShell->GetPrimaryFrameFor(mCurrentFocus, &curFocusFrame);
+  else if (mCurrentFocus && !docHasFocus)
+    curFocusFrame = mCurrentFocusFrame;

mCurrentFocusFrame will be null in the case were there was nothing selected and
the document did not have focus (see the previous chunk of your patch).  That
seems wrong.

+      if (shell)
+	 shell->GetPrimaryFrameFor(mCurrentFocus, aFrame);

Is it worth caching the primary frame in mCurrentFocusFrame?

Other than that, looks good.

Comment 34

[Brian Ryner (not reading)]

Attachment: patch #2

addressed all of jkeiser's comments, I hope.

I also decided that we probably want to set NS_FRAME_EXTERNAL_REFERENCE so we
don't end up with a dangling pointer if the frame goes away.  I refactored the
code that sets that into an inline function since we use it so much.

Comment 35

[John Keiser (jkeiser)]

Comment on attachment 114948 [details] [diff] [review]
patch #2

+    if (mCurrentFocusFrame)
+      curFocusFrame = mCurrentFocusFrame;
+    else
+      presShell->GetPrimaryFrameFor(mCurrentFocus, &curFocusFrame);

This should just do GetFocusedFrame(&curFocusFrame); -- that will cause it to
be cached and properly referenced and all.

r=me with that.  I verily like the frame external reference refactoring :)

Comment 36

[Brian Ryner (not reading)]

Attachment: patch #3

fixed.	I also converted one other call site that should have been using
GetFocusedFrame, and added back a null check that may be necessary in some
cases.

Comment 37

[John Keiser (jkeiser)]

Comment on attachment 114962 [details] [diff] [review]
patch #3

Looks good.

Comment 38

[Brian Ryner (not reading)]

Attachment: same as patch #3, but with nsPresShell diffs

Oops... the change to presshell has to accompany this for the ESM changes to do
any good.  I had it in my tree, just forgot to include it in the previous
patches.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 117254 [details] [diff] [review]
same as patch #3, but with nsPresShell diffs

sr=bzbarsky

Comment 40

[Brian Ryner (not reading)]

checked in.

Comment 41

[Boris Zbarsky [:bzbarsky]]

This caused bug 214373.

Comment 42

[Firefox Bug Husbandry Bot]

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