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: