Closed Bug 55244 Opened 24 years ago Closed 24 years ago

surfing to a named anchor stop working

Categories

(Core :: DOM: Navigation, defect, P3)

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: cmaximus, Assigned: adamlock)

References

(
URL
)

Details

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

Attachments

(6 files)

Patch to presshell to fix anchor scrolling
2.82 KB, patch
Details | Diff | Splinter Review
New patch scrolls to any element with a matching "id" attr or the <A> tag with a matching "name" attr.
2.71 KB, patch
Details | Diff | Splinter Review
Some sample XHTML with anchors, save as xhtml-anchor.xml or somesuch and use "Open File..." to load it
17.66 KB, text/plain
Details
Updated to do XML documents - new code looks for matching anchor tag in the XHTML namespace in XML documents. Review please.
3.86 KB, patch
Details | Diff | Splinter Review
CSS for my next test case
54 bytes, text/css
Details
Testcase: XML file with XML ID target and HTML targets
2.46 KB, text/xml
Details
If you have a page with an anchor in it clicking the link to the anchor does absolutely nothing. You can check this out at my test page located above. For kicks tryout the Netscape homepage which now features copious use of anchors. To Repro: Load page. Click link. Wait patiently. Expected: Scroll page to linked anchor. I reproduced this with 2000100408 BRANCH builds (Win98). Early evidence says this was not in the branch in yesterday's builds. This would be bad mmmkay.
dup of 55252 "Unable to follow the target link # within the html page" *** This bug has been marked as a duplicate of 55252 ***
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → DUPLICATE
*** Bug 55252 has been marked as a duplicate of this bug. ***
not to be a stickler but 55252 and 55249 are dupes of _this_ bug. Established precedent is to dupe the newer on the old. Plus it was silly to then go and change the assignees to be the same this one already had. That alone would be reason enough to leave this one even if it did have a higher number.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
I would like to see a rtm and/or mozilla0.6/9 as keyword.
adding rtm ... we don't want to have a bad first hand impression ...
Keywords: rtm
One clue is that if you browse to the URL including the anchor it works, but it won't scroll to an anchor on the current page. I've also noticed that anchor scrolling doesn't happen until loading completes even long after the anchor has loaded - is this right? Investigating further...
CC'ing Rod. Rod Spears recently modified the behaviour of the nsHTMLDocument::MatchId() function: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&fi le=nsHTMLDocument.cpp&root=/cvsroot&subdir=mozilla/layout/html/document/src&comm and=DIFF_FRAMESET&rev1=3.287&rev2=3.288 It's possible that the removal of the "name" attribute test broke the anchor scrolling code. When nsDocShell::LoadURI is called docshell is correctly performing the anchor scrolling code, but it is failing to find the named element so it returns without doing anything. The problem is that GetElementById is searching through the document elements and not finding the item. Someone familiar with the HTML dom code might also like to comment whether it is right for presshell to call GetElementById to find an anchor element's "name" attribute. A stack trace: nsHTMLDocument::GetElementById(nsHTMLDocument * const 0x05e2b274, const basic_nsAReadableString<unsigned short> & {...}, nsIDOMElement * * 0x0012f6fc) line 2353 + 27 bytes PresShell::GoToAnchor(const PresShell * const 0x05e6fd70, const nsString & {...}) line 2884 + 54 bytes nsDocShell::ScrollIfAnchor(nsDocShell * const 0x04a789a0, nsIURI * 0x04aa9750, int * 0x0012fabc) line 3644 + 45 bytes nsDocShell::InternalLoad(nsDocShell * const 0x04a789a0, nsIURI * 0x04aa9750, nsIURI * 0x066a1d90, nsISupports * 0x00000000, int 0x00000001, int 0x00000000, const char * 0x0012fbb4, nsIInputStream * 0x00000000, nsIInputStream * 0x00000000, unsigned int 0x00200001, nsISHEntry * 0x00000000) line 2875 + 23 bytes nsWebShell::HandleLinkClickEvent(nsIContent * 0x05ec0f20, nsLinkVerb eLinkVerb_Replace, const unsigned short * 0x04b1dd60, const unsigned short * 0x100a66c8 gCommonEmptyBuffer, nsIInputStream * 0x00000000, nsIInputStream * 0x00000000) line 838 OnLinkClickEvent::HandleEvent() line 692 HandlePLEvent(OnLinkClickEvent * 0x04b1d6e0) line 706 PL_HandleEvent(PLEvent * 0x04b1d6e0) line 576 + 10 bytes PL_ProcessPendingEvents(PLEventQueue * 0x00a7aa00) line 509 + 9 bytes _md_EventReceiverProc(HWND__ * 0x01f20208, unsigned int 0x0000c121, unsigned int 0x00000000, long 0x00a7aa00) line 1054 + 9 bytes USER32! 77e71820() 00a7aa00()
In the 20001005** builds, this is broken even when just browsing the URL. The last M18 build in which anchors work for me under PC Linux is 2000100408.
Not 100% sure, but it seems like the anchor-finding code in PresShell, which is searching for a tag that looks like this: <a name="foo"> Should be calling nsHTMLDocument::GetElementsByName, then since this will potentially return a list of tags, just grab the first item off the list? Was this the behaviour of nsHTMLDocument::GetElementById before the change?
http://www.w3.org/TR/html401/struct/links.html#h-12.1.3 <a name> and <a id> should both work as named anchors, but I don't sure what the correct search order is.
Please review the patch. I've modified PresShell::GoToAnchor so that it uses GetElementById and then tries GetElementsByName if that fails. It also has a test to ensure the matched element really is an anchor because it would have scrolled to anything before.
Status: REOPENED → ASSIGNED
According to another part of the spec, the anchor names must be unique, and share the same namespace with ids, so search order is not specified and *should* not matter... ;) http://www.w3.org/TR/html401/struct/links.html#h-12.2.1 In other words, Adams fix above looks both complete and correct.
*** Bug 45794 has been marked as a duplicate of this bug. ***
This look good to me, I just applied the patch and it works r=rods
*** Bug 55529 has been marked as a duplicate of this bug. ***
Adding some keywords from a dup...
Keywords: dogfood, regression
*** Bug 55588 has been marked as a duplicate of this bug. ***
I've been running with Adam Lock's patch for about 24 hours now, including a large amount of navigating through W3C recommendations thick with named anchors, and all seems well on Linux.
*** Bug 55816 has been marked as a duplicate of this bug. ***
But http://www.w3.org/TR/html401/struct/links.html#h-12.2.3 says "The id attribute may be used to create an anchor at the start tag of any element (including the A element)." In other words, *anything* (not just <a>) can be an anchor with the 'id' attribute. So I think that the first part of the patch shouldn't be so exclusive. Furthermore, I'm not convinced that "name"'d anchors only apply to <a> elements. (I.e., need the second part of the patch be as exclusive?) But somebody more familiar with the spec should pipe up.
Another interesting bit. "Destination anchors in HTML documents may be specified either by the A element (naming it with the name attribute), or by any other element (naming with the id attribute)." Redoing the patch
My only comment on the last patch is that you don't need to get the tag's name anymore ;-). So remove the first autostring ctor and the superfluous GetTagName() call, and a=waterson.
It's gone :)
When you work with GoToAnchor, please keep in mind that we use it for XML as well. In XML we can have HTML elements from the HTML namespace, so A with NAME and any HTML element with ID attribute should be matched. Additionally for "pure" XML we must explicitly say which attributes are ID attributes. We say in the internal subset: <!ATTLIST bar myid ID #IMPLIED> This says that element bar has an optional attribute named myid which is of type ID. So if I have: <bar myid="foo"> that element must be found by GetElementById("foo") as well as by GoToAnchor("foo"). So no hardwiring of nsIHTMLDocument or element/attribute names or this stops working for XML.
So is there a simple way that we can accomodate namespaces and other grammars in this code?
DOM2 Core defines getElementsByTagNameNS. I guess the easiest would be to add a new if block in case it is not an HTML document and use that method. HTML namespace is http://www.w3.org/1999/xhtml, lower case a. http://www.w3.org/TR/DOM-Level-2-Core/core.html#i-Document
*** Bug 55818 has been marked as a duplicate of this bug. ***
I think this code is mostly correct for the XML and HTML cases. I call nsIDOMDocument::GetElementById first so this code should function correctly with XML assuming that the implementation of GetElementById on the XML document is comparing the correct attribute on each element for it's id. Where it might not be correct is if you mix HTML into an XML document. In which case, how do I know what namespace is used for the HTML elements? I must pass something into getElementsByTagNameNS.
Only the HTML namespace can be used for HTML namespace. If you use other namespace we do not recognize that they are special HTML elements. So: GetElementsByTagNameNS("http://www.w3.org/1999/xhtml","a"
*** Bug 55430 has been marked as a duplicate of this bug. ***
*** Bug 55957 has been marked as a duplicate of this bug. ***
*** Bug 55922 has been marked as a duplicate of this bug. ***
*** Bug 55957 has been marked as a duplicate of this bug. ***
mostfreq keyw.
Keywords: mostfreq
Updated QA contact.
I am testing the latest patch, will probably attach another test case soon. I have a few commenst about the patch before that. 1. Do not use "<pointer> <operator> nsnull" (i.e. "content == nsnull"). Use the well-known C/C++ idioms "if (content)" or "if (!content)" instead. Specifically, if you have a nsCOMPtr and try to do "nsnull == content" (notice the order) this will not even compile on all platforms. scc says that when he is reviewing he requires one to remove all comparisons to nsnull. 2. According to our C++ portability rules you should not define variables in the for loop initializer list, i.e. replace for (PRUint32 i=0 with PRUint32 i; for (i=0
I have applied the 10/10/00 05:13 patch to my branch, and I pass my testcase 10/10/00 16:15. The testcase is an XML document, which has HTML A links to 1) HTML A with NAME, 2) HTML A with ID, 3) HTML DIV with ID and 4) an arbitrary XML element that has a declared attribute of type ID. I just noticed that I did not explicitly set the namespace for the HTML element's attributes, but it seems to work. It also works if I use them (for example I say <html:a html:name="id1">). There are different opinions on whether this should work without the explicit namespace on the attribute but lets not open that can of worms - just be happy with the way it is. If it counts any, you have r=heikki (although I would like to have those changes I suggested in my previous comment).
Assignee: valeski → adamlock
Status: ASSIGNED → NEW
over to adam who's working on this.
Fix is checked into the trunk.
Status: NEW → ASSIGNED
*** Bug 56140 has been marked as a duplicate of this bug. ***
Too many sites have intra-page links... this really makes it hard to surf... not to mention hard to use the home.netscape.com page (gulp). Marking dogfood-plus. Since a comment above says "fix checked into trunk," I assume (?) there are reviews and superreviews in hand. I'll go ahead and nominate it for rtm inclusion. I'll note that this has been running on the trunk for a day... and probably should be included in the N6 branch (so that we can use the home.netscape.com page). If there are any regressions detected in the trunk, we need to straighten the patch out there before applying it to the branch. Thanks, Jim
Whiteboard: [dogfood+][rtm+]
rtm++
Whiteboard: [dogfood+][rtm+] → [dogfood+][rtm++]
Yes, r & a=waterson@netscape.com. Fix is now checked into the branch too. Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
The fix works in all of my testcases on the 2000-10-12-06 MTrunk build.
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?
bzbarsky is right. Named anchors work within a single page, but not page-to-page. His w3c testcase is a good one.
I opened bug 56285 to track the problem that surfing to a named anchor on another page doesn't work. Opening a second bug now on the reload issue ...
... and I opened bug 56287 to track the "reload ignores named anchor" problem.
*** Bug 56428 has been marked as a duplicate of this bug. ***
*** Bug 56502 has been marked as a duplicate of this bug. ***
*** Bug 56502 has been marked as a duplicate of this bug. ***
claudius - can you verify? thanks.
QA Contact: adamlock → claudius
I opened bug 54359 almost three weeks ago which describes exactly what Boris mentioned above. There were a few supporting comments, but nobody ever confirmed that bug. Is there any speacial reason why?
VERIFIED Fixed on BRANCH and TRUNK builds with the 2000101608 builds on all platforms modulo bugs 56285 and 56287
Status: RESOLVED → VERIFIED
*** Bug 59022 has been marked as a duplicate of this bug. ***
*** Bug 59022 has been marked as a duplicate of this bug. ***
*** Bug 59022 has been marked as a duplicate of this bug. ***
*** Bug 59591 has been marked as a duplicate of this bug. ***
*** Bug 59875 has been marked as a duplicate of this bug. ***
*** Bug 61569 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: