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

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: nicolas claisse, Assigned: Aaron Leventhal)

Tracking

(Depends on: 1 bug, {testcase})

Trunk
x86
Windows XP
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 6 obsolete attachments)

(Reporter)

Description

13 years ago
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

13 years ago
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.
Status: UNCONFIRMED → NEW
Component: General → Layout
Ever confirmed: true
Product: Firefox → Browser
Version: unspecified → Trunk

Updated

13 years ago
Assignee: firefox → nobody
QA Contact: firefox.general → core.layout
Created attachment 158257 [details]
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.
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. 
(Reporter)

Comment 4

13 years ago
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...
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.
ccing some people who may know why we do what we do (and whether we want to do it).

Updated

13 years ago
Keywords: testcase
*** Bug 263034 has been marked as a duplicate of this bug. ***
Changing summary to something less misleading (I just copied over the summary
from the duped bug 263034).
Summary: window.location = "#some_ID_of_a_div_with_a_table" points the wrong place → clicked anchor with location hash set to element id gives first child anchor focus rectangle

Comment 9

13 years ago
Created attachment 161181 [details]
Better testcase

From the duped bug 263034.

Comment 10

13 years ago
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.
(Assignee)

Comment 11

13 years ago
(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.
Created attachment 161194 [details] [diff] [review]
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.
(Assignee)

Comment 13

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

How does that test out?
(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.
(Assignee)

Comment 15

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

esm->SetContentState(nsnull, NS_EVENT_STATE_FOCUS); 
Created attachment 161372 [details] [diff] [review]
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 :)
Attachment #161194 - Attachment is obsolete: true

Updated

13 years ago
Attachment #161372 - Flags: review?(aaronleventhal)
(Assignee)

Comment 17

13 years ago
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.
Assignee: nobody → m.wargers
(Assignee)

Comment 18

13 years ago
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.
Attachment #161372 - Flags: review?(aaronleventhal) → review+

Updated

13 years ago
Attachment #161372 - Flags: superreview?(dbaron)
Comment on attachment 161372 [details] [diff] [review]
Anchorpatch2

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

Comment 20

13 years ago
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.
Attachment #161372 - Flags: superreview?(dbaron)
Attachment #161372 - Flags: review+
(In reply to comment #20)
> 3086	   mLastFocusedWith != eEventFocusedByMouse) {

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

Comment 22

13 years ago
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.
Created attachment 161456 [details] [diff] [review]
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."
Attachment #161372 - Attachment is obsolete: true
(Assignee)

Comment 24

13 years ago
Created attachment 161517 [details] [diff] [review]
Expose high level esm::ChangeFocusWith().
Assignee: m.wargers → aaronleventhal
Attachment #161456 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

13 years ago
Attachment #161517 - Flags: superreview?(bryner)
(Assignee)

Comment 25

13 years ago
Our focus API needs to be cleaned up some day, but that's another story.
Attachment #161517 - Flags: superreview?(bryner) → superreview+
(Assignee)

Updated

13 years ago
Attachment #161517 - Flags: review?(mats.palmgren)
(Assignee)

Comment 26

13 years ago
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.
Attachment #161517 - Flags: review?(mats.palmgren) → review?(bzbarsky)
I'm not likely to get to this earlier than about a week from now...
Created attachment 163775 [details]
Another testcase
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.
Attachment #161517 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 30

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 32

13 years ago
(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?
Created attachment 164175 [details]
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.
Attachment #158257 - Attachment is obsolete: true
(Assignee)

Comment 34

13 years ago
That's weird. In your tests does directly relate to the position:fixed element?
Or can you repro the problem without that somehow?
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? 
(Assignee)

Updated

13 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 36

13 years ago
Created attachment 164192 [details]
Click on named anchor link, tab skips a link
Attachment #164175 - Attachment is obsolete: true
(Assignee)

Comment 37

13 years ago
Created attachment 164197 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #164197 - Flags: review?(mats.palmgren)

Updated

13 years ago
Blocks: 267427
(Assignee)

Updated

13 years ago
Attachment #164197 - Attachment is obsolete: true
Attachment #164197 - Flags: review?(mats.palmgren)
(Assignee)

Comment 38

13 years ago
Created attachment 164752 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #164752 - Flags: review?(mats.palmgren)
(Assignee)

Comment 39

13 years ago
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.
Whiteboard: Seeking r=mats
(Assignee)

Updated

13 years ago
Summary: clicked anchor with location hash set to element id gives first child anchor focus rectangle → clicked anchor with location hash set to element id gives first child anchor focus rectangle & selection not always updated correctly
Attachment #164752 - Flags: superreview+
Attachment #164752 - Flags: review?(mats.palmgren)
Attachment #164752 - Flags: review+
(Assignee)

Comment 40

13 years ago
Checking in nsPresShell.cpp;
/cvsroot/mozilla/layout/html/base/src/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.790; previous revision: 3.789
done
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
Whiteboard: Seeking r=mats

Comment 41

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

Comment 42

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

Comment 43

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

Updated

12 years ago
Depends on: 308064
This caused bug 308064.  That seems like a pretty serious annoyance for keyboard users...

Updated

12 years ago
Depends on: 322588
You need to log in before you can comment on or make changes to this bug.