Bug 36020 - Spatial Navigation: Add a scrollIntoView call when focusing an element.
Summary: Spatial Navigation: Add a scrollIntoView call when focusing an element.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on: 18662
Blocks: 35984
  Show dependency treegraph
 
Reported: 2010-03-11 10:11 PST by Antonio Gomes
Modified: 2010-03-16 06:48 PDT (History)
2 users (show)

See Also:


Attachments
patch 0.1 (3.10 KB, patch)
2010-03-11 10:15 PST, Antonio Gomes
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
patch 0.2 - add a custom scrollIntoView method (5.04 KB, patch)
2010-03-12 11:25 PST, Antonio Gomes
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
patch 0.3 - same as 0.2 + adds a better ChangeLog entry (6.14 KB, patch)
2010-03-15 08:35 PDT, Antonio Gomes
simon.fraser: review+
tonikitoo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-03-11 10:11:18 PST
In the current Spatial Navigation logic, for an element to get the focus it just needs to be partially visible (in current viewport), not completely.

Even calling |element->focus()| does not necessarily bring element's rect to be completely visible in cases where it is already partially visible.

I think it would be nice to call 'element->scrollIntoView' for that.

patch coming ...
Comment 1 Antonio Gomes 2010-03-11 10:15:24 PST
Created attachment 50515 [details]
patch 0.1

Calls element->scrollIntoView() before element->focus();
Comment 2 Antonio Gomes 2010-03-12 11:25:36 PST
Created attachment 50614 [details]
patch 0.2 - add a custom scrollIntoView method

Added a helper scrollIntoView in SpatialNavigation.h. There a inflate operation is performed before scrolling.
Comment 3 Antonio Gomes 2010-03-15 08:35:30 PDT
Created attachment 50711 [details]
patch 0.3 - same as 0.2 + adds a better ChangeLog entry

Simon, could you help me w/ that one?
Comment 4 Simon Fraser (smfr) 2010-03-15 12:08:51 PDT
Comment on attachment 50711 [details]
patch 0.3 - same as 0.2 + adds a better ChangeLog entry


> +void scrollIntoView(Element* element)
> +{
> +    // NOTE: Element's scrollIntoView method could had been used here, but
> +    // it is preferable to inflate |element|'s bounding rect a bit before
> +    // scrolling it for accurate reason.
> +    // Element's scrollIntoView method does not provide this flexibility.
> +    static const int fudgeFactor = 2;

Shouldn't this be related to the platform's focus outline width? That information doesn't seem to be available, though.

r=me
Comment 5 Antonio Gomes 2010-03-16 06:47:35 PDT
> > +void scrollIntoView(Element* element)
> > +{
> > +    // NOTE: Element's scrollIntoView method could had been used here, but
> > +    // it is preferable to inflate |element|'s bounding rect a bit before
> > +    // scrolling it for accurate reason.
> > +    // Element's scrollIntoView method does not provide this flexibility.
> > +    static const int fudgeFactor = 2;
> 
> Shouldn't this be related to the platform's focus outline width? That
> information doesn't seem to be available, though.

I can check that, for sure, Simon.

> r=me

thx again


Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/platform/gtk/Skipped
	M	WebCore/ChangeLog
	M	WebCore/page/FocusController.cpp
	M	WebCore/page/SpatialNavigation.cpp
	M	WebCore/page/SpatialNavigation.h
Committed r56057
	M	WebCore/ChangeLog
	M	WebCore/page/SpatialNavigation.h
	M	WebCore/page/FocusController.cpp
	M	WebCore/page/SpatialNavigation.cpp
	M	LayoutTests/platform/gtk/Skipped
	M	LayoutTests/ChangeLog

r56057 = 4d0624cd978ee7b14b78500555f896a081b40466 (trunk)
Comment 6 Antonio Gomes 2010-03-16 06:48:17 PDT
I had a talk to Kenneth about the desired behaviour of scrolling actions when running Spatial Navigation on a mobo device. It turns out that most of the current browser clients that use it (via a fake mouse point or not) keep the focusable element (and the fake mouse point) on the center of the screen when possible. It will be addressed on its own bug. Current behaviour, matches Opera's destop browser.