[Webkit-unassigned] [Bug 49382] Spatial Navigation: issues with the node selection algorithm.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49382





--- Comment #22 from Yael <yael.aharon at nokia.com>  2010-11-16 06:00:27 PST ---
(In reply to comment #20)
Thanks for reviewing at 1:30 in the morning :-) A new patch is coming as soon as I am done compiling and running some tests. 

> > WebCore/page/FocusController.cpp:817
> > +bool FocusController::navigateIn4WayDirection(FocusDirection direction, KeyboardEvent* event)
> 
> I would really like a better name here. Why not keep the previous name?
I like the old name better too :-)

> > WebCore/page/FocusController.cpp:833
> > +        container = scrollableOrFrameParentForNodeAndDirection(direction, focusedNode);
> > +        startingRect = nodeRectInAbsoluteCoordinates(focusedNode, true);
> 
> On IRC we talked about renamed FocusableCandidate to FocusableContent. It would have an additional IntRect, and Node could be 0. The 'container' above would be the FocusableContent::scrollableEnclosingNode. Does it make sense to implement, so it all gets consistent?

I did add a IntRect to FocusCandidate. The code you are pointing at is the initialization of the currently focused node, before we have a focus candidate. I kind of like the existing logic where we have a FocusCandidate, and a "contender" FocusCandidate. Do you really think we should wrap the currently focused node and its container with a FocusCandidate? That will break the current logic.

> > WebCore/page/FocusController.cpp:836
> > +    bool consumed = false;
> 
> what is consumed?
The keyboard event. If we move focus or scroll, we also consume the event. If we don't do any of those, we don't consume the keyboard event.

> > WebCore/page/SpatialNavigation.cpp:494
> > +    default:
> > +        break;
> > +    }
> 
> Add an assert_not_reached here, and return :)
> 
> > WebCore/page/SpatialNavigation.cpp:528
> > +            ASSERT_NOT_REACHED();
> 
> add a 'return' here as well.
Sometimes we call this without direction, mainly when we deal with overflow:hidden, and no scrolling is involved.

> 
> > WebCore/page/SpatialNavigation.cpp:569
> > +
> > +        return true;
> 
> Remove this empty line.
> 
> > WebCore/page/SpatialNavigation.cpp:667
> > +        if (parent->isDocumentNode()) // This is true only if node is a document node.
> 
> I would remove this comment.
> 
> > WebCore/page/SpatialNavigation.cpp:672
> > +            parent = parent->parentNode();
> > +    } while (parent && !canScrollInDirection(direction, parent) && !parent->isDocumentNode());
> > +    return parent;
> 
> Empty line between this two.
> 
> > WebCore/page/SpatialNavigation.cpp:705
> > +    frame->view()->calculateScrollbarModesForLayout(horizontalMode, verticalMode);
> 
> Maybe FrameView::calculateScrollbarModesForLayout does not consider the case when Webkit side disabled the scrollbars? See ScrollView::scrollbarModes()...
> 
During my testing I called the API to turn off scrollbars (hacked FrameLoaderClientQt to do that for every frame) and this gave the correct results.

> > WebCore/page/SpatialNavigation.cpp:848
> > +    switch (direction) {
> > +    case FocusDirectionLeft:
> > +        if (nodeRect.right() > currentRect.x())
> > +            return;
> > +        break;
> > +    case FocusDirectionUp:
> > +        if (nodeRect.bottom() > currentRect.y())
> > +            return;
> > +        break;
> > +    case FocusDirectionRight:
> > +        if (nodeRect.x() < currentRect.right())
> > +            return;
> > +        break;
> > +    case FocusDirectionDown:
> > +        if (nodeRect.y() < currentRect.bottom())
> > +            return;
> 
> are not you doing these checks in areRectsMoreThanFullScreenApart?
Sorry :-) Removed the check from areRectsMoreThanFullScreenApart() since this is done first.

> > WebCore/page/SpatialNavigation.cpp:898
> > +    IntRect candidateRect = nodeRectInAbsoluteCoordinates(candidate.node);
> 
> do not need to do this. Just use candidate.rect ?
Good catch.

One last comment, I tried using the middle point before started I using entry/exit points, and the results seem to not be as good as using entry/exit points. I'd like to not use middle point, so I modified isRectInDirection accordingly.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.