Attachment #51362 for bug #36168

View Details Raw Unified Return to Bug 36168

| Differences between



2010-03-22  Antonio Gomes  <tonikitoo@webkit.org>

        Reviewed by NOBODY (OOPS!).
        Patch by Antonio Gomes <tonikitoo@webkit.org>

        Spatial Navigation: Initial code simplification in FocusController.cpp and SpatialNavigation.cpp

        WebCore::distanceInDirection method was handling much of the logic not
        strictly only related to the distance between nodes acquisition. This
        method was simplified and renamed to 'WebCore::distanceDataForNode'.
        The latter is now responsible for only getting the distance and alignment
        data, while all assignement logic previously distanceInDirection method
        was moved place to updateFocusCandidateIfCloser.

        Parent document distance and alignment acquisitions, on their turns, have
        also changed location: they are both got from deepFindFocusableNodeInDirection,
        and passed in a recursive call to findFocusableNodeInDirection via the candidateParent
        variable. Yet on deepFindFocusableNodeInDirection method, the need for the
        'focusCandidateCopy.distance' variable was removed, making the code much cleaner.

        No behaviour change at this point. Mostly moving code around to the place
        where it should live in.

        * page/FocusController.cpp:
        (WebCore::FocusController::advanceFocusDirectionally):
        (WebCore::updateFocusCandidateIfCloser):
        (WebCore::FocusController::findFocusableNodeInDirection):
        (WebCore::FocusController::deepFindFocusableNodeInDirection):
        * page/FocusController.h:
        * page/SpatialNavigation.cpp:
        (WebCore::distanceDataForNode):
        (WebCore::renderRectRelativeToRootDocument):
        * page/SpatialNavigation.h:

2010-03-22  Yury Semikhatsky  <yurys@chromium.org>

        Reviewed by Pavel Feldman.



    frame = frame->tree()->top();

    FocusCandidate focusCandidate;
    findFocusableNodeInDirection(frame->document(), focusedNode, direction, event, focusCandidate, FocusCandidate());

    Node* node = focusCandidate.node;
    if (!node || !node->isElementNode()) {

    return true;
}

// FIXME: Make this method more modular, and simpler to understand and maintain.
static void updateFocusCandidateIfCloser(Node* focusedNode, FocusCandidate candidate, FocusCandidate& closest)
{
    bool sameDocument = candidate.document() == closest.document();
    if (sameDocument) {
        if (closest.alignment > candidate.alignment
            || (closest.parentAlignment && candidate.alignment > closest.parentAlignment))
            return;
    } else if (closest.alignment > candidate.alignment
               && (closest.parentAlignment && candidate.alignment > closest.parentAlignment))
        return;

    if (candidate.alignment != None
        || (!closest.isNull() && closest.parentAlignment >= candidate.alignment
        && closest.document() == candidate.document())) {

        // If we are now in an higher precedent case, lets reset the current |closest|'s
        // |distance| so we force it to be bigger than any result we will get from
        // |spatialDistance()| (see below).
        if (closest.alignment < candidate.alignment
            && closest.parentAlignment < candidate.alignment)
            closest.distance = maxDistance();

        closest.alignment = candidate.alignment;
    }

    // Bail out if candidate's distance is bigger than the closest candidate's distance.
    if (candidate.distance >= closest.distance)
        return;

    // If |focusedNode| and |candidate| are in the same document and current
    // |closest| is not in an {i}frame that is preferable to get focused ...
    if (focusedNode->document() == candidate.document()
        && candidate.distance < closest.parentDistance)
        closest = candidate;
    else if (focusedNode->document() != candidate.document()) {
        // If the |focusedNode| is in an inner document and |candidate| is in a
        // different document, we only consider to change focus if there is not
        // another already good focusable candidate in the same document as |focusedNode|.
        if (!((isInRootDocument(candidate.node) && !isInRootDocument(focusedNode))
            && focusedNode->document() == closest.document()))
            closest = candidate;
    }
}

