Merge lp:~uriboni/webbrowser-app/keyboard-navigation into lp:webbrowser-app
| Status: | Merged | ||||
|---|---|---|---|---|---|
| Approved by: | Olivier Tilloy on 2015-06-16 | ||||
| Approved revision: | 1097 | ||||
| Merged at revision: | 1063 | ||||
| Proposed branch: | lp:~uriboni/webbrowser-app/keyboard-navigation | ||||
| Merge into: | lp:webbrowser-app | ||||
| Prerequisite: | lp:~rpadovani/webbrowser-app/newTabRefactoring | ||||
| Diff against target: |
1236 lines (+756/-78) 16 files modified
src/app/webbrowser/AddressBar.qml (+16/-8) src/app/webbrowser/Browser.qml (+213/-21) src/app/webbrowser/Chrome.qml (+3/-0) src/app/webbrowser/KeyboardShortcut.qml (+25/-0) src/app/webbrowser/KeyboardShortcuts.qml (+41/-0) src/app/webbrowser/Suggestion.qml (+3/-2) src/app/webbrowser/Suggestions.qml (+40/-38) src/app/webbrowser/limit-proxy-model.cpp (+16/-0) src/app/webbrowser/limit-proxy-model.h (+2/-0) tests/autopilot/webbrowser_app/emulators/browser.py (+17/-2) tests/autopilot/webbrowser_app/tests/__init__.py (+10/-2) tests/autopilot/webbrowser_app/tests/http_server.py (+4/-0) tests/autopilot/webbrowser_app/tests/test_addressbar_bookmark.py (+0/-2) tests/autopilot/webbrowser_app/tests/test_keyboard.py (+276/-0) tests/autopilot/webbrowser_app/tests/test_suggestions.py (+68/-3) tests/unittests/limit-proxy-model/tst_LimitProxyModelTests.cpp (+22/-0) |
||||
| To merge this branch: | bzr merge lp:~uriboni/webbrowser-app/keyboard-navigation | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Olivier Tilloy | 2015-05-26 | Approve on 2015-06-16 | |
| PS Jenkins bot | continuous-integration | Needs Fixing on 2015-06-16 | |
|
Review via email:
|
|||
Commit Message
Make the browser chrome usable on desktop by implementing common keyboard shortcuts and behaviors that users normally expect in such an app
Description of the Change
The goal of this merge request is to make the browser chrome usable on desktop by implementing common keyboard shortcuts and behaviors that users normally expect in such an app.
Some come directly from the design document and some have been added based on what is obvious and intuitive (and already by other browsers).
This is based on, and supersedes, the following contributor branch: https:/
It is also based on this refactoring branch: https:/
Finally it also supersedes this branch adding keyboard support to the suggestions list, as it is essentially a superset of that work so they might as well go together: https:/
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1050
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Olivier Tilloy (osomon) wrote : | # |
The flake8 unit test fails.
A few functional issues (I haven’t done a complete review yet):
- If the address bar is empty and focused (e.g. when opening a new blank tab), pressing the down arrow key removes focus from it, and pressing the up arrow key doesn’t restore it.
- If I type a word in the address bar and wait for search suggestions to appear, then press the down arrow key to navigate the suggestions list, the search suggestions disappear.
- If I focus the address bar, then open the drawer menu with the mouse and open the history view from there, it cannot be dismissed with ESC (it works as expected if the address bar wasn’t focused or if the view was opened with Ctrl+H).
| Ugo Riboni (uriboni) wrote : | # |
> - If the address bar is empty and focused (e.g. when opening a new blank
> tab), pressing the down arrow key removes focus from it, and pressing the up
> arrow key doesn’t restore it.
This was fixed by making sure we can move to the suggestions list with Down only if there are actually items in it.
> - If I type a word in the address bar and wait for search suggestions to
> appear, then press the down arrow key to navigate the suggestions list, the
> search suggestions disappear.
This was more tricky than how you described it. It happened only if the only suggestions in the list were from the search engine. If there were history or bookmarks in the list it did not happen.
The cause was the fact that the search engine suggestions model would be disabled (therefore reducing the count of suggestion to zero and closing the list) when the chrome did not have the focus, but did not take into account the case when the suggestion list itself had focus.
> - If I focus the address bar, then open the drawer menu with the mouse and
> open the history view from there, it cannot be dismissed with ESC (it works as
> expected if the address bar wasn’t focused or if the view was opened with
> Ctrl+H).
Wasn't giving focus to the history page when activating from menu.
Note: I added a test case to prevent the first issue from re-occurring, but I have no idea how to test for the other two, as the tests would need to verify that *eventually* something does not happen, which is hard to do without using arbitrary waits that make the tests fragile.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1056
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1057
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Olivier Tilloy (osomon) wrote : | # |
The following autopilot test is broken on desktop:
webbrowser_
| Olivier Tilloy (osomon) wrote : | # |
A few more functional issues I found while testing:
- If I load a page, then focus the address bar, then type e.g. "ab", then press the down arrow to navigate the suggestions, then press the up arrow to re-focus the address bar, then press ESC, the current URL is not restored in the address bar (if I do it while navigating the suggestions it is restored)
- If I’m browsing a page that has a favicon and that’s loaded over a secure connection (i.e. the lock icon is displayed), and if I focus the address bar to start typing search terms (e.g. "ab"), the favicon and the lock icon are hidden, and a magnifier icon is displayed. But if I then press the down key to navigate the suggestions, the favicon and lock icon are displayed again. I think the magnifier icon should remain when navigating through the suggestions.
- On a page where I can navigate back/forward, if I focus the address bar with Ctrl+L, then press Alt+[Left/Right], the previous/next page is loaded, but the address bar is cleared. I would say that navigating back/forward should remove focus from the address bar (and that should solve the issue).
- If I open the history view with Ctrl+H, then press Ctrl+L, nothing visible happens (as expected because the address bar is not visible), however I can’t close the history view with ESC any longer. I think Ctrl+L should not focus the address bar if it’s not visible.
- Similarly, Ctrl+D to (un)bookmark also works when the history view is visible, but it shouldn’t.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1059
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1063
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1064
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Olivier Tilloy (osomon) wrote : | # |
webbrowser_
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1065
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Olivier Tilloy (osomon) wrote : | # |
ESC in the tabs view (recentView) and in the settings page should exit the view (I think it’s ok if more advanced keyboard navigation is not implemented in those views yet, but at the very least it should be possible to leave them using the keyboard).
It might be interesting to have Ctrl+T, Ctrl+W and Ctrl+Tab work while recentView is visible.
The current implementation of Ctrl+Tab doesn’t switch to the next tab, it switches to the last one, is that intended?
I’m not fond of the name of the 'textLocked' property: it kind of suggests that the text cannot be modified, which isn’t true, because keyboard input will modify the text. Can we think of a more appropriate name?
In internal.
In the definition of LimitProxyModel
In Browser.
Can PrepopulatedDat
According to https:/
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1069
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Ugo Riboni (uriboni) wrote : | # |
What does not have a reply was simply fixed.
> ESC in the tabs view (recentView) and in the settings page should exit the
> view (I think it’s ok if more advanced keyboard navigation is not implemented
> in those views yet, but at the very least it should be possible to leave them
> using the keyboard).
> It might be interesting to have Ctrl+T, Ctrl+W and Ctrl+Tab work while
> recentView is visible.
I did all of the above, and also unified the way we exit the tabs view (by switching to whatever tab is current, or by switching to the tab that the user clicked on). This helps fix the problem of sometimes not focusing the address bar when the new tab page is selected (and allows having less places where to put the conditional to focus it only when on desktop that you mention below).
> The current implementation of Ctrl+Tab doesn’t switch to the next tab, it
> switches to the last one, is that intended?
Yes, due to the way tabs are implemented the current tab is always brought to the top of the stack, so this is by design.
> I’m not fond of the name of the 'textLocked' property: it kind of suggests
> that the text cannot be modified, which isn’t true, because keyboard input
> will modify the text. Can we think of a more appropriate name?
I suck at naming variables. preventSimplifyText is the best I could come up with. Hope it works.
> In Browser.
> press a key? Shouldn’t the instance be created in Browser.__init__() instead?
> And why is there a try… except block? When can we get a RuntimeError for
> pressing a key? Silently swallowing exceptions is usually not a good practice,
> except in very specific cases.
This was from the autopilot guide. I think the create() method can give a RuntimeError if no keyboard is available, which is also why I was getting the keyboard only when in need to send a keystroke.
However I looked at the UI toolkit emulators and they get the keyboard at set up and without any error checking, so it must be safe. I did the same thing that they do now.
> Can PrepopulatedDat
> that all tests can re-use?
I see little point. Most of the class is populating the database with data, and the data is different for every test.
You could possibly generalize some of the database access in a base class and allow subclasses to provide only the list of data, but it would be much work for little advantage in my opinion.
I'd rather avoid this.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1074
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1074
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Olivier Tilloy (osomon) wrote : | # |
When in the tabs view, pressing Ctrl+T to open a new tab should probably dismiss the view (just like clicking the "New Tab" button does).
> I suck at naming variables. preventSimplifyText is the best I could
> come up with. Hope it works.
While it works, I must say I don’t like the name very much either. I would prefer 'canSimplifyText' (changing the semantics of the property to its opposite).
> You could possibly generalize some of the database access in a base
> class and allow subclasses to provide only the list of data, but it
> would be much work for little advantage in my opinion.
Yeah, that’s what I was kind of suggesting. We can do that separately later on though.
In AddressBar.qml:
- updateUrlFromFo
- I’m not sure I understand why the test on 'preventSimplif
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
- F6 should focus the address bar, just like Ctrl+L and Alt+D
- and F5 for page reload, as you suggested, would be a useful addition, too
| Ugo Riboni (uriboni) wrote : | # |
All done except where commented below:
> - I’m not sure I understand why the test on 'preventSimplif
> inside updateUrlFromFo
> 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:/
| Olivier Tilloy (osomon) wrote : | # |
> 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.
Right, and we should probably consider refactoring that code to include the suggestions list in the address bar. In a separate branch, of course.
| Olivier Tilloy (osomon) wrote : | # |
We’re getting there nicely. A few more minor remarks:
288 + if (tabsModel.count >= 0) {
It should be (tabsModel.count > 0), there is no point in trying to close the current tab if there isn’t any.
680 + def setUp(self, url="/test1"):
That argument would better be named 'path', rather than 'url'.
986 + self.assertThat
Although functionally equivalent, GreaterThan would be more appropriate than NotEquals.
996 + def test_keyboard_
Can test_keyboard_
1085 + QCOMPARE(
Can I suggest QVERIFY(
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1081
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1082
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1085
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Riccardo Padovani (rpadovani) wrote : | # |
Thanks for working on this, now the desktop experience is definitely better!
I leave some impression, don't know if you want to fix in this branch or in a latter one
Some comments:
- I expect to navigate the history with arrows after I open it with CTRL+H
- If I navigate to a page I can scroll with arrows (good!), then I open history with CTRL+H, then I press ESC, arrows don't do anything anymore. I expect they still scroll the page
Some more keybinding I would like:
- CTRL+SHIFT+T to reopen the last closed tab
- ALT+F (as on Chromium) to open the menu and then arrows to navigate it
- F11 to go fullscreen (but this I think requires some other works)
I took a fast look to the code and, apart of a little typo I left a comment inline, looks good to me, except Python I didn't review due my lack of knowledge of Python.
Cool work!
| Olivier Tilloy (osomon) wrote : | # |
I don’t think the fix in revision 1085 is correct, the tabs view is not being open any longer, so it breaks the test assumption, doesn’t it?
| Olivier Tilloy (osomon) wrote : | # |
> I expect to navigate the history with arrows after I open it with CTRL+H
We agreed that this would be implemented later on
> If I navigate to a page I can scroll with arrows (good!), then I open history with CTRL+H, then I press ESC, arrows don't do anything anymore. I expect they still scroll the page
Good catch, active focus should be restored on the webview when exiting the history view, if the address bar wasn’t focused previously.
> CTRL+SHIFT+T to reopen the last closed tab
Would be nice to have, but will require more work to keep track of recently closed tab, so let’s do that separately.
> ALT+F (as on Chromium) to open the menu and then arrows to navigate it
Would be nice to have too, but not a must for this iteration. Again, let’s do that separately.
> F11 to go fullscreen (but this I think requires some other works)
Yes, this is bug #1464333.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1087
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1089
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Olivier Tilloy (osomon) wrote : | # |
webbrowser_
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1093
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1093
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1094
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://

FAILED: Continuous integration, rev:1049 jenkins. qa.ubuntu. com/job/ webbrowser- app-ci/ 1813/ jenkins. qa.ubuntu. com/job/ generic- deb-autopilot- vivid-touch/ 2950/console jenkins. qa.ubuntu. com/job/ webbrowser- app-vivid- amd64-ci/ 570/console jenkins. qa.ubuntu. com/job/ webbrowser- app-vivid- armhf-ci/ 570/console jenkins. qa.ubuntu. com/job/ webbrowser- app-vivid- i386-ci/ 570/console jenkins. qa.ubuntu. com/job/ generic- mediumtests- builder- vivid-armhf/ 2948/console
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/webbrowser- app-ci/ 1813/rebuild
http://