Closed Bug 56285 Opened 24 years ago Closed 24 years ago

surfing to a link with named anchor doesn't take you to anchor point on page

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: ekrock, Assigned: adamlock)

References

(
URL
)

Details

(Keywords: html4, regression, Whiteboard: [rtm++])

Attachments

(4 files)

Testcase: case sensitivity
1.25 KB, text/html
Details
Patch to do anchor scrolling via events
6.00 KB, patch
Details | Diff | Splinter Review
New patch following Vidur's suggestion to use FlushPendingNotifications(). Cuts down the complexity a lot.
3.28 KB, patch
Details | Diff | Splinter Review
Updated patch including removal of mRefContent
3.89 KB, patch
Details | Diff | Splinter Review
----------------------------------------------------------------------------- ------- Additional Comments From Boris Zbarsky 2000-10-12 10:28 ------- Hmm. I'm still seeing a problem related to this (using the 2000101208 build). Go to: http://www.w3.org/TR/html4/ scroll down to the "Full Table of Contents" and click on one of the links that are not top-level (eg. "Copyright Notice" under "About the HTML 4 Specification" heading). Mozilla will go to the right page, but not scroll to the anchor. If I just type the URL in the url bar or into the open location window, it works fine. Also, if I go to a named anchor and then reload the page, I get scrolled back to the top. This is patently wrong. Should I file a separate bug, or should this one be reopened? ------- Additional Comments From Jeffrey Baker 2000-10-12 10:33 ------- bzbarsky is right. Named anchors work within a single page, but not page-to-page. His w3c testcase is a good one. ----------------------------------------------------------------------------- We must fix this. This is basic HTML 1.0; named anchors are used all over the web to point into specific locations of other documents. Marking P1 as high-profile backward compatibility (definite stop-ship bug; "Netscape browser doesn't support HTML links?") and nominating RTM. Opening separate bug to track named anchor reload issue.
Assignee: clayton → sgehani
Keywords: 4xp, html4, rtm
Priority: P3 → P1
rtm,html4,4xp. P1 high profile backward compatibility. (Basic HTML links!) RTM stop-ship. Reassigning to sgehani@netscape.com so doesn't languish in clayton's list.
Keywords: regression
Erik: who did you really intend to assign this to? Samir works on install issues, this bug belongs in Gecko-land
Assignee: sgehani → clayton
This works for me, but if it doesn't for others it could be a race/timing issue. Same goes for the reload situation which is probably a facet of the same problem. Here's an anchor URL to try directly: http://www.w3.org/Consortium/Legal/ipr-notice-20000612#Copyright
WFM buildid 2000101208 WinNT. I was able to reproduce on 10-10-00 build no problem. Somebody fixed this already.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → WORKSFORME
Yes, links _within_ a document have been fixed. However, linking to a named anchor in a different document breaks. I've tried that with buildids 2000101208 and 2000101212 and it doesn't work for me. I'm using Linux, not NT, though.
2000101212/MN6 on Win98 doesn't work.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
dup of bug 46877?
I don't think this is a dup of bug 46877, which has to do with remembering the scroll position. However, I *do* think this is a dup of bug 45338 (Scrolling to an anchor on page load does not work)
Very basic functionality, very important for lots of real-wrold uses. I think this is a show-stopper that isn't getting any attention. assigning to adam, who fixed similar problems recently.
Assignee: clayton → adamlock
Status: REOPENED → NEW
Keywords: dogfood
Right, this bug is not a dup of bug 46877. This bug is about not jumping to the anchor point when navigating to a separate page; bug 46877 was about not remembering the scroll position. *If* behavior is identical when typical a URL with an anchor into the URL bar (bug 45338) and when clicking a link in the page (this bug 56285), then bug 45338 and bug 56285 may be DUPs. Suggest that Nisheet fix bug 45338 or Adam fix bug 56285 and then see whether both bugs disappear, rather than working on separate fixes independently.
Bug 45338 looks like it's exactly describing this problem. Content sink code is here: http://lxr.mozilla.org/seamonkey/source/layout/html/document/src/nsHTMLContentSi nk.cpp The method ProcessATag gets called for each anchor element to be parsed. This does the compare to the ref in the URL but only to A tags with a "name" attribute. It also does a string insensitive compare when it should be case sensitive. Scrolling only happens after loading completes. So the things to do are: 1. All new elements need to be tested to see if they match the ref, not just the anchor. 2. Scrolling to the anchor should happen earlier, possibly following the first reflow following a sucessful match and thereafter (incase of strange reflow in tables moves the anchor position)
Nisheeth, adding you to CC here. This looks like dup of your bug 45338. I agree with Adam's assessment on what seems to be the correct thing to do. This shouldn't slow document load too much since in the normal case there would be no ref (so there would be one extra function call and one null pointer check per element compared to the current situation). The ProcessATag method should also be extended to test for id attribute. An interesting note: IE does case-insensitive match, but NN 4.7 is case sensitive. I'll attach a test case. I second Adam in that we should be case sensitive here because we are case-sensitive in inter-page links. The attachment will have 2 A HREF links to 2 A NAME locations. The first A NAME location is "foobar" and the latter is "FOOBAR".
A danger of our being case-sensitive when IE is case-insensitive: given IE's current market share and case-insensitivity, lots of people may write sloppy HTML links that *depend* on case-insensitive matching to work correctly. a) What does the HTML 4 spec mandate: case sensitivity or insensitivity in link anchor matching? b) Would we be causing any other problems if we adopted more-foregiving case-insensitive matching on links/anchors? What I'm concerned about is the risk that folks find themselves unable to browse web content and follow links because of sloppy IE-tested HTML that has random capitalization on the two sides. They then consider this a Moz/N6 product bug and give up in frustration. Also, we might then have to add "case consistency in web links and anchors" to our list of things to evangelize--ouch! Of course, we could always go with strict case-sensitive matching for RTM and then fall back to case-insensitive matching as a "bug fix" in post-RTM versions if it turns out to be a problem. Thoughts?
The spec says that the NAME attribute to the A tag is case-sensitive and furthermore that it shares the same namespace as the ID attribute (which is also case-sensitive). So we may need to be case-sensitive just to handle multiple named anchors whose names differ only by case on the same page (granted, an unlikely scenario). I have seen very few cases of anchor names that are not all lower-case, so I am not sure compatibility with IE on this will be a problem.
Reading the bug, it sounds like the title needs to be corrected, and the issue is whether anchors should be searched for with case insensitivity. I assume this works fine as is on 99% of sites, and hence is not a dogfood usage stopper. Please correct me (renominate) if I'm mistaken. Marking dogfood-minus
Whiteboard: [dogfood-]
jar@netscape.com, i believe you have misread the bug. The bug is indeed very serious. For example: 1) go to http://www.w3.org/TR/html4/ 2) scroll down to the table fo contents, 12.2.1 "Syntax of anchor names" 3) Click the link Actual result: you are taken to the top of chapter twelve. Expected result: you are taken directly to section 12.2.1 This is a grave bug that breaks a feature which has been in HTML forever. It does not work on %99 of sites. It is certainly broken at http://www.w3.org/, http://www.tbtf.com/, and others. Curiously, it is *not* broken at lxr.mozilla.org, which can take you straight to the line number in the source you are looking for. Is this because lxr's anchors are only #[0-9]+ ?
Let's keep one issue per bug, please. This bug seems to cover that links to a named anchor within a separate page don't work in most cases, whereas they do work when going to the same anchor when the page is already loaded. Discussion of case sensitivity in general should be a separate bug if someone thinks we want to give in to IE (and break backwards-compatibility with correct pages that worked in 4.x). Clearing [dogfood-] since jar said "Please correct me (renominate) if I'm mistaken."
Whiteboard: [dogfood-]
Also look at http://www.nevod.ru/linux/doc/lig/Lig.html Press a link in the left frame. 2000101408-Mtrunk
Having stepped through the content sink code, my impression is that the scrolling code (whether correct or not) may be firing before the layout has done the reflow that creates the associated view for the element. This has the means that scrolling to the element does nothing at all. I have a patch that updates the content sink scrolling code so it uses GoToAnchor in the pres shell rather than duplicating code but I need to be able to force a reflow so there is a view to scroll to. Anyone know how I can do this?
This looks like a duplicate of 54359
*** Bug 54359 has been marked as a duplicate of this bug. ***
Didn't Boris say *this* looks like a duplicate? Bug 54359 is much older, and I thought it would be current practice to resolve the newer bugs as dups of the older ones.
This bug is better analyzed. We don't always mark newer bugs duplicates of older ones.
The problem clearly seems to be a layout issue. I've discovered that the scrolling code does work in that it correctly locates the named element (though only for anchor tags), but it is trying to do the scrolling from the call to HTMLContentSink::DidBuildModel(). At this point, the content has been parsed but not laid out so scrolling does nothing. Scrolling might work for longer documents where incremental reflows have washed over the anchor element before the call to DidBuildModel. Once the reflow (including one to the anchor element) has happened the element's container frame has valid dimensions and can be scrolled to. The most straightforward way to solve this may be to force a reflow before the scroll attempt or to post the scroll operation onto the message queue so it is processed after the reflow events. I need some input from the layout people on the best way to proceed.
Would it be too late to run this with the onload handlers? That should get around any potential async reflow problems (esp. with respect to nested IFRAMEs n' stuff). The down side is that you'll not jump to the anchor until the entire page (images and all) load, which'd be different that 4.x behavior IIRC.
Doing it that late makes things a bit difficult for users viewing long pages over a modem connection (eg. following a link into the middle of one of the W3C specs...). This does not mean that this should not be handled by the onload handlers, but we should consider the usability impact. Then again, it would be better usability than what we have now. :)
Here is a patch which follows through with one of these ideas. I've changed the implementation of GoToAnchor so it uses the event queue to do anchor scrolling. PresShell::GoToAnchor posts a GoToAnchorEvent which is handled by calling InternalGoToAnchor (the old anchor scrolling code). This puts the scrolling after the reflow. I've also changed the content sink so it calls GoToAnchor on the presshell rather than doing the scrolling in its own way. Comments and review please.
Status: NEW → ASSIGNED
Some comments, haven't tried compiling so take these with a grain of salt... 1. Could you make the event be a class instead of struct, and "private:" the private members? 2. You can get rid of the reinterpret_cast by doing: nsIPresShell *ips = presShell.get(); PresShell* ps = NS_STATIC_CAST(PresShell*, ips); 3. In PresShell::GoToAnchor() you are doing a C-style cast and then a static_cast which doesn't seem too good. Also, you are not testing for an out of memory condition. Would this work? GoToAnchorEvent *e = new GoToAnchorEvent(NS_STATIC_CAST(nsIPresShell*,this), aAnchorName); if (!e) return NS_ERROR_OUT_OF_MEMORY;
*** Bug 56899 has been marked as a duplicate of this bug. ***
Instead of opening a new bug report... I wanted to point out that currently (win build 2000101604) if you click on an anchor that links to an image above it, mozilla scrolls to the bottom edge of the image instead of the top of the image like NAV4 and IE5 do. I think that it is confusing that you have to scroll further up after clicking the anchor to see the actual image.
That should be part of bug 38280, not this one.
Heikki, all your points are valid and I've updated my code to reflect the changes. I've left the awkward cast which is there because nsIPresShell::GoToAnchor is declared as a 'const' method for some unknown reason.
OS: Windows NT → Mac System 7
*** Bug 56287 has been marked as a duplicate of this bug. ***
OS=ALL? (the other bug is OS=WinNT)
This is a problem on Linux too. Sounds like OS=ALL....
Whiteboard: [rtm need info]
Marking [rtm need info] (I think it's OK for me to do that, right? apologies if only PDT is supposed to do that) to indicate that Adam is actively working on a fix and to differentiate from untriaged rtm nominees. Personlly, I believe we should accept a patch for this bug if reviewed & super-reviewed and deemed safe but of course PDT will have to make the final call.
As I understand it, this bug is about case-sensitivity in the search for the anchor. For most sites, this is not a problem (certainly lxr is working well, as is the Netscape home page, and tons of others). I can see that stressing this out with case variance is interesting... but I don't think it is common, and hence dogfood Marking dogfood minus Please be sure to update this bug with information suggesting that this problem is more widespread than I realized. Thanks, Jim
Whiteboard: [rtm need info] → [rtm need info][dogfood-]
Jim, it seems that you are intentionally ignoring my examples of www.w3.org and www.tbtf.com, which are two that I found in only a few seconds of looking. These examples are not related to case sensitivity as far as I can tell. They are clera evidence that the problem is more widespread than you believe.
Whiteboard: [rtm need info][dogfood-] → [rtm need info]
No it's not about case sensitivity. It's about the anchors not working when browsing to a new page because the document has not reflowed so scrolling does nothing. The patch fixes that. It also fixes the case sensitivity problem which was noticed during investigation and several other issues by throwing away the content sink's anchor scrolling code in favour of the correct implementation in the presshell. The current behaviour is just plain wrong and very noticeable. It should be fixed. Removing dogfood-.
Jim, this is an important fix that can affect a large number of existing pages. Adam, could this not be reduced to just a 1 line change (the addition of the FlushPendingNotifications call) if you leave the existing mechanism of recording the referenced content in the HTMLContentSink in place? It seems like the existing HTMLContentSink mechanism is potentially more efficient than the depth-first search in nsPresShell::GoToAnchor, at the cost of a bit of redundancy.
This can not be reduced to 1 line change. The content sink only finds A elements with NAME attribute, and even that is case-insensitive, and finally it does not scroll because the page has not been laid out yet. So there are several bugs here, even tghough they should all be fixed at the same time, IMHO.
A elements with NAME attributes (named anchors) are the only elements we're supposed to scroll to. And the FlushPendingNotifications should do the final layout...or am I missing something?
I believe we also have to scroll to any element if its ID matches the named anchor. (the ID attribute and the NAME attribute of A share a namespace, so there should be no ambiguity).
We went through all the issues with scrolling in bug 55244. Basically the spec says, anchor scrolling should work with an A tag with a matching "name" attribute, or any element with a matching "id" attribute, or in XML any element which has identifier attribute. All comparisons should be case sensitive. Rather than fixing the content sink, it seemed sensible to delete the broken anchor scrolling from the content sink and call the presumed correct implementation in the presshell. And do FlushPendingNotifications of course.
My mistake (or ignorance, really). sr=vidur for the last patch.
Thanks Vidur, fix checked into the trunk
This should get a rtm+. It is basic browser functionality that confuses people heavily when it doesn't work. It will certainly be pointed out in the reviews if this doesn't work. It has a r=, sr= and has been checked into the trunk.
Adam, you can also nuke mRefContent from the nsHTMLContentSink.
Actually, you MUST get rid of mRefContent, the destructor is now trying to release an unitialized pointer!
Many thanks for the fix! It works again now (for me), both on Win NT (build 2000101820) and on Linux (source build from CVS).
Getting rid of mRefContent would be nice, but it's not necessary since HTMLContentSink has a NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW, so mRefContent will be nsnull in the destructor. We should get rid of the pointer on the trunk at some point, but it's not required for the branch, IMO
The offending mRefContent got zapped in the trunk. I reckon the code is now ready for the branch so marking pdt+. Speak up now if anyone still sees problems.
Whiteboard: [rtm need info] → [rtm need info][pdt+]
Did you mean to mark [rtm+] instead of [pdt+]?
Yes indeed I do :) Updated.
Whiteboard: [rtm need info][pdt+] → [rtm need info][rtm+]
Ok, the updated patch that also gets rid of mRefContent and mNotAtRef (yes! more deletions!) looks good to me, r=heikki.
Whiteboard: [rtm need info][rtm+] → [rtm+]
Oh boy, I just realized that we do nothing for XML. IE5 can follow these links (except the last one because it does not recognize ID type attributes), but we can't (try clicking these URLs from here, or write them in URLbar): http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16714#id1 http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16714#id2 http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16714#id3 http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16714#id4 Adam, care to fix this for XML as well?
Whiteboard: [rtm+] → [rtm need info]
When Vidur blesses the patch you're going to check in, then update the bug with that comment and put rtm++ in the whiteboard. Any change related to XML is NOT covered by this provisional approval.
Why did this get knocked back to rtm need info? Do we still have an automatic rtm++ once we get a r=vidur?
Heikki, I've only modified the HTML content sink, not the XML content sink so XHTML probably doesn't go into the code I've just written. Rather than hold up this bug any longer I suggest you raise a new bug for the XML issue.
I agree with Adam. The XML/XHTML anchors issue warrants a separate bug. sr=vidur for the latest patch and let's close this one out.
rtm+ again...
Whiteboard: [rtm need info] → [rtm+]
rtm++
Whiteboard: [rtm+] → [rtm++]
Fix checked into the branch.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
*** Bug 57784 has been marked as a duplicate of this bug. ***
Yay! VERIFIED on Mac 10-24, Win NT 10-24 and Linux 10-24 branch bits. Marking vtrunk? If it shouldn't be added, take it out and do a final VERIFIED mark. -d
Status: RESOLVED → VERIFIED
Keywords: vtrunk
Yay! VERIFIED on Mac 10-24, Win NT 10-24 and Linux 10-24 branch bits. Marking vtrunk? If it shouldn't be added, take it out and do a final VERIFIED mark. -d
I'm an idiot. Must leave in RESOLVED FIXED mode. Vtrunk is marked from before though...
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Last step.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Verified Fixed on Mozilla trunk builds. The http://www.w3.org/TR/html4/ links work when opened in same and new windows. The case sensitivity testcase also passes. linux 102408 RedHat 6.2 win32 102404 NT 4 mac 102308 Mac OS9 Setting bug to Verified and removing vtrunk keyword.
Status: RESOLVED → VERIFIED
Keywords: vtrunk
*** Bug 57813 has been marked as a duplicate of this bug. ***
Does not work with cyrillic "anchors". 1. Load http://www.fcenter.ru/wn/hn02082000.htm 2. click on the first link (3dfx...) 3. Works fine. 4. Go back. 5. Now select the second link 6. Nothing happens, only the URLbar updates to http://www.fcenter.ru/wn/hn02082000.htm#%E1%E5%EB%E0%FF Is this a new bug or the same? Please open another one and tell here the number or reopen this one. Tnx.
The links are working fine for me on NT, Commercial release (branch) build 2000-10-31-14.
Also working for me on Linux trunk build 2000110108.
Hmm... I use moz 2000103004-mtunk-talkback. And after restart I see this. I'll try tomorrow with a new build.
Filed bug 58819 on the Cyrillic anchors issue.
*** Bug 59206 has been marked as a duplicate of this bug. ***
SPAM. HTML Element component deprecated, changing component to Layout. See bug 88132 for details.
Component: HTML Element → Layout
When I click the link Go to Non-Credit Course on this page https://sr.glenbard.org/CourseOffering.php it goes to the wrong area. When I click the link Go to Credit Courses (further down) it does go to the correct place. It works properly on Chrome, Safari, and IE11
That has nothing to do with this bug. But for what it's worth, that's a quirk in the exact styling of that site: it's using an inline-block ("Credit Courses") followed by an empty anchor, and linking to that anchor, which tries to scroll to the line the anchor is on, which is not helpful in that case. It might be worth filing a separate bug on this case.

(In reply to Heikki Toivonen (remove -bugzilla when emailing directly) from comment #60)

Oh boy, I just realized that we do nothing for XML. IE5 can follow these
links
(except the last one because it does not recognize ID type attributes), but
we
can't (try clicking these URLs from here, or write them in URLbar):

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16714#id1
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16714#id2
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16714#id3
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16714#id4

Adam, care to fix this for XML as well?

Works:

But the Firefox page position is wrong for MediaWiki anchors. Examples in wikis that are using MediaWiki:

(In reply to David Hedlund from comment #85)

(In reply to Heikki Toivonen (remove -bugzilla when emailing directly) from comment #60)

Oh boy, I just realized that we do nothing for XML. IE5 can follow these
links
(except the last one because it does not recognize ID type attributes), but
we
can't (try clicking these URLs from here, or write them in URLbar):

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16714#id1
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16714#id2
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16714#id3
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16714#id4

Adam, care to fix this for XML as well?

Works:

But the Firefox page position is wrong for MediaWiki anchors. Examples in wikis that are using MediaWiki:

Sorry, this was a browser extension issue with Zoom Page WE.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: