[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 #30 from Yael <yael.aharon at nokia.com>  2010-11-19 10:20:47 PST ---
(In reply to comment #29)
> (From update of attachment 73993 [details])
Thank you for the review, Antonio. Your comments are very good feedback to me, but I would like to address a few comments here, and explain why things were done the way I did them.

I added ASSERTs in the code to make sure we never get "stuck". Some of the comments below will result in removing those ASSERTs, but I would really like to avoid that. If we get "stuck" in the algorithm, the user will press an arrow key and will get no feedback, which is a very frustrating situation. Please consider if we should really remove those ASSERTs.

I added code to control the scrolling very tightly. A suggestion below is to rely on existing API for scrolling, and not use my own code. The reason I did not do that is that there are cases in which the existing API was scroll the parent when that was not needed. The result was not pretty :-(

I tracked the unwanted scrolling to RenderLayer::scrollRectToVisible line 1487:
parentLayer->scrollRectToVisible(newRect, scrollToAnchor, alignX, alignY);
Sometimes it scrolls the parent unnecessarily. We could look at why it does that separately.

I will update the patch soon, and ask you to review again :-)

>> WebCore/page/FocusController.cpp:638
>> +        HitTestResult result = >candidate.node->document()->page()->mainFrame()->eventHandler()->hitTestResult>AtPoint(IntPoint(x, y), false, true);
>
>page()->eventHandler()
page does not have a eventHandler ?

> > WebCore/page/FocusController.cpp:709
> From within scrollInDirection, you check canScrollInDirection, bailing out earlier if it can not. Adjust the following 'return' calls accordingly.

Currently, I do not check if can scroll from inside scrollInDirection. There is an ASSERT there, to make sure scrollInDirection is called _ONLY_ if we can scroll. There is only one call site that calls canScrollInDirection before scrollInDirection. That happens only if we found nothing. If I make the change, I will loose the ASSERT, and that will allow bugs to creep, where if someone forgets to check return value, we think we scroll, but in effect we get "stuck". That will leave the user not knowing why the browser ignored his click.

> > WebCore/page/FocusController.cpp:725
> 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.
Changed the name to "startingRect". If the startingRect is empty, we construct a virtual starting rect. 


> > WebCore/page/FocusController.cpp:727
> > +        static_cast<HTMLFrameOwnerElement*>(focusCandidate.node)->contentFrame()->document()->updateLayoutIgnorePendingStylesheets();
> 
> Why not just do 
> 
> focusCandidate.node->document()?

focusCandidate.node->document() returns the parent document of the iframe element, here I need the document inside the iframe itself.

> > WebCore/page/FocusController.cpp:729
> Also put {} around the 'if' here.
"Style police" does not allow that :-)

> > WebCore/page/FocusController.cpp:747
> > +        scrollInDirection(container, direction);
> 
> you are ignoring the return value here. Should you?
Yes. 
A scrollable container is considered in the algorithm only if it can scroll in the specific direction. Otherwise, it is treated like any non-scrollable node. I added the ASSERT in scrollInDirection to make sure that we keep the logic this way. So if the ASSERT was not hit, I can be sure that we scrolled.

> 
> > WebCore/page/FrameView.h:238
> > +    void calculateScrollbarModesForLayout(ScrollbarMode& hMode, ScrollbarMode& vMode);
> 
> Your changelog should explain this change. Is this method designed to be public?
I will explain this in the changelog.
This method knows if a frame can scroll, without taking the scrollMode API into account (we discussed this API on IRC). We really need a public method to tell us if we can scroll the frame or not, and this function is perfect for that purpose.

> > WebCore/page/SpatialNavigation.cpp:491
> > +    default:
> > +        break;
> 
> It is not clear to me why you do not assert, and bail out. Could you explain?
> 
We use this method for 2 different situations. (1) If the container of the node is scrollable, we pass the direction, and we want to know if the node will be offscreen after we scroll (if needed). (2) If the container has overflow:hidden,  we want to know if the node is offscreen without scrolling, because scrolling is not allowed.
I updated the comment to explain that.

> > WebCore/page/SpatialNavigation.cpp:507
> > +    ASSERT(frame && canScrollInDirection(direction, frame->document()));
> 
> you should be assert "canScrollInDirection" here. it should actually early return here.
See explanation above :-)

> > WebCore/page/SpatialNavigation.cpp:560
> 
> 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.
My testing showed unwanted scrolling of the parent container if I don't do that. 

> > 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.
Again, my testing show unwanted scrolling of the parent if I do that. We do not want the "more" that it gives :-) This is especially true in nested iframes.

> > 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*?
I tried to add const everywhere that the node is not modified. e.g. canScrollInDIrection does not modify the node, so it has a const Node*.
scrollInDirection does modify the node, so it does not have const. since canScrollInDirection calls isScrollableContainerNode, they need to be consistent or they won't compile.

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