Code review comment for lp:~uriboni/webbrowser-app/keyboard-navigation

Revision history for this message
Ugo Riboni (uriboni) wrote :

All done except where commented below:

> - I’m not sure I understand why the test on 'preventSimplifyText' is not
> inside updateUrlFromFocus(). When the address bar has active focus, we always
> want the text to be expanded, right? So the check would be better placed in
> the 'else if' branch of the function, no?

Address bar and suggestion list should be logically considered one single unit in terms of what should happen when they have active focus. However they are two different elements in two different locations in the object tree.
The canSimplifyText exists for this reason: when it is true it means that neither the address bar nor the suggestions list have active focus.
I moved the check inside the updateUrlFromFocus anyway, as it makes the code simpler, but it should not be in the else branch as you suggest.

> As pointed out by Bill in his e-mail review (adding it here for future
> reference), the following need implementing (per design spec):
> - F11 to toggle fullscreen mode

This will be addressed later as we first need to create a proper design that takes into account the difference between application fullscreen and page fullscreen. Tracked by: https://pad.lv/1464333

« Back to merge proposal