Closed
Bug 258514
Opened 20 years ago
Closed 20 years ago
clicked anchor with location hash set to element id gives first child anchor focus rectangle & selection not always updated correctly
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mazarin, Assigned: aaronlev)
References
(Depends on 1 open bug, )
Details
(Keywords: testcase)
Attachments
(5 files, 6 obsolete files)
2.97 KB,
text/html
|
Details | |
9.96 KB,
patch
|
MatsPalmgren_bugz
:
review+
bryner
:
superreview+
|
Details | Diff | Splinter Review |
1.43 KB,
text/html
|
Details | |
484 bytes,
text/html
|
Details | |
4.79 KB,
patch
|
bryner
:
review+
bryner
:
superreview+
|
Details | Diff | Splinter Review |
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•20 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•20 years ago
|
Assignee: firefox → nobody
QA Contact: firefox.general → core.layout
Comment 2•20 years ago
|
||
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•20 years ago
|
||
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•20 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...
Comment 5•20 years ago
|
||
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•20 years ago
|
||
ccing some people who may know why we do what we do (and whether we want to do it).
Comment 7•20 years ago
|
||
*** Bug 263034 has been marked as a duplicate of this bug. ***
Comment 8•20 years ago
|
||
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•20 years ago
|
||
From the duped bug 263034.
Comment 10•20 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•20 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.
Comment 12•20 years ago
|
||
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•20 years ago
|
||
Try just esm->SetFocusedContent(nsnull); // Set focus to document How does that test out?
Comment 14•20 years ago
|
||
(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•20 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);
Comment 16•20 years ago
|
||
(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•20 years ago
|
Attachment #161372 -
Flags: review?(aaronleventhal)
Assignee | ||
Comment 17•20 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•20 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•20 years ago
|
Attachment #161372 -
Flags: superreview?(dbaron)
Comment 19•20 years ago
|
||
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•20 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+
Comment 21•20 years ago
|
||
(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•20 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.
Comment 23•20 years ago
|
||
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•20 years ago
|
||
Assignee: m.wargers → aaronleventhal
Attachment #161456 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Attachment #161517 -
Flags: superreview?(bryner)
Assignee | ||
Comment 25•20 years ago
|
||
Our focus API needs to be cleaned up some day, but that's another story.
Updated•20 years ago
|
Attachment #161517 -
Flags: superreview?(bryner) → superreview+
Assignee | ||
Updated•20 years ago
|
Attachment #161517 -
Flags: review?(mats.palmgren)
Assignee | ||
Comment 26•20 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)
Comment 27•20 years ago
|
||
I'm not likely to get to this earlier than about a week from now...
Comment 28•20 years ago
|
||
Comment 29•20 years ago
|
||
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•20 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
Closed: 20 years ago
Resolution: --- → FIXED
Comment 31•20 years ago
|
||
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•20 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?
Comment 33•20 years ago
|
||
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•20 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?
Comment 35•20 years ago
|
||
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•20 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 36•20 years ago
|
||
Attachment #164175 -
Attachment is obsolete: true
Assignee | ||
Comment 37•20 years ago
|
||
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•20 years ago
|
Attachment #164197 -
Flags: review?(mats.palmgren)
Assignee | ||
Updated•20 years ago
|
Attachment #164197 -
Attachment is obsolete: true
Attachment #164197 -
Flags: review?(mats.palmgren)
Assignee | ||
Comment 38•20 years ago
|
||
The patch should be easy to review -- it's only large because of the indentation change in some parts.
Assignee | ||
Updated•20 years ago
|
Attachment #164752 -
Flags: review?(mats.palmgren)
Assignee | ||
Comment 39•20 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•20 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
Updated•20 years ago
|
Attachment #164752 -
Flags: superreview+
Attachment #164752 -
Flags: review?(mats.palmgren)
Attachment #164752 -
Flags: review+
Assignee | ||
Comment 40•20 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
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Whiteboard: Seeking r=mats
Comment 41•20 years ago
|
||
*** Bug 270349 has been marked as a duplicate of this bug. ***
Comment 42•20 years ago
|
||
*** Bug 217366 has been marked as a duplicate of this bug. ***
Comment 43•20 years ago
|
||
*** Bug 217366 has been marked as a duplicate of this bug. ***
Comment 44•19 years ago
|
||
This caused bug 308064. That seems like a pretty serious annoyance for keyboard users...
You need to log in
before you can comment on or make changes to this bug.
Description
•