Bug 36463 – Spatial Navigation: make it work with focusable elements in overflow content

• Status: CLOSED
• Resolution: FIXED
• Component: Accessibility

[Antonio Gomes]

Created attachment 51364 [details]
testcase

The attached testcase has a overflow'ed content (a scrollable <div> for instance) with some focusable offscreen content in it.

It is currently not supported by Spatial Navigation.

[Antonio Gomes]

Created attachment 51954 [details]
WIP - make scrollable divs to work.

Not ready for review yet.

[Antonio Gomes]

Created attachment 53183 [details]
patch v1

I will request review as soon as patch in bug 37461 is landed, otherwise, commit bots will acuse build errors

[Antonio Gomes]

Created attachment 53652 [details]
patch v2

patch v2 is not yet to be reviewed, but is totally functional.

v3 will bring it simpler as well as with some further improvement.

[Antonio Gomes]

Created attachment 53683 [details]
patch v3

In order to make reviewing easier, I filed bug 37802 and bug 37803 and splitted up patch v2, resulting in a smallar patch.

I am not yet requesting review, but will as soon as the blocks mentioned earlier get FIXED.

[Antonio Gomes]

Created attachment 56073 [details]
patch v4 - patch ready for review

Patch addresses the problem that current Spatial Navigation does not properly traverse scrollable contents, including scrollable div's. For that, a new class member called scrollableEnclosingBox was introduced to FocusCandidate class to keep the track of current scrollable box Node wrapping a FocusCandidate. The common case is obviously that there is no scrollable container wrapping it.
    
To make use of enclosingScrollableBox of FocusCandidate, the DOM traversal algorithm routine  (FocusController::findNextFocusableInDirection) was changed as follows: At the time it encounters a scrollable Node (read for now a scrollable div), each focusable node that is inner of it keeps track of the scrollable node. By the time a sibling of the scrollable Node in case is encountered, there is not need to track this reference any more, and the traverse continues ...
    
updateFocusCandiditeIfCloser logic was also adapted to fit the need of the newly introduced enclosingScrollableBox class member, getting simpler and more easily maintainable.

[Simon Fraser (smfr)]

Comment on attachment 56073 [details]
patch v4 - patch ready for review

WebCore/page/FocusController.cpp:398
 +          // sameContainerAsCandidate and sameContainerAsClosest are mutually excluvive.
Typo: "excluvive"

WebCore/page/FocusController.cpp:405
 +          } else if (sameContainerAsClosest) {
Don't use 'else' after a return.

WebCore/page/FocusController.cpp:409
 +          } else { // ### !sameContainerAsCandidate && !sameContainerAsClosest
Ditto

WebCore/page/FocusController.cpp:449
 +          } else if (candidate->hasTagName(divTag) && isScrollableContainerNode(candidate)) {
It's not correct to assume that only divs can have overflow: scroll. Any block element can. It would be more appropriate to check the style for overflow.



r- for assuming that only divs can have overflow:scroll.

[Antonio Gomes]

Created attachment 56402 [details]
patch v5

Fixed what Simon pointed out: Algorithm is working not only for scrollable <div>, but for any scrollable block element that is different than #document and has child nodes.

[Antonio Gomes]

Created attachment 56483 [details]
patch v5.1

Patch is the same as patch v5, but fixed a typo in the ChangeLog.

Summary:
- Addressed Simon's requests by extending support for travesing any scrollable block elements that has at least one child node and is not a document node.
- Patch basically handle scrollable block elements in the same way {i}frame are handle. Internal traversal has precedence over elements outside it.
- Patch refactories updateFocusCandidateIfCloser to work with the newly introduced enclosingScrollableBox class member of FocusCandidate.
- As a real world testcase: patch makes pages like maps.google.com useful

[Kenneth Rohde Christiansen]

Why all these newlines in the tests?

[Antonio Gomes]

(In reply to comment #9)
> Why all these newlines in the tests?

(In reply to comment #9)
> Why all these newlines in the tests?

+It has a visible link_2.
+
+
+
+
+
+
+
+... and an overflowed link like link_3.

In this case, for example, there is a div, and "It has visible link_2" is in the clipped overflow part of it, while "and and overflowed link like link_3" is overflowed.

that said, the "newlines" seems to be the way empty content inside the div is represented by DRT.

[Chris Jerdonek]

FYI, these patches aren't applying because the ChangeLog portions of the patch have more than 3 lines of trailing context:

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 84d49ec..35111da 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,8 +1,46 @@
> [phone]  Antonio Gomes  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Spatial Navigation: make it work with focusable elements in overflow content
> <snip!>
> +        (WebCore::FocusCandidate::FocusCandidate):
> +        (WebCore::FocusCandidate::isInScrollableContainer):
> +
>  2010-05-18  Chris Jerdonek  <[e-mail]>
>  
>          Reviewed by Darin Adler.
>  
>          Refactored FrameLoader::isDocumentSandboxed() from a private member function
>          to a static, non-member, non-friend function.
>  
>          https://bugs.webkit.org/show_bug.cgi?id=39157
> diff --git a/WebCore/page/FocusController.cpp b/WebCore/page/FocusController.cpp
> index 08219a6..9bdee79 100644
> --- a/WebCore/page/FocusController.cpp
> +++ b/WebCore/page/FocusController.cpp
> @@ -337,113 +337,169 @@ bool FocusController::advanceFocusDirectionally(FocusDirection direction, Keyboa
>      Element* element = static_cast<Element*>(node);

IIRC, the ChangeLog diffs are applied with "fuzz=3" to allow the patch to apply (and be inserted at the top) in the frequent case that new ChangeLogs are added to the file between the time you generate the diff and the time the patch is applied using svn-apply.

Do you know why your diffs have more than 3 lines of context?  People normally have 3.  Are you invoking git diff with special parameters?

[Antonio Gomes]

> Do you know why your diffs have more than 3 lines of context?  People normally have 3.  Are you invoking git diff with special parameters?

as I said on the email, I am using "git format-patch -1 COMMIT_HASH -U8" to generate my patches, so 8 lines of context.

I personally think that 3 lines of context is Ok for changeLogs, but not code, specially in less simple patches.

[Kenneth Rohde Christiansen]

Comment on attachment 56483 [details]
patch v5.1

Not must a review of the actual code, but there goes my comments.

> Patch addresses the problem that Spatial Navigation currently does not properly
> traverse scrollable contents, including scrollable div's. For that to work, a new
> class member called scrollableEnclosingBox was introduced to FocusCandidate class
> to keep the track of current scrollable box Node wrapping a FocusCandidate. To make
> use of enclosingScrollableBox of FocusCandidate, the DOM traversal algorithm routine
> (FocusController::findNextFocusableInDirection) was changed as follows: At the
> time it encounters a scrollable Node, each focusable node that is inner of it keeps
> track of the container reference. By the time a sibling of the scrollable Node in case
> is encountered, there is not need to track this reference any more, and the traversal
> algorithm continues normally.

This is a bit hard reading; consider adding a few newlines where it logically makes sense.


> +    This test ensures the content overflow traversal correctness of Spatial Navigation

No need to use capitals for Spatial Navigation.

> +    algorithm: focusable elements in an scrollable containers (e.g. <div>) should be
> +    accessible, including offscreen content.

That doesn't sounds like an algorithm to me.

> +
> +    * Pre-conditions:
> +    1) DRT support for SNav enable/disable.

Just write out spatial navigation.


> -// FIXME: Make this method more modular, and simpler to understand and maintain.
> -static void updateFocusCandidateIfCloser(Node* focusedNode, const FocusCandidate& candidate, FocusCandidate& closest)
> +static void updateFocusCandidateInTheSameContainer(const FocusCandidate& candidate, FocusCandidate& closest)

I would leave out the The.


> +        // ### !sameContainerAsCandidate && !sameContainerAsClosest

### what does that means? is that a FIXME: ?


>  void FocusController::deepFindFocusableNodeInDirection(Node* container, Node* focusedNode,
>                                                         FocusDirection direction, KeyboardEvent* event,
>                                                         FocusCandidate& closestFocusCandidate)

Do not add newlines in the function definition.



> +    if (RenderObject* renderer = node->renderer())
> +        return (renderer->isBox() && toRenderBox(renderer)->canBeScrolledAndHasScrollableArea()
> +             && node->hasChildNodes() && !node->isDocumentNode());

Code style issue, you will need braces here: the return spans two real lines.

> +    bool isInScrollableContainer() const { return node && enclosingScrollableBox; }

Please check if other "isIn" methods exist, or they whether they just use "in"

[Antonio Gomes]

Created attachment 56880 [details]
patch v5.2

(In reply to comment #13)
> (From update of attachment 56483 [details])
> Not must a review of the actual code, but there goes my comments.

Thank you for looking. Comments addressed.

> > +        // ### !sameContainerAsCandidate && !sameContainerAsClosest
> 
> ### what does that means? is that a FIXME: ?

It means more like a NOTE: than a FIXME: . see other uses here http://pastebin.com/BSJrtURx

> Please check if other "isIn" methods exist, or they whether they just use "in"

only "in" is generally used. Changed accordingly.

[Antonio Gomes]

Created attachment 56882 [details]
patch v5.3

... this time uploaded the right patch

[Antonio Gomes]

Ping review?

Patch is waiting for weeks, and blocks bug 38590 and bug 39195, both already with patches, but waiting for this one to land.

These three are likely the last three spatial navigation patches I want to land before QtWebKit2.0 (and qt 4.7) release.

[Antonio Gomes]

Created attachment 58640 [details]
patch v5.4 - same as patch v5.3 but rebased against tot and applicable by ews bots

[WebKit Review Bot]

Attachment 58640 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/page/FocusController.cpp:379:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antonio Gomes]

Created attachment 58644 [details]
(committed in r61134, reviewed by smfr and kenneth) patch v5.4 - same as patch v5.4 but fixed style bug

[Simon Fraser (smfr)]

Comment on attachment 58644 [details]
(committed in r61134, reviewed by smfr and kenneth) patch v5.4 - same as patch v5.4 but fixed style bug



> +    bool sameContainer;
> +    if (candidate.document() != closest.document())
> +        sameContainer = false;
> +    else
> +        sameContainer = candidate.enclosingScrollableBox == closest.enclosingScrollableBox;

This would be clearer as
  bool sameContainer == candidate.document() == closest.document() && candidate.enclosingScrollableBox == closest.enclosingScrollableBox;

> +    // ### !sameContainerAsCandidate && !sameContainerAsClosest

Odd comment style; please don't use ####

> +            // 1) If candidateParent is not null, and it holds the distance and alignment data of the
> +            // parent container element itself.
> +            if (!candidateParent.isNull()) {
> +                currentFocusCandidate.parentAlignment = candidateParent.alignment;
> +                currentFocusCandidate.parentDistance = candidateParent.distance;
> +                currentFocusCandidate.enclosingScrollableBox = candidateParent.node;
> +
> +            // 2) Parent of outer is either:
> +            // * <frame> or <iframe>;
> +            // * or any other scrollable block element.
> +            } else if (!isInRootDocument(outer)) {
> +                if (Document* document = static_cast<Document*>(outer->parent()))
> +                    currentFocusCandidate.enclosingScrollableBox = static_cast<Node*>(document->ownerElement());
> +

Put the comments inside the relevant braces. This style is confusing.

r=me with those comments addressed.

[Antonio Gomes]

Comment on attachment 58644 [details]
(committed in r61134, reviewed by smfr and kenneth) patch v5.4 - same as patch v5.4 but fixed style bug

Clearing flags on attachment: 58644

Committed r61134: <http://trac.webkit.org/changeset/61134>

[Antonio Gomes]

> r=me with those comments addressed.

fixed before landing. thank you

[Simon Hausmann]

Antonio, the patch doesn't apply against the qtwebkit-2.0 branch. Could you help resolve the conflicts? Is there another patch missing maybe?

[Antonio Gomes]

Created attachment 58773 [details]
patch v5.4 that applies against qtwebkit-2.0 branch

(In reply to comment #23)
> Antonio, the patch doesn't apply against the qtwebkit-2.0 branch. Could you help resolve the conflicts? Is there another patch missing maybe?

patch for qtwebkit-2.0 branch.

[Simon Hausmann]

Revision r61134 cherry-picked into qtwebkit-2.0 with commit 1e7918b7e0a6e32b4f4cf63dea80e5a5f002561b