18649 - ScrollFrameIntoView and SCROLL_IF_NOT_VISIBLE

Status: VERIFIED FIXED

(Core :: Layout, enhancement, P3)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

It would be nice if nsIPresShell::ScrollFrameIntoView would recognize a new type
of scrolling, call it:

NS_PRESSHELL_SCROLL_ONLY_IF_NOT_FULLY_VISIBLE

Currently the default behaviour after following a link is that we scroll
horizontally to the location of the target. This is annoying to say the least.
For example, if you have:

A term is this
   This is a definition, here is <target>the target</target> of some
   link.

and you have a link pointing to the target and you have a horizontal scroll bar
it will show up like this:

<target>the target</target> of some

and you need to scroll left to see what is being said.

What it should do for horizontal scroll is that if and only if the frame is not
*completely* visible would it scroll horizontally.

Note that you can not use NS_PRESSHELL_SCROLL_LEFT because some languages are
read from right to left and in that case scroll left would be the last thing
they wanted.

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix to ScrollFrameToView and one usage, GoToAnchor

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Test case

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Maybe I was not clear in what I wanted... In short, I would not like the screen
to scroll horizontally if even a part of the target frame was visible in the
view (and part was not). Currently even if a small part of the frame is outside
the view, we scroll.

The patch adds code to ScrollFrameIntoView to handle this. I also changed one
caller, in the GoToAnchor. Other places where this occurs is in search etc.

The test case has one line that is extremely long. You should resize your window
so that id1 is completely outside of the view, oldscroll is part visible and
part outside and noscroll is totally inside the view. Then try the different
links. The oldscroll link shows the difference in behaviour.

Comment 4

[troy]

What you're describing in the first part of the bug report is not true. What
Gecko does is scroll horizontally the minimum amount in order for the target to
be fully visible.

This is also what IE does, and is better than what the 4.x Netscape browsers do
which is typically to scroll all the way to the right edge of the window

Comment 5

[troy]

Attachment: My test case

Comment 6

[troy]

I can see your point, but it's not clear what you propose is any better. It's
different than our 4.x browser and IE which is bad, because it's not backwards
compatible

Your proposal assumes that what has been scrolled off on the left is more
important than the entire target. I don't know that that is true.

Unless there is a very compelling reason to change our current behavior I don't
see why we should. Marking WONTFIX

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I can live with wontfix for the general behaviour, but could you still check in
the code that enables my behaviour, not any callers? I need this behaviour in a
system we developed for a customer. If there was code that could do this on
request I'd only need to call that code. As it is now I am required to mess with
the baseline code.

Comment 8

[Chris Petersen]

Based on troy's comments, marking as verified won't fix.

Comment 9

[rods (gone)]

After talking to troy I am going to reopen this, I need sort of this same thing
for gfx widgets.

Comment 10

[rods (gone)]

reassigning to me

Comment 11

[leger]

Clearing WON'TFIX resolution due to reopen.

Comment 12

[rods (gone)]

setting milestone to M14

Comment 13

[rods (gone)]

The code is in and it works and is tested for form controls, I did not change 
anything to do with links.

Comment 14

[rods (gone)]

Fixed

Comment 15

[Eric Pollmann]

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

Comment 16

[Chris Petersen]

Marking verified fixed in Jan 31 build per last comments.