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: