258514 - clicked anchor with location hash set to element id gives first child anchor focus rectangle & selection not always updated correctly

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[nicolas claisse]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3 StumbleUpon/1.995
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3 StumbleUpon/1.995

I tested the page in Opera, it works correctly but in FF, I have the problem
with some javascript anchors links that they don't point to the good place (you
can test it when clicking on those links
# HTML : Introduction
# Le fonctionnement du Web
or when you click on titles (it causes the div that contains this tittle to
retract/expand), and when expands, sometimes it bugs... I think it's when the
div contains a table or a list.

Reproducible: Always
Steps to Reproduce:
1. just click on js generate link 'Le fonctionnement du Web' or 'HTML :
Introduction'
2. OR rectract expand some titles (by ex. 'Le fonctionnement du Web') by
clicking on it, and see the difference with other ones...
3.

Actual Results:  
page 'scrolls' to a bad place, not corresponding to the beginning of the div
with ID used to form new location : base_URL#ID 


Expected Results:  
the same as what would arrive when I enter the anchor link in the adress bar of
the navigator

My english is not really good ;) french speaker

please do not share my document

Comment 1

[Markus Lindström]

Confirmed.

Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040908 Firefox/0.10

This is a Gecko issue, changing product to Browser.

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

This testcase shows this is happening because Mozilla scrolls to the position
of the first focusable item in the target content where the link pointed to.

Comment 3

[Martijn Wargers (dead)]

I'm seeing this problem even in Mozilla1.0.
Wouldn't it be a better behavior of Mozilla if it would the (invisible caret) at
the position of the targeted content (targeted from the link)? 
The user could then tab himself to the first focusable item in that content. 

Comment 4

[nicolas claisse]

I'm not sure this is the right interpretation : "Mozilla scrolls to the position
of the first focusable item in the target content where the link pointed to".
because in most of other cases where links points to divs with id, the target
reached is the one expected, and the divs all begins with a title (it's a
focusable item no?) -
- 
In Opera, and in expected behaviour, the target points to the top left corner of
the box (containing CSS margins and everything), this is very nice for
positionning...

Comment 5

[Martijn Wargers (dead)]

These two lines are responsible for the problem:
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsPresShell.cpp#4002

PRBool isSelectionWithFocus;
esm->MoveFocusToCaret(PR_TRUE, &isSelectionWithFocus);

Removing them solves the bug, but that way, the focus doesn't change either.
This isn't desirable. 
The focus should change, but changing the focus should not cause scrolling
further below.
Or (what I prefer) the focus should be removed from the link you clicked, and
the caret should be put at the place of the target content.

Comment 6

[Boris Zbarsky [:bzbarsky]]

ccing some people who may know why we do what we do (and whether we want to do it).

Comment 7

[Martijn Wargers (dead)]

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

Comment 8

[Martijn Wargers (dead)]

Changing summary to something less misleading (I just copied over the summary
from the duped bug 263034).

Comment 9

[Brad Fults]

Attachment: Better testcase

From the duped bug 263034.

Comment 10

[Brad Fults]

I would like to draw attention to what I see as a more verbose description of
the problem (or at least one manifestation of it) and a solution that makes
everyone happy (most notably maintaining accessibility): bug 263034 comment 0.

Comment 11

[Aaron Leventhal]

(In reply to comment #5)
> Or (what I prefer) the focus should be removed from the link you clicked, and
> the caret should be put at the place of the target content.
This should be fine. You can put focus on the document itself and leave
selection in the new place. If someone whips up a patch I'll try it out and
review it.

Comment 12

[Martijn Wargers (dead)]

Attachment: Like this?

Like this? I have no idea what's the best method to put the focus on the
document.
I just found this method somewhere else in Mozilla's code.

Comment 13

[Aaron Leventhal]

Try just
esm->SetFocusedContent(nsnull); // Set focus to document

How does that test out?

Comment 14

[Martijn Wargers (dead)]

(In reply to comment #13)
> Try just
> esm->SetFocusedContent(nsnull); // Set focus to document
> 
> How does that test out?
Unfortunately that doesn't seem to work, the clicked link keeps the focus somehow.

Comment 15

[Aaron Leventhal]

Sorry to keep doing this to you :/
This should really work, it's used in nsTypeAheadFind.cpp

esm->SetContentState(nsnull, NS_EVENT_STATE_FOCUS); 

Comment 16

[Martijn Wargers (dead)]

Attachment: Anchorpatch2

(In reply to comment #15)
> Sorry to keep doing this to you :/
No problem.

> This should really work, it's used in nsTypeAheadFind.cpp
> 
> esm->SetContentState(nsnull, NS_EVENT_STATE_FOCUS); 
Yes, that seems to work :)

Comment 17

[Aaron Leventhal]

Okay, I need to think about this. We might need to test for the presence of a
screen reader and still focus the anchor if one is present. This may be how the
screen reader will know to update it's position when the user clicks on on a
named anchor.

Comment 18

[Aaron Leventhal]

Comment on attachment 161372 [details] [diff] [review]
Anchorpatch2

r=aaronlev assuming that you verified that tabbing does in fact still navigate
relative to the new position.

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 161372 [details] [diff] [review]
Anchorpatch2

It doesn't, after clicking the link tabbing once takes
you back to the first link.

Comment 20

[Aaron Leventhal]

Comment on attachment 161372 [details] [diff] [review]
Anchorpatch2

Okay, well we probably need to look at ShiftFocusInternal and see why this code
is not getting entered:
3085   if (!aStart && itemType != nsIDocShellTreeItem::typeChrome &&
3086	   mLastFocusedWith != eEventFocusedByMouse) {
3087	 // We're going to tab from the selection position

This change can't break tabbing relative to the new position.

Comment 21

[Mats Palmgren (inactive)]

(In reply to comment #20)
> 3086	   mLastFocusedWith != eEventFocusedByMouse) {

Removing that test fixes it, but I'm worried it was there for a reason...

Comment 22

[Aaron Leventhal]

So it thinks the last focus was from the mouse, and we need to somehow inform it
that the last focus was from eEventFocusedByApplication (because that's the
truth). That will allow it to get through.

The problem now is that nsIEventStateManager is an ugly API for focus and
doesn't allow you to set that.

Comment 23

[Martijn Wargers (dead)]

Attachment: Patch from Aaron

Sorry, I had checked that the patch worked properly, but it turned that there
was something wrong with my build.

This is a patch that Aaron sent me by e-mail. I've changed this rule:
+	   if (currFrame->IsFocusable(&tabIndexUnused, PR_TRUE)) {
into:
+	   if (currFrame->IsFocusable(&tabIndexUnused)) {
or else it wouldn't compile.

I haven't checked if this actually fixes the bug (yet).

Extra remarks from Aaron:
"Maybe something like this would work reliably.
I can't get it to link, but I don't have time right now to mess with it too
much.
The enum declaration wouild ideally go inside the interface, if we can."

Comment 24

[Aaron Leventhal]

Attachment: Expose high level esm::ChangeFocusWith().

Comment 25

[Aaron Leventhal]

Our focus API needs to be cleaned up some day, but that's another story.

Comment 26

[Aaron Leventhal]

Comment on attachment 161517 [details] [diff] [review]
Expose high level esm::ChangeFocusWith(). 

Bz, I realize you're busy -- but mats must be on vacation or something.

Comment 27

[Boris Zbarsky [:bzbarsky]]

I'm not likely to get to this earlier than about a week from now...

Comment 28

[Mats Palmgren (inactive)]

Attachment: Another testcase

Comment 29

[Mats Palmgren (inactive)]

Comment on attachment 161517 [details] [diff] [review]
Expose high level esm::ChangeFocusWith(). 

A difference is that onfocus() is not called (and there is no focus outline)
on the target.	IMO this is the correct behaviour, I just wanted to point
it out.  The code looks good to me.

Comment 30

[Aaron Leventhal]

Checking in layout/html/base/src/nsPresShell.cpp;
/cvsroot/mozilla/layout/html/base/src/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.787; previous revision: 3.786
done
Checking in content/events/public/nsIEventStateManager.h;
/cvsroot/mozilla/content/events/public/nsIEventStateManager.h,v  <-- 
nsIEventStateManager.h
new revision: 1.59; previous revision: 1.58
done
Checking in content/events/src/nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <-- 
nsEventStateManager.cpp
new revision: 1.543; previous revision: 1.542
done
Checking in content/events/src/nsEventStateManager.h;
/cvsroot/mozilla/content/events/src/nsEventStateManager.h,v  <-- 
nsEventStateManager.h
new revision: 1.124; previous revision: 1.123
done

Comment 31

[Martijn Wargers (dead)]

The "Better testcase" seems to work correctly now.
But the "testcase" doesn't work correct, afaik.
After clicking on the 'a href="#fonctionnementweb"' link and then pressing tab
once, the window scrolls to the 'First focusable item' link, but the focus is
set on the 'a href="#fonctionnementweb"' link. I would expect it to be set on
the  'First focusable item' link.

Comment 32

[Aaron Leventhal]

(In reply to comment #31)
> But the "testcase" doesn't work correct, afaik.
I can't even read "testcase". Want to post a new readable version that
demonstrates the problem?

Comment 33

[Martijn Wargers (dead)]

Attachment: Testcase

Yes, sorry, that testcase is indeed not very clear. Hopefully this one is.
With this testcase, I would expect that when clicking on the first or second
link and then pressing tab, the focus would be on the second link.
That's not what is happening. The focus gets on the third link, somehow.

Comment 34

[Aaron Leventhal]

That's weird. In your tests does directly relate to the position:fixed element?
Or can you repro the problem without that somehow?

Comment 35

[Martijn Wargers (dead)]

Ah no, the first link in the targetted div seems to get ignored in every case,
so I can reproduce it without the positioned:fixed div.
Shall I file a new bug on this issue? 

Comment 36

[Aaron Leventhal]

Attachment: Click on named anchor link, tab skips a link

Comment 37

[Aaron Leventhal]

Attachment: Don't select an element, put caret (collapsed selection) at that position

I've run into this before. The nsIDOMRAnge/nsISelection APIs collapse methods
don't necessarily do what I would think they'd do when a node has been
selected.

Comment 38

[Aaron Leventhal]

Attachment: Make sure selection gets moved even when aScroll is false (happens when moving to page with anchor in history)

The patch should be easy to review -- it's only large because of the
indentation change in some parts.

Comment 39

[Aaron Leventhal]

This one is actually needed for currently ongoing screen reader beta tests. The
fix ensures that the selection does the right thing when clicking on an anchor
with a location hash, and the screen reader ends of following that selection to
get the new position.

Comment 40

[Aaron Leventhal]

Checking in nsPresShell.cpp;
/cvsroot/mozilla/layout/html/base/src/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.790; previous revision: 3.789
done

Comment 41

[Josh Birnbaum]

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

Comment 42

[Josh Birnbaum]

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

Comment 43

[Josh Birnbaum]

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

Comment 44

[Boris Zbarsky [:bzbarsky]]

This caused bug 308064.  That seems like a pretty serious annoyance for keyboard users...