61224 - Links do not reach empty anchors

Status: VERIFIED DUPLICATE of bug 38280

(Core :: Layout, defect, P2)

Description

[Srinivasa Rao Borusu]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.0; Windows NT; DigExt)
BuildID:    Netscape 6 (Mozilla/5.0 20001108)

I created a link (to top of the page) in the bottom of the page and clicked on 
the link. It doesn't reach top of the page.

Reproducible: Always
Steps to Reproduce:
1. Open a new composer window.
2. Insert a Named anchor "Test" at the top of the page.
3. Scroll down to bottom of the page by pressing enter key such that the 
created anchor goes beyond the visibility.
4. Create a link at the bottom of the page. The link should point to "Test" 
target.
5. Save the page.
6. Open the page in browser and scroll down and click on the link.

Actual Results:  It didn't reach the target (Top of the page)

Expected Results:  It should reach the target "Test" (Top of the page)

Comment 1

[rubydoo123]

links work fine on win95 using the release build. I selected to edit this page
(the bug report). At the text string 'Bugzilla version 2.11' I inserted an
anchor with the name 'test.' I then scrolled to the bottom of the page and
highlighted 'Expected Results' and selected LInk from the toolbar. I selected
'test' as the target name. Saved the file and rendered it in the browser.
Scrolled to the bottom and selected the link -- it works fine.

Comment 2

[Srinivasa Rao Borusu]

It doesn't reach the target properly. After clicking I have to scroll upwards to  
see the top of the page (Target Place). Its working fine with netscape 4.73.
Now try to reproduce this...

Comment 3

[rubydoo123]

assigning to kin, kin can you check the positioning after the link selection.

Comment 4

[kinmoz]

Changing status to NEW since I see this bug. This isn't a composer bug, it's 
actually a bug in layout's nsPresShell::ScrollFrameIntoView() method.

Comment 5

[kinmoz]

Not sure who owns PresShell ... let's start with [e-mail] since he 
owns layout.

Comment 6

[buster]

nsPresShell::ScrollFrameIntoView is working correctly.
The problem is *which* frame we're scrolling into view.
Consider the following source, an empty anchor followed by text (the original
test case for this problem); and an anchor containing text:
  <a name="top"></a>top <a name="top2">x</a>

The frame model we'll get for that is something like this:
  Inline(a)(1) next=00E7BD58 {975,225,0,0}
  Text(2)@00E7BD58[0,4,T]  next=00E7BD94 {975,0,360,285}
    "top "
  >
  Inline(a)(3) next=00E7BE08 {1335,0,105,285}
    Text(0)@00E7BDCC[0,1,T]  {0,0,105,285}
      "x"
    >
  >

Notice that the first anchor is 0 width and 0 height, and is bottom-aligned
within the line that it's on.  That's correct for an empty inline frame
according to CSS2.  When we scroll this empty inline frame into view, it is
entirely in view when the bottom edge of the containing line is in view.  Just
what we asked the pres shell to do!
Compare this to second anchor.  To scroll it entirely into view, we need to
scroll the entire line into view, because it is as tall as the line it is
within.  So this case works as expected.

I think the way to fix this is to have the code that determines which frame to
scroll to, PresShell::GoToAnchor(), do the work to find a more appropriate
frame.  In this case, I think it should detect that the anchor is 0-height, and
scroll in the line frame instead.

Comment 7

[buster]

my last suggestion won't quite work, because lines do not implement nsIFrame.

A possible solution would be for the anchor frame itself to store the line
height as a frame property.  It would do so only in the case where it was
0-height, so the average additional overhead would be very minimal.
PresShell::GoToAnchor()would know to look for a 0-height anchor and grab the
frame property.  An additional parameter would need to be added (with a default
value, to preserve existing usage) to PresShell::ScrollToFrame() to override the
frame height with a given value.

This almost certainly won't make the cut for the mozilla 1.0 release unless
someone can present compelling argument for bumping up it's priority.  It would
cost me one full day to get this right, I think.

Comment 8

[buster]

Attachment: test case

Comment 9

[buster]

or, we could get someone smarter than me to do it.  :)
dbaron has this one nailed, dup of bug 38280

*** This bug has been marked as a duplicate of 38280 ***

Comment 10

[Loco]

VERIFIED.