195905 - [FIX]Clicking on link does not activate it (line with tall image and link)

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Jeremy Sanders]

If I go to the page above, and try to click on "Postscript" (in "Symbols 1-527
(also available in PostScript)."), the link is not activated but the browser
scrolls back up to the top of the image to the right of the bullet point. The
same is true for all the other links.

The html goes something like
<ul>
<li>Some text <a href="blah">link</a> <img src="large.gif">
<li>...
</ul>

This is with Mozilla 1.3b on Linux.

Comment 1

[Jeremy Sanders]

I should clarify. By experimenting, the window should be wide enough to display
the bullet point and image side-by-side, and short enough so that all the image
cannot be displayed enough. Clicking on the link moves to the top of the image,
rather than going to the link.

Comment 2

[MrJeff]

Also fails on Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b)
Gecko/20030210

Comment 3

[Boris Zbarsky [:bzbarsky]]

I bet the issue is that ScrollFrameIntoView scrolls to the top of the line box
for inline frames
(http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsPresShell.cpp#4302);
in this case the line box is huge (because of the replaced inline <img>) and so
we scroll to hell-knows-where.

The code in question was added for bug 38280, and if the link were wrapped
around the image the scrolling may indeed be desired behavior....  So we need to
try to fix this without regressing bug 38280

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about we scroll the top of the line box to the top of the scrolled area,
unless that puts the bottom of the line box below the bottom of the scrolled
area, in which case we put the bottom of the line box at the bottom of the
scrolled area?

Comment 5

[Boris Zbarsky [:bzbarsky]]

That won't really fix this bug.  The problem is that if we move the <a> such that the
mouseup no longer happens inside it, we get no click event on it, hence do not follow
the link.  With that proposal, following the link will become possible
if it is near the bottom of the viewport, but not otherwise.

On the other hand, this is exactly the distinction that's supposed to
be addressed by the scroll position flags, no?

The behavior of SCROLL_ANYWHERE and SCROLL_IF_NOT_VISIBLE is not clearly
differentiated in nsIPresShell, but I'd think that one of them would do the right thing
here....  (in fact, the definition of SCROLL_ANYWHERE sounds like
SCROLL_IF_NOT_VISIBLE to me; all this stuff could use some documenting
and cleanup  :( )

Comment 6

[Boris Zbarsky [:bzbarsky]]

*** Bug 200332 has been marked as a duplicate of this bug. ***

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 200565 has been marked as a duplicate of this bug. ***

Comment 8

[kinmoz]

Attachment: Patch to prevent scrolling when focusing by mouse click.

I don't think it's a problem with the flags. One of the problems is that the
rect we are trying to scroll into view is wrong ... it's the dimension of the
frame we truly want to scroll placed at the top of the line. That's what
dbaron's bug  202977 is about.

So what will this patch I am attaching break? It suppresses scrolling when we
focus content by a mouse click. We really shouldn't be scrolling objects when
the user clicks on them since as this bug points out, we could scroll it out
from underneath the mouse.

Comment 9

[Boris Zbarsky [:bzbarsky]]

That will make some things weird... eg clicking a link or textbox that's
partially offscreen will not scroll to it.

I'm not sure whether scrolling to such is desirable or not, come to think of
it... aaronl?  Thoughts?

Comment 10

[kinmoz]

Comment on attachment 121440 [details] [diff] [review]
Patch to prevent scrolling when focusing by mouse click.

Doh, I just realized I attached a slightly older diff file, the only difference
between what is in my tree and the attached patch is that I have:


+  else if (didSuppress && focusController) {


instead of:


+  else if (didSuppress) {

Comment 11

[Aaron Leventhal]

Boris, in my current build I don't see that clicking in a textfield that's
partially offscreen scrolls to it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w FOR REVIEW ONLY

The basic idea here is that if we are asked to scroll a frame "anywhere" and
the frame has a nonzero height, then we should not get the line box.  This
keeps the case of "link to named anchor" working, since that scrolls to "top",
not "anywhere", and deals with the original problem in bug 38280 (which was an
inline frame with a zero height).

I've tested all the testcases in both bugs, and this seems like the most
reasonable approach...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132259 [details] [diff] [review]
Same as diff -w FOR REVIEW ONLY

David, what do you think?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 132259 [details] [diff] [review]
Same as diff -w FOR REVIEW ONLY

>+  if (frameBounds.height == 0 || aVPercent != NS_PRESSHELL_SCROLL_ANYWHERE) {

I think I prefer not checking the height here.	I think it's better to get the
whole line in view in the general case.  (Consider a definition in the middle
of a paragraph with a tall image in the line being scrolled to.)  r+sr=dbaron
with that change.

I also wonder how this interacts with the things discussed in bug 66619.

Comment 16

[Boris Zbarsky [:bzbarsky]]

David, the line height check is so that we will get the whole line into view
_either_ if the scroll is "not anywhere" _or_ if the height is 0.  Removing that
check makes us scroll the whole line into view in fewer cases (and you say you
want that to happen as often as possible)...  It still won't regress bug 38280,
so I don't mind removing that check, but it's your call.  Please let me know.

As for bug 66619, that bug and this one are diametrically opposed in the current
setup.  Both clicking on the link and tabbing to it focus it; focus is what
triggers the scrolling.  In the click case, you want the link to scroll as
little as possible, otherwise you get this bug.  In the tab case, bug 66619 is
asking for a lot of scrolling (scrolling to the top of the link no matter what,
basically).  As long as both scrolls are triggered from the same place in the
code with identical values of the "scroll percentage", there is no way to fix both.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, right.  OK.  Never mind.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Checked in