void FocusController::findFocusableNodeInDirection(Document* document, Node* focusedNode, FocusDirection direction, KeyboardEvent* event, FocusCandidate& closestFocusCandidate, FocusCandidate candidateParent)
{
    ASSERT(document);

    // Walk all the child nodes and update |closestFocusCandidate| if we find a nearer node.
    for (Node* candidate = document->firstChild(); candidate; candidate = candidate->traverseNextNode()) {
        // Inner documents case.

        if (candidate->isFrameOwnerElement())
            deepFindFocusableNodeInDirection(focusedNode, candidate, direction, event, closestFocusCandidate);
        else if (candidate != focusedNode && candidate->isKeyboardFocusable(event)) {
            FocusCandidate currentFocusCandidate(candidate);

            // Get distance and alignment from current candidate.
            distanceDataForNode(direction, focusedNode, currentFocusCandidate);

            // If |candidateParent| is not Null, it means that we are in a recursive call
            // from deepFineFocusableNodeInDirection (i.e. processing an element in an iframe),
            // and holds the distance and alignment data of the iframe element itself.
            if (!candidateParent.isNull()) {
                currentFocusCandidate.parentAlignment = candidateParent.alignment;
                currentFocusCandidate.parentDistance = candidateParent.distance;
            }

            updateFocusCandidateIfCloser(focusedNode, currentFocusCandidate, closestFocusCandidate);
        }
    }
}

        return;

    if (innerDocument == focusedNode->document())
        findFocusableNodeInDirection(innerDocument, focusedNode, direction, event, closestFocusCandidate, FocusCandidate());
    else {
        // Check if the current {i}frame element itself is a good candidate
        // to move focus to. If it is, then we traverse its inner nodes.
        FocusCandidate candidateParent = FocusCandidate(candidate);
        distanceDataForNode(direction, focusedNode, candidateParent);

        // FIXME: What about aligment?
        if (candidateParent.distance < closestFocusCandidate.distance)
            findFocusableNodeInDirection(innerDocument, focusedNode, direction, event, closestFocusCandidate, candidateParent);
            focusCandidateCopy.parentDistance = distance;

            findFocusableNodeInDirection(innerDocument, focusedNode, direction, event, focusCandidateCopy);

            // If we really have an inner closer focus candidate node, take it.
            if (closestFocusCandidate.node != focusCandidateCopy.node)
                closestFocusCandidate = focusCandidateCopy;
        }
    }
}


Lines 63-69 private: a/WebCore/page/FocusController.h_sec1
- a/WebCore/page/FocusController.h -1 / +1 lines
63	bool advanceFocusDirectionally(FocusDirection, KeyboardEvent*);	63	bool advanceFocusDirectionally(FocusDirection, KeyboardEvent*);
64	bool advanceFocusInDocumentOrder(FocusDirection, KeyboardEvent*, bool initialFocus);	64	bool advanceFocusInDocumentOrder(FocusDirection, KeyboardEvent*, bool initialFocus);
65		65
66	void findFocusableNodeInDirection(Document, Node, FocusDirection, KeyboardEvent*, FocusCandidate&);	66	void findFocusableNodeInDirection(Document, Node, FocusDirection, KeyboardEvent*, FocusCandidate&, FocusCandidate);
67	void deepFindFocusableNodeInDirection(Node, Node, FocusDirection, KeyboardEvent*, FocusCandidate&);	67	void deepFindFocusableNodeInDirection(Node, Node, FocusDirection, KeyboardEvent*, FocusCandidate&);
68		68
69	Page* m_page;	69	Page* m_page;

Lines 47-61 static bool areRectsPartiallyAligned(FocusDirection, const IntRect&, const IntRe a/WebCore/page/SpatialNavigation.cpp_sec1
- a/WebCore/page/SpatialNavigation.cpp -34 / +20 lines
47	static bool isRectInDirection(FocusDirection, const IntRect&, const IntRect&);	47	static bool isRectInDirection(FocusDirection, const IntRect&, const IntRect&);
48	static void deflateIfOverlapped(IntRect&, IntRect&);	48	static void deflateIfOverlapped(IntRect&, IntRect&);
49		49
50	long long distanceInDirection(Node* start, Node* dest, FocusDirection direction, FocusCandidate& candidate)	50	void distanceDataForNode(FocusDirection direction, Node* start, FocusCandidate& candidate)
51	{	51	{
52	RenderObject* startRender = start->renderer();	52	RenderObject* startRender = start->renderer();
53	if (!startRender)	53	if (!startRender) {
54	return maxDistance();	54	candidate.distance = maxDistance();
		55	return;
		56	}
55		57
56	RenderObject* destRender = dest->renderer();	58	RenderObject* destRender = candidate.node->renderer();
57	if (!destRender)	59	if (!destRender) {
58	return maxDistance();	60	candidate.distance = maxDistance();
		61	return;
		62	}
59		63
60	IntRect curRect = renderRectRelativeToRootDocument(startRender);	64	IntRect curRect = renderRectRelativeToRootDocument(startRender);
61	IntRect targetRect = renderRectRelativeToRootDocument(destRender);	65	IntRect targetRect = renderRectRelativeToRootDocument(destRender);
Lines 65-103 long long distanceInDirection(Node* start, Node* dest, FocusDirection direction, a/WebCore/page/SpatialNavigation.cpp_sec2
65	deflateIfOverlapped(curRect, targetRect);	69	deflateIfOverlapped(curRect, targetRect);
66		70
67	if (curRect.isEmpty() \|\| targetRect.isEmpty()	71	if (curRect.isEmpty() \|\| targetRect.isEmpty()
68	\|\| targetRect.x() < 0 \|\| targetRect.y() < 0)	72	\|\| targetRect.x() < 0 \|\| targetRect.y() < 0) {
69	return maxDistance();	73	candidate.distance = maxDistance();
		74	return;
		75	}
70		76
71	if (!isRectInDirection(direction, curRect, targetRect))	77	if (!isRectInDirection(direction, curRect, targetRect)) {
72	return maxDistance();	78	candidate.distance = maxDistance();
		79	return;
		80	}
73		81
74	// The distance between two nodes is not to be considered alone when evaluating/looking	82	// The distance between two nodes is not to be considered alone when evaluating/looking
75	// for the best focus candidate node. Alignment of rects can be also a good point to be	83	// for the best focus candidate node. Alignment of rects can be also a good point to be
76	// considered in order to make the algorithm to behavior in a more intuitive way.	84	// considered in order to make the algorithm to behavior in a more intuitive way.
77	RectsAlignment alignment = alignmentForRects(direction, curRect, targetRect);	85	candidate.alignment = alignmentForRects(direction, curRect, targetRect);
78		86	candidate.distance = spatialDistance(direction, curRect, targetRect);
79	bool sameDocument = dest->document() == candidate.document();
80	if (sameDocument) {
81	if (candidate.alignment > alignment \|\| (candidate.parentAlignment && alignment > candidate.parentAlignment))
82	return maxDistance();
83	} else if (candidate.alignment > alignment && (candidate.parentAlignment && alignment > candidate.parentAlignment))
84	return maxDistance();
85
86	// FIXME_tonikitoo: simplify the logic here !
87	if (alignment != None
88	\|\| (!candidate.isNull() && candidate.parentAlignment >= alignment
89	&& candidate.document() == dest->document())) {
90
91	// If we are now in an higher precedent case, lets reset the current \|candidate\|'s
92	// \|distance\| so we force it to be bigger than the result we will get from
93	// \|spatialDistance\| (see below).
94	if (candidate.alignment < alignment && candidate.parentAlignment < alignment)
95	candidate.distance = maxDistance();
96
97	candidate.alignment = alignment;
98	}
99
100	return spatialDistance(direction, curRect, targetRect);
101	}	87	}
102		88
103	// FIXME: This function does not behave correctly with transformed frames.	89	// FIXME: This function does not behave correctly with transformed frames.



    RectsAlignment parentAlignment;
};

void distanceDataForNode(FocusDirection direction, Node* start, FocusCandidate& candidate);
bool scrollInDirection(Frame*, FocusDirection);
void scrollIntoView(Element*);
bool hasOffscreenRect(Node*);
- 

Return to Bug 36168

Lines 305-311 bool FocusController::advanceFocusDirectionally(FocusDirection direction, Keyboa a/WebCore/page/FocusController.cpp_sec1
- a/WebCore/page/FocusController.cpp -42 / +64 lines
305	frame = frame->tree()->top();	305	frame = frame->tree()->top();
306		306
307	FocusCandidate focusCandidate;	307	FocusCandidate focusCandidate;
308	findFocusableNodeInDirection(frame->document(), focusedNode, direction, event, focusCandidate);	308	findFocusableNodeInDirection(frame->document(), focusedNode, direction, event, focusCandidate, FocusCandidate());
309		309
310	Node* node = focusCandidate.node;	310	Node* node = focusCandidate.node;
311	if (!node \|\| !node->isElementNode()) {	311	if (!node \|\| !node->isElementNode()) {
Lines 342-387 bool FocusController::advanceFocusDirectionally(FocusDirection direction, Keyboa a/WebCore/page/FocusController.cpp_sec2
342	return true;	342	return true;
343	}	343	}
344		344
345	static void updateFocusCandidateIfCloser(Node* focusedNode, Node* candidate, long long distance, FocusCandidate& closestFocusCandidate)	345	// FIXME: Make this method more modular, and simpler to understand and maintain.
		346	static void updateFocusCandidateIfCloser(Node* focusedNode, FocusCandidate candidate, FocusCandidate& closest)
346	{	347	{
347	// Bail out if \|distance\| is bigger than the current closest candidate.	348	bool sameDocument = candidate.document() == closest.document();
348	if (distance >= closestFocusCandidate.distance)	349	if (sameDocument) {
		350	if (closest.alignment > candidate.alignment
		351	\|\| (closest.parentAlignment && candidate.alignment > closest.parentAlignment))
		352	return;
		353	} else if (closest.alignment > candidate.alignment
		354	&& (closest.parentAlignment && candidate.alignment > closest.parentAlignment))
349	return;	355	return;
350		356
351	// If \|focusedNode\| and \|candidate\| are in the same document AND	357	if (candidate.alignment != None
352	// current \|closestFocusCandidadte\| is not in an {i}frame that is	358	\|\| (!closest.isNull() && closest.parentAlignment >= candidate.alignment
353	// preferable to get focused.	359	&& closest.document() == candidate.document())) {
354	if (focusedNode->document() == candidate->document()	360
355	&& distance < closestFocusCandidate.parentDistance) {	361	// If we are now in an higher precedent case, lets reset the current \|closest\|'s
356	closestFocusCandidate.node = candidate;	362	// \|distance\| so we force it to be bigger than any result we will get from
357	closestFocusCandidate.distance = distance;	363	// \|spatialDistance()\| (see below).
358	closestFocusCandidate.parentDistance = maxDistance();	364	if (closest.alignment < candidate.alignment
359	} else if (focusedNode->document() != candidate->document()) {	365	&& closest.parentAlignment < candidate.alignment)
360	// If the \|focusedNode\| is in an inner document and the \|candidate\| is	366	closest.distance = maxDistance();
361	// in a different document, we only consider to change focus if there is	367
362	// not another already good focusable candidate in the same document as	368	closest.alignment = candidate.alignment;
363	// \|focusedNode\|.	369	}
364	if (!((isInRootDocument(candidate) && !isInRootDocument(focusedNode))	370
365	&& focusedNode->document() == closestFocusCandidate.document())) {	371	// Bail out if candidate's distance is bigger than the closest candidate's distance.
366	closestFocusCandidate.node = candidate;	372	if (candidate.distance >= closest.distance)
367	closestFocusCandidate.distance = distance;	373	return;
368	}	374
		375	// If \|focusedNode\| and \|candidate\| are in the same document and current
		376	// \|closest\| is not in an {i}frame that is preferable to get focused ...
		377	if (focusedNode->document() == candidate.document()
		378	&& candidate.distance < closest.parentDistance)
		379	closest = candidate;
		380	else if (focusedNode->document() != candidate.document()) {
		381	// If the \|focusedNode\| is in an inner document and \|candidate\| is in a
		382	// different document, we only consider to change focus if there is not
		383	// another already good focusable candidate in the same document as \|focusedNode\|.
		384	if (!((isInRootDocument(candidate.node) && !isInRootDocument(focusedNode))
		385	&& focusedNode->document() == closest.document()))
		386	closest = candidate;
369	}	387	}
370	}	388	}
371		389
372	void FocusController::findFocusableNodeInDirection(Document* document, Node* focusedNode, FocusDirection direction, KeyboardEvent* event, FocusCandidate& closestFocusCandidate)	390	void FocusController::findFocusableNodeInDirection(Document* document, Node* focusedNode, FocusDirection direction, KeyboardEvent* event, FocusCandidate& closestFocusCandidate, FocusCandidate candidateParent)
373	{	391	{
374	ASSERT(document);	392	ASSERT(document);
375		393
376	// Walk all the child nodes and update focusCandidate if we find a nearer node.	394	// Walk all the child nodes and update \|closestFocusCandidate\| if we find a nearer node.
377	for (Node* candidate = document->firstChild(); candidate; candidate = candidate->traverseNextNode()) {	395	for (Node* candidate = document->firstChild(); candidate; candidate = candidate->traverseNextNode()) {
378	// Inner documents case.	396	// Inner documents case.
		397
379	if (candidate->isFrameOwnerElement())	398	if (candidate->isFrameOwnerElement())
380	deepFindFocusableNodeInDirection(focusedNode, candidate, direction, event, closestFocusCandidate);	399	deepFindFocusableNodeInDirection(focusedNode, candidate, direction, event, closestFocusCandidate);
381	else if (candidate != focusedNode && candidate->isKeyboardFocusable(event)) {	400	else if (candidate != focusedNode && candidate->isKeyboardFocusable(event)) {
382	long long distance = distanceInDirection(focusedNode, candidate,	401	FocusCandidate currentFocusCandidate(candidate);
383	direction, closestFocusCandidate);	402
384	updateFocusCandidateIfCloser(focusedNode, candidate, distance, closestFocusCandidate);	403	// Get distance and alignment from current candidate.
		404	distanceDataForNode(direction, focusedNode, currentFocusCandidate);
		405
		406	// If \|candidateParent\| is not Null, it means that we are in a recursive call
		407	// from deepFineFocusableNodeInDirection (i.e. processing an element in an iframe),
		408	// and holds the distance and alignment data of the iframe element itself.
		409	if (!candidateParent.isNull()) {
		410	currentFocusCandidate.parentAlignment = candidateParent.alignment;
		411	currentFocusCandidate.parentDistance = candidateParent.distance;
		412	}
		413
		414	updateFocusCandidateIfCloser(focusedNode, currentFocusCandidate, closestFocusCandidate);
385	}	415	}
386	}	416	}
387	}	417	}
Lines 397-420 void FocusController::deepFindFocusableNodeInDirection(Node* focusedNode, Node* a/WebCore/page/FocusController.cpp_sec3
397	return;	427	return;
398		428
399	if (innerDocument == focusedNode->document())	429	if (innerDocument == focusedNode->document())
400	findFocusableNodeInDirection(innerDocument, focusedNode, direction, event, closestFocusCandidate);	430	findFocusableNodeInDirection(innerDocument, focusedNode, direction, event, closestFocusCandidate, FocusCandidate());
401	else {	431	else {
402	// Check if the current {i}frame element itself is a good candidate	432	// Check if the current {i}frame element itself is a good candidate
403	// to move focus to. If it is, then we traverse its inner nodes.	433	// to move focus to. If it is, then we traverse its inner nodes.
404	// Lets pass a copy of the best candidate, to not get fooled by a	434	FocusCandidate candidateParent = FocusCandidate(candidate);
405	// frame without focusable elements.	435	distanceDataForNode(direction, focusedNode, candidateParent);
406	FocusCandidate focusCandidateCopy = closestFocusCandidate;	436
407	long long distance = distanceInDirection(focusedNode, candidate, direction, focusCandidateCopy);	437	// FIXME: What about aligment?
408	if (distance < focusCandidateCopy.distance) {	438	if (candidateParent.distance < closestFocusCandidate.distance)
409	focusCandidateCopy.parentAlignment = focusCandidateCopy.alignment;	439	findFocusableNodeInDirection(innerDocument, focusedNode, direction, event, closestFocusCandidate, candidateParent);
410	focusCandidateCopy.parentDistance = distance;
411
412	findFocusableNodeInDirection(innerDocument, focusedNode, direction, event, focusCandidateCopy);
413
414	// If we really have an inner closer focus candidate node, take it.
415	if (closestFocusCandidate.node != focusCandidateCopy.node)
416	closestFocusCandidate = focusCandidateCopy;
417	}
418	}	440	}
419	}	441	}
420		442

Lines 118-124 struct FocusCandidate { a/WebCore/page/SpatialNavigation.h_sec1
- a/WebCore/page/SpatialNavigation.h -2 / +1 lines
118	RectsAlignment parentAlignment;	118	RectsAlignment parentAlignment;
119	};	119	};
120		120
121	long long distanceInDirection(Node, Node, FocusDirection, FocusCandidate&);	121	void distanceDataForNode(FocusDirection direction, Node* start, FocusCandidate& candidate);
122	bool scrollInDirection(Frame*, FocusDirection);	122	bool scrollInDirection(Frame*, FocusDirection);
123	void scrollIntoView(Element*);	123	void scrollIntoView(Element*);
124	bool hasOffscreenRect(Node*);	124	bool hasOffscreenRect(Node*);
125	-

Lines 1-3 a/WebCore/ChangeLog_sec1
- a/WebCore/ChangeLog +34 lines
		1	2010-03-22 Antonio Gomes <tonikitoo@webkit.org>
		2
		3	Reviewed by NOBODY (OOPS!).
		4	Patch by Antonio Gomes <tonikitoo@webkit.org>
		5
		6	Spatial Navigation: Initial code simplification in FocusController.cpp and SpatialNavigation.cpp
		7
		8	WebCore::distanceInDirection method was handling much of the logic not
		9	strictly only related to the distance between nodes acquisition. This
		10	method was simplified and renamed to 'WebCore::distanceDataForNode'.
		11	The latter is now responsible for only getting the distance and alignment
		12	data, while all assignement logic previously distanceInDirection method
		13	was moved place to updateFocusCandidateIfCloser.
		14
		15	Parent document distance and alignment acquisitions, on their turns, have
		16	also changed location: they are both got from deepFindFocusableNodeInDirection,
		17	and passed in a recursive call to findFocusableNodeInDirection via the candidateParent
		18	variable. Yet on deepFindFocusableNodeInDirection method, the need for the
		19	'focusCandidateCopy.distance' variable was removed, making the code much cleaner.
		20
		21	No behaviour change at this point. Mostly moving code around to the place
		22	where it should live in.
		23
		24	* page/FocusController.cpp:
		25	(WebCore::FocusController::advanceFocusDirectionally):
		26	(WebCore::updateFocusCandidateIfCloser):
		27	(WebCore::FocusController::findFocusableNodeInDirection):
		28	(WebCore::FocusController::deepFindFocusableNodeInDirection):
		29	* page/FocusController.h:
		30	* page/SpatialNavigation.cpp:
		31	(WebCore::distanceDataForNode):
		32	(WebCore::renderRectRelativeToRootDocument):
		33	* page/SpatialNavigation.h:
		34
1	2010-03-22 Yury Semikhatsky <yurys@chromium.org>	35	2010-03-22 Yury Semikhatsky <yurys@chromium.org>
2		36
3	Reviewed by Pavel Feldman.	37	Reviewed by Pavel Feldman.