key navigation and visible area of the page

[Tobias Anton]

On Montag, 17. Mai 2004 17:19, Lubos Lunak wrote:
>
>  I already described the problem. The Tab behaviour is weird if the focused
> item is not visible. Go e.g. to
> http://webcvs.kde.org/cgi-bin/cvsweb.cgi/kdelibs/khtml/khtmlview.cpp , hit
> Tab few times, scroll ten pages down, and start hitting Tab again. I doubt
> you can guess what it will do, but even if you do, I don't consider that to
> be known behaviour, in fact I was pretty confused when that happened to me.

I admit that mixing keyboard and mouse navigation does not work well.
Regarding your proposal, is not easy to determine the element corresponding to 
certain viewport coordinates. There might be multiple results, especially 
when using layers or absolute positioning. Nontheless, there exists a 
solution for this problem.

I just noticed that the scrolling code is badly broken anyway since a commit 
from PMK that renamed 
	bool gotoLink(bool)
to 
	void focusNextPrevNode(bool)
.
I'm about to restore the old behaviour to make some test cases work again that 
are currently broken. Unfortunately, restoring the old version of the code 
would conflict with your caret mode code, which manipulates d->scrollBarMoved 
in a manner I don't quite understand.

Could you explain to me when d->scrollBarMoved is set and when it is cleared?

My old tabbing code (-r1.429 khtmlview.cpp) assumes d->scrollBarMoved to be 
set whenever the last scrollbar move happened after the last call to 
focusNextPrevNode().

>  But perhaps this problem would go away after changing Tab to start from
> the visible area, like Arend suggested. What's your opinion on this?

I consider it a good idea. This behaviour is already implemented, but it is 
only active if there's no currently focused node. If you have a look at 
khtmlview.cpp:1682, you'll notice that this is supported already, but is only 
active if no node is currently focused. I could fix it by restoring the old 
behaviour, but I need some information on how you are using 
d->scrollBarMoved.

>  I found using scrollTo() to be better than ensureVisible() - it takes
> QRect instead of just a point, and seems to be a bit better at positioning.
> So I'd like to keep the onepage argument. Scrolling by one page for
> accesskeys definitely doesn't make sense.

I agree with your opinion on the accesskey behaviour.
But scrollTo() is just a replacement for ensureVisible(), differing only in 
its one-page-per-click scrolling behaviour. If you don't want that, don't use 
that function.

I claim that there is no advantage of
	scrollTo(QR)
over
	ensureVisible(QR.center().x(), QR.center().y());
. The latter does exactly the same as the former if you use it The Right Way.

The tiny detail I don't like about your change is that you added a bool 
argument to a function in order to make it behave like another, already 
existing function. That looks like code bloat to me. For that, I'd propose 
that you use ensureVisible, we remove the "bool" argument of the scrollTo() 
function again and I add support for selecting the first visible link instead 
of the next one in document order if tab is pressed after the view has been 
scrolled by hand.

Cheers
-- 
Tobias