RESOLVED FIXED 36020
Spatial Navigation: Add a scrollIntoView call when focusing an element. 
https://bugs.webkit.org/show_bug.cgi?id=36020
Summary Spatial Navigation: Add a scrollIntoView call when focusing an element.
Antonio Gomes
Reported 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 ...
Attachments
patch 0.1 (3.10 KB, patch)
2010-03-11 10:15 PST, Antonio Gomes 		tonikitoo: commit-queue-
DetailsFormatted DiffDiff
patch 0.2 - add a custom scrollIntoView method (5.04 KB, patch)
2010-03-12 11:25 PST, Antonio Gomes 		tonikitoo: commit-queue-
DetailsFormatted DiffDiff
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-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Antonio Gomes
Comment 1 2010-03-11 10:15:24 PST
Created attachment 50515 [details] patch 0.1 Calls element->scrollIntoView() before element->focus();
Antonio Gomes
Comment 2 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.
Antonio Gomes
Comment 3 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?
Simon Fraser (smfr)
Comment 4 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
Antonio Gomes
Comment 5 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)
Antonio Gomes
Comment 6 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.
Note You need to log in before you can comment on or make changes to this bug.