[Webkit-unassigned] [Bug 49382] Spatial Navigation: issues with the node selection algorithm.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 18 21:16:39 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=49382
Antonio Gomes <tonikitoo at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #73993|review? |review-
Flag| |
--- Comment #29 from Antonio Gomes <tonikitoo at webkit.org> 2010-11-18 21:16:37 PST ---
(From update of attachment 73993)
View in context: https://bugs.webkit.org/attachment.cgi?id=73993&action=review
> WebCore/page/FocusController.cpp:638
> + HitTestResult result = candidate.node->document()->page()->mainFrame()->eventHandler()->hitTestResultAtPoint(IntPoint(x, y), false, true);
page()->eventHandler()
> WebCore/page/FocusController.cpp:700
> + // The starting rect is the rect of the focused node, in document coordinates.
> + // Compose a virtual starting rect if there is no focused node or if it is off screen.
> + // The virtual rect is the edge of the container or frame. We select which
> + // edge depending on the direction of the navigation.
> + IntRect newStartingRect = startingRect;
> +
> + if (startingRect.isEmpty()) {
> + newStartingRect = nodeRectInAbsoluteCoordinates(container);
> + switch (direction) {
> + case FocusDirectionLeft:
> + newStartingRect.setX(newStartingRect.right());
> + newStartingRect.setWidth(0);
> + break;
> + case FocusDirectionUp:
> + newStartingRect.setY(newStartingRect.bottom());
> + newStartingRect.setHeight(0);
> + break;
> + case FocusDirectionRight:
> + newStartingRect.setWidth(0);
> + break;
> + case FocusDirectionDown:
> + newStartingRect.setHeight(0);
> + break;
> + default:
> + ASSERT_NOT_REACHED();
> + }
> + }
Make it a helper.
> WebCore/page/FocusController.cpp:709
> + if (focusCandidate.isNull()) {
> + if (canScrollInDirection(direction, container)) {
> + // Nothing to focus, scroll if possible.
> + scrollInDirection(container, direction);
Lets change the logic here as following:
if (focusCandidate.isNull()) {
// Nothing to focus, scroll if possible.
if (scrollInDirection)
...
}
>From within scrollInDirection, you check canScrollInDirection, bailing out earlier if it can not. Adjust the following 'return' calls accordingly.
> WebCore/page/FocusController.cpp:719
> + if (hasOffscreenRect(focusCandidate.node, direction)) {
> + Frame* frame = focusCandidate.node->document()->view()->frame();
> + scrollInDirection(frame->document(), direction);
> + return true;
You have two scrollInDirection methods. Here you should just use the one that takes Frame*
Frame* frame = focusCandidate.node->document()->view()->frame();
scrollInDirection(frame, direction);
return true;
or make it:
Document* document = focusCandidate.node->document();
scrollInDirection(document, direction);
return true;
> WebCore/page/FocusController.cpp:725
> + IntRect rect;
> + Node* focusedNode = focusedOrMainFrame()->document()->focusedNode();
> + if (focusedNode && !hasOffscreenRect(focusedNode))
> + rect = nodeRectInAbsoluteCoordinates(focusedNode, true /* ignore border */);
what happens here if you have no focused node, so 'rect' is empty. Also 'rect' is a too generic name in this context. Lets give it a more descriptive name.
> WebCore/page/FocusController.cpp:726
> + ASSERT(static_cast<HTMLFrameOwnerElement*>(focusCandidate.node)->contentFrame());
Move this assert to outside the outer if here. If FrameOwnerElement is not a contentFrame it just be no-op.
it should be
if (focusCandidate.node->isFrameOwnerElement() && static_cast<HTMLFrameOwnerElement*>(focusableCandidate.node)->contentFrame())
> WebCore/page/FocusController.cpp:727
> + static_cast<HTMLFrameOwnerElement*>(focusCandidate.node)->contentFrame()->document()->updateLayoutIgnorePendingStylesheets();
Why not just do
focusCandidate.node->document()?
> WebCore/page/FocusController.cpp:728
> + if (!navigateInContainer(static_cast<HTMLFrameOwnerElement*>(focusCandidate.node)->contentFrame()->document(), rect, direction, event))
Same here.
Also, let same "Document* focusedDocument = focusCandidate.node->document()" in a auxiliar variable to avoid make this cast so many times.
> WebCore/page/FocusController.cpp:729
> + // The new frame had nothing interesting, skip past it and try again.
"skip past it" sounds strange.
Also put {} around the 'if' here.
> WebCore/page/FocusController.cpp:747
> + scrollInDirection(container, direction);
you are ignoring the return value here. Should you?
> WebCore/page/FrameView.h:238
> + void calculateScrollbarModesForLayout(ScrollbarMode& hMode, ScrollbarMode& vMode);
Your changelog should explain this change. Is this method designed to be public?
> WebCore/page/SpatialNavigation.cpp:491
> + default:
> + break;
It is not clear to me why you do not assert, and bail out. Could you explain?
> WebCore/page/SpatialNavigation.cpp:507
> + ASSERT(frame && canScrollInDirection(direction, frame->document()));
you should be assert "canScrollInDirection" here. it should actually early return here.
> WebCore/page/SpatialNavigation.cpp:560
> + switch (direction) {
> + case FocusDirectionLeft:
> + dx = - min(Scrollbar::pixelsPerLineStep(), container->renderBox()->scrollLeft());
> + break;
> + case FocusDirectionRight:
> + ASSERT(container->renderBox()->scrollWidth() > (container->renderBox()->scrollLeft() + container->renderBox()->clientWidth()));
> + dx = min(Scrollbar::pixelsPerLineStep(), container->renderBox()->scrollWidth() - (container->renderBox()->scrollLeft() + container->renderBox()->clientWidth()));
> + break;
> + case FocusDirectionUp:
> + dy = - min(Scrollbar::pixelsPerLineStep(), container->renderBox()->scrollTop());
> + break;
> + case FocusDirectionDown:
> + ASSERT(container->renderBox()->scrollHeight() - (container->renderBox()->scrollTop() + container->renderBox()->clientHeight()));
> + dy = min(Scrollbar::pixelsPerLineStep(), container->renderBox()->scrollHeight() - (container->renderBox()->scrollTop() + container->renderBox()->clientHeight()));
why do we need the minimal between the scrollable offset available and Scrollbar::pixelPerLineStep ?
Just do ScrollGranularity line, and the rest will be handled for you.
> WebCore/page/SpatialNavigation.cpp:567
> + container->renderBox()->enclosingLayer()->scrollByRecursively(dx, dy);
Why not do page()->eventHandler()->scrollRecursively( ... , ..., node), where 'node' is the 'startingNode'. It does that enclosing layer does and more.
> WebCore/page/SpatialNavigation.cpp:628
> +bool isScrollableContainerNode(const Node* node)
why does it need to be const here and not in the other method we are passing a Node*?
> WebCore/page/SpatialNavigation.cpp:767
> +// Exit point is the closest point in the startingRect, depending on the direction of the navigation.
> +// Entry point is the closest point in the candidate node, depending on the direction of the navigation.
Lets rephrase this comment.
> WebCore/page/SpatialNavigation.h:133
> +Node* scrollableOrFrameParentForNodeAndDirection(FocusDirection, Node* node);
Lets name it either scrollableEnclosingBoxOrParentFrameForNodeInDirection or scrollableOrParentFrameForNodeInDirection
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list