webbrowser-app

Merge lp:~gang65/webbrowser-app/webbrowser-app-keyboard-shortcuts into lp:webbrowser-app

webbrowser-app-keyboard-shortcuts
Merge into trunk

Proposed by Bartosz Kosiorek on 2015-02-09

Status:

Rejected

Rejected by:

Ugo Riboni on 2015-05-26

Proposed branch:

lp:~gang65/webbrowser-app/webbrowser-app-keyboard-shortcuts

Merge into:

lp:webbrowser-app

Diff against target:

120 lines (+112/-0)

1 file modified

src/app/webbrowser/Browser.qml (+112/-0)

To merge this branch:

bzr merge lp:~gang65/webbrowser-app/webbrowser-app-keyboard-shortcuts

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Ugo Riboni (community)			Disapprove on 2015-05-26
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-04-30
Bartosz Kosiorek (community)			Needs Information on 2015-04-30
Olivier Tilloy		2015-02-09	Needs Fixing on 2015-03-16
Review via email: mp+249134@code.launchpad.net

Description of the Change

I have added most needed shortcuts:
* Ctrl + L: Select the content in the address bar
* Ctrl + T: Open new Tab
* Ctrl + W: Close current Tab
* Ctrl + Tab: Navigate between tabs
* Ctrl + R: Reload current Tab
* Ctrl + D: Bookmark current Tab
* Ctrl + H: Show History
* Shift + Backspace: Goes to the next page in history
* Backspace: Goes to the previous page in history
* Alt + Left Arrow: Goes to the previous page in history
* Alt + Right Arrow: Goes to the next page in history
* Alt + D: Select the content in the address bar

Unfortunately it is impossible to implement switching to the next tab, without reimplementation tabs-model.cpp.
I think we could consider to use following private fields:
QList<QObject*> m_tabs; //store all tabs data
int current_tab_index; //store current tab index.

What do you think about such implementation?
It will simplify implementation and allow easily switching between tabs.

PS Jenkins bot (ps-jenkins) wrote on 2015-02-10:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1461/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-02-11:

Thanks for your work on this Bartosz! I haven’t had time to do a full review yet, but here are a few comments already, in no particular order:

- The changes to equality operators are unrelated, can you please revert them?

- When closing the current tab (Ctrl+W), the next tab will automatically be open. However the tab should be explicitly closed, see how it’s done there: http://bazaar.launchpad.net/~phablet-team/webbrowser-app/trunk/view/head:/src/app/webbrowser/TabsView.qml#L66

- For navigation between tabs: the tabs model is designed so that the current tab is always the first one, so you can’t really navigate linearly between tabs.

- Ctrl+D should check whether the current tab is already bookmarked, and if so remove the bookmark.

- Calls to currentWebview.go[Back|Forward] should be guarded by checks on the value of currentWebview.canGo[Back|Forward].

lp:~gang65/webbrowser-app/webbrowser-app-keyboard-shortcuts updated on 2015-02-12

899. By Bartosz Kosiorek on 2015-02-12: Implement changes after review

PS Jenkins bot (ps-jenkins) wrote on 2015-02-12:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1465/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-02-12:

86 + if (currentWebview && currentWebview.canGoForwar) {

typo here ^ (canGoForwar)

review: Needs Fixing

Bartosz Kosiorek (gang65) wrote on 2015-02-12:

Thanks Olivier for review.

I would like to ask it change:
- beginMoveRows(QModelIndex(), index, index, QModelIndex(), 0);
127 + beginMoveRows(QModelIndex(), index, m_tabs.count(), QModelIndex(), 0);

is ok for you.

Previously if you have tabs: "1 2 3 4"
and setCurrent to "2"
you will receive "2 1 3 4"

After my implementation the order of tabs are preserved: "2 3 4 1"

review: Needs Information

lp:~gang65/webbrowser-app/webbrowser-app-keyboard-shortcuts updated on 2015-02-13

900. By Bartosz Kosiorek on 2015-02-13: Fix typo

PS Jenkins bot (ps-jenkins) wrote on 2015-02-13:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1466/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-02-16:

> Thanks Olivier for review.
>
> I would like to ask it change:
> - beginMoveRows(QModelIndex(), index, index, QModelIndex(), 0);
> 127 + beginMoveRows(QModelIndex(), index, m_tabs.count(),
> QModelIndex(), 0);
>
> is ok for you.
>
>
> Previously if you have tabs: "1 2 3 4"
> and setCurrent to "2"
> you will receive "2 1 3 4"
>
> After my implementation the order of tabs are preserved: "2 3 4 1"

That wouldn’t conform to the specification any longer: tabs are always ordered by recency: if you have tabs "1 2 3 4", 1 is the most recent and 2 is the second most recent. Setting the current tab to 2 makes it the most recent, and consequently 1 becomes the second most recent, so the order should be "2 1 3 4" indeed.

Bartosz Kosiorek (gang65) wrote on 2015-02-16:

Thanks Olivier.
I would like to write my own method to switch between tabs, via keyboard shortcuts.

What do you think about that idea?

review: Needs Information

Olivier Tilloy (osomon) wrote on 2015-02-16:

I think it will make sense when the desktop version of the browser is augmented with an actual tab bar (and when that happens I believe the model will be different, not ordered by recency any more).
Until then, I suspect this would be tricky and unintuitive.

Bartosz Kosiorek (gang65) wrote on 2015-02-18:

I see two solutions for that issue:
1. Ask UX team if it will be possible to change webbrowser-app design, to allow fixed order tabs.
2. Remove completely switching tabs implementation from this MR.

Do you think it will be possible to apply first option (ask design team)?

review: Needs Information

Olivier Tilloy (osomon) wrote on 2015-02-18:

I know for sure that a fixed order for tabs won’t be considered for mobiles, but it will definitely be needed for desktop once we switch to visible tabs. That won’t happen in the very near future though. Given that, I would remove the tab switching implementation from this MR for now, and re-introduce it later on when the tabs model supports it.

lp:~gang65/webbrowser-app/webbrowser-app-keyboard-shortcuts updated on 2015-02-20

901. By Bartosz Kosiorek on 2015-02-19: Remove switching between tabs
902. By Bartosz Kosiorek on 2015-02-20: Add shortcut CTRL+F4 to close current tab

PS Jenkins bot (ps-jenkins) wrote on 2015-02-20:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1480/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-02-20:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1481/rebuild

review: Needs Fixing (continuous-integration)

lp:~gang65/webbrowser-app/webbrowser-app-keyboard-shortcuts updated on 2015-03-13

903. By Bartosz Kosiorek on 2015-03-13: [ Bartosz Kosiorek ]
* Add keyboard shortcuts support (LP: #1287361)
[ Olivier Tilloy ]
* Remove all references to private Qt headers, now that we require Qt
  5.4.
* Specify the cache path on the web context, to avoid caching data
  under ~/.local/share/. (LP: #1424726)
* Update translation template.
[ CI Train Bot ]
* New rebuild forced.
[ Olivier Tilloy ]
* Use the new Item::grabToImage() API (new in Qt 5.4) to replace the
  custom ItemCapture element. (LP: #1401581, #1425550)
[ Robert Bruce Park ]
* Launchpad automatic translations update.
[ Olivier Tilloy ]
* Fix a flaky autopilot test. (LP: #1423115)
* Use Qt::AA_ShareOpenGLContexts when building with Qt >= 5.4. (LP:
  #1387537)
[ Riccardo Padovani ]
* Fixed design of multiselection in history view (LP: #1412732)
[ CI Train Bot ]
* New rebuild forced.
[ Olivier Tilloy ]
* Honour Window.close() requests.
* Work around autopilot test failure by ensuring that the selection is
  cleared before clicking on the action button. (LP: #1417118)
* Add a config option to allow users to turn off automatic session
  restore.
* No-change rebuild against Qt 5.4.0.

PS Jenkins bot (ps-jenkins) wrote on 2015-03-13:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1507/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-03-16:

That looks good, thanks for the updates.

One question:

51 + if (bookmarksModel.contains(currentWebview.url)) {
52 + bookmarksModel.remove(currentWebview.url)
53 + }
54 + bookmarksModel.add(currentWebview.url, currentWebview.title, currentWebview.title)

Do you really mean to remove the bookmark and re-add it right away if the page was already bookmarked? Shouldn’t Ctrl+D toggle the bookmark status of the page?

Bartosz Kosiorek (gang65) wrote on 2015-03-16:

I thought that it is safer to re-add bookmark.
The website title could be changed, and user would like to update the bookmark, by pressing CTRL+D.

review: Needs Information

Olivier Tilloy (osomon) wrote on 2015-03-16:

OK, that makes sense.

Bartosz Kosiorek (gang65) wrote on 2015-03-16:

Hi Oliver.

Could you please approve this MR, if you don't have any other issues?

review: Needs Information

Olivier Tilloy (osomon) wrote on 2015-03-16:

I’m seeing a few functional issues:

- Ctrl+L (and Alt+D) doesn’t select the contents of the address bar, it merely gives it focus. I would expect it to select the entire URL for quick and easy edition.

- Pressing Ctrl+H doesn’t work, I get: Browser.qml:693: ReferenceError: toggleActivityView is not defined.

- Looks like there is some focus issue in the app itself: if I open the tabs view then close it by clicking on the "Done" button, keyboard shortcuts don’t work any longer. The issue doesn’t lie in the code you added, but it will need to be fixed as part of this MR to make it fully functional.

review: Needs Fixing

lp:~gang65/webbrowser-app/webbrowser-app-keyboard-shortcuts updated on 2015-03-23

904. By Bartosz Kosiorek on 2015-03-23: [ CI Train Bot ]
* New rebuild forced.
[ Olivier Tilloy ]
* On mobile, reveal the tabs view with a swipe gesture from the bottom
  edge. (LP: #1329943)
[ Olivier Tilloy ]
* Update an autopilot test to click on the drawer button to dismiss
  the drawer. With the upcoming changes to the text selection in the
  UITK, clicking on the address bar might accidentally grab a text
  selection handle, thus not dismissing the drawer menu. Clicking on
  the drawer button again should be more robust.

PS Jenkins bot (ps-jenkins) wrote on 2015-03-23:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1553/rebuild

review: Needs Fixing (continuous-integration)

lp:~gang65/webbrowser-app/webbrowser-app-keyboard-shortcuts updated on 2015-04-27

905. By Bartosz Kosiorek on 2015-04-27: [ Alexandre Abreu ]
* Add missing reload button from the webapp container as specified in
  the design document.
[ CI Train Bot ]
* New rebuild forced.
* Resync trunk.
[ Ken VanDine ]
* added ShareLink to contextualActions
[ Leo Arias ]
* In the autopilot tests, removed the extra focus step to write a URL.
  (LP: #1441551)
[ Olivier Tilloy ]
* Always exit fullscreen mode when the application becomes inactive.
  (LP: #1331475)
* Recognize about:blank as a valid URL. (LP: #1444139)
* Save the updated homepage when pressing return. (LP: #1441874)
[ Ugo Riboni ]
* Include bookmark results in the suggestions list (LP: #1351177)
[ Arthur Mello ]
* Add model support to control which history entries will be displayed
  based on a blacklist database
* Make Top Sites format equal to Bookmarks on the New Tab view
[ CI Train Bot ]
* New rebuild forced.
* Resync trunk.
[ Alexandre Abreu ]
* remove qtwebkit deps (LP: #1362640) (LP: #1362640)
[ CI Train Bot ]
* New rebuild forced.
[ Justin McPherson ]
* Command line options for media-hub use through Oxide.
[ Olivier Tilloy ]
* Update translation template.
[ CI Train Bot ]
* New rebuild forced.
* Resync trunk.
[ Olivier Tilloy ]
* Add a "Clear Cache" entry under the privacy settings. (LP: #1296364,
  #1260014)
* Add settings page, per design specification. This adds qml-module-
  qt-labs-folderlistmodel and qml-module-qt-labs-settings as runtime
  dependencies for webbrowser-app. (LP: #1351183)
* Autopilot tests for the settings UI. added:
  tests/autopilot/webbrowser_app/tests/test_settings.py
[ Riccardo Padovani ]
* Add settings page, per design specification. This adds qml-module-
  qt-labs-folderlistmodel and qml-module-qt-labs-settings as runtime
  dependencies for webbrowser-app. (LP: #1351183)
* Add settings page, per design specification. This adds qml-module-
  qt-labs-folderlistmodel and qml-module-qt-labs-settings as runtime
  dependencies for webbrowser-app. (LP: #1351183)
[ CI Train Bot ]
* New rebuild forced.
[ Olivier Tilloy ]
* Always initialize member attribute at construction time.
* Remove two broken symlinks. removed:
  src/Ubuntu/Components/Extras/Browser/favicon-image-provider.cpp@
  src/Ubuntu/Components/Extras/Browser/favicon-image-provider.h@
* Rewrite URLs with an uppercase scheme. (LP: #1436312)
* Use the new locationBarController API available in oxide 1.5 to
  control the position of the chrome. (LP: #1365179, #1429132)
[ Olivier Tilloy ]
* Discard selection when navigating away. (LP: #1433503)
* Expose the SearchEngine type to QML.
* Remove workaround for bug #1377198, which was fixed in oxide 1.5.
  (LP: #1377198)
* Rename test helper function from clear_cache() to clear_datadir(),
  to better reflect what it really does.
* Update the 'selection' test page to hopefully fix the failing
  autopilot test on mako. (LP: #1434193, #1433020)
* Update translation template.
* Use the loadEvent signal (introduced in oxide 1.3).
906. By Bartosz Kosiorek on 2015-04-27: Post review changes

Bartosz Kosiorek (gang65) wrote on 2015-04-27:

Everything is ready except selecting address while pressing CTRL+L
Do you know how to access to AddressBar.qml TextField from Browser.qml?

review: Needs Information

Olivier Tilloy (osomon) wrote on 2015-04-27:

> Do you know how to access to AddressBar.qml TextField from Browser.qml?

You shouldn’t try to access the internals of AddressBar from Browser.qml. Instead, a method should be added to AddressBar to select all.

PS Jenkins bot (ps-jenkins) wrote on 2015-04-27:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1695/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-04-28:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1696/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-04-28:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1698/rebuild

review: Needs Fixing (continuous-integration)

lp:~gang65/webbrowser-app/webbrowser-app-keyboard-shortcuts updated on 2015-04-30

907. By Bartosz Kosiorek on 2015-04-30: Add switching between tabs key shortcut
908. By Bartosz Kosiorek on 2015-04-30: Add switching between tabs feature

Bartosz Kosiorek (gang65) wrote on 2015-04-30:

Hi Oliver.
I have implemented switching between tabs shortcut.
I already implemented function to selecting all textfield in Addressbar.

How I could access to that function from browser.qml?

review: Needs Information

Bartosz Kosiorek (gang65) wrote on 2015-04-30:

In this revision there is implemented function to select all textField:
http://bazaar.launchpad.net/~gang65/webbrowser-app/webbrowser-app-keyboard-shortcuts/revision/907

PS Jenkins bot (ps-jenkins) wrote on 2015-04-30:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1717/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-04-30:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1718/rebuild

review: Needs Fixing (continuous-integration)

Ugo Riboni (uriboni) wrote on 2015-05-06:

I noticed the following issues:

1) CTRL+W and CTRL+TAB should work also when the "new tab" page is visible. Right now they do nothing when on that page.

2) CTRL+H shows history, but pressing CTRL+H again on that page will cause the browser to show an empty screen and there is no way to get out of it.

3) As Olivier also mentioned, when pressing ALT+L or CTRL+L the entire text of the address bar should be selected. Add a new function to AddressBar.qml to do this and then another one to Chrome.qml that calls it. Then from Browser.qml call the function exposed by Chrome.qml

I also would add the following things that are not explicit in the design but they are what most users who are used to keyboard navigation would expect in a browser:

1) CTRL+D should toggle bookmarks, not only add them. This should be the same behaviour as clicking on the bookmark star in the address bar.

2) Pressing ESC should escape the current context:
- defocus the address bar if it is focused
- return to the page if any non-page screen is open (history, settings, etc)
Otherwise there is no way to escape these pages once you get to them with the other keyboard shortcuts

3) History navigation has ALT+LEFT and ALT+RIGHT, but tabs navigation has only CTRL+TAB. Intuitively we need also CTRL+SHIFT+TAB to navigate backwards in the list.

review: Needs Fixing

Ugo Riboni (uriboni) wrote on 2015-05-26:

Please do not do any further work on this proposal.
It has been taken over and superseded by the following merge request: https://code.launchpad.net/~uriboni/webbrowser-app/keyboard-navigation/+merge/260183

Thanks for your contributions so far, they have been a great base on which to work !

review: Disapprove

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Bartosz Kosiorek

Ubuntu Phablet Team

to status/vote changes:

Rachel Liu

 === modified file 'src/app/webbrowser/Browser.qml'
 --- src/app/webbrowser/Browser.qml	2015-04-22 10:25:50 +0000
 +++ src/app/webbrowser/Browser.qml	2015-04-30 19:17:21 +0000
@@ -952,4 +952,116 @@
+             }
+         }
+     }
++
++    Keys.onPressed: {
++        if (event.modifiers & Qt.ControlModifier) {
++            switch(event.key) {
++            case Qt.Key_L:
++                // Ctrl + l: Select the content in the address bar
++                internal.focusAddressBar();
++                event.accepted = true;
++                break;
++
++            case Qt.Key_T:
++                // Ctrl + t: Open a new Tab
++                openUrlInNewTab("", true);
++                event.accepted = true;
++                break;
++
++            case Qt.Key_W:
++            case Qt.Key_F4:
++                // Ctrl + w: Close the current Tab
++                if (tabsModel.count >= 0) {
++                    var tab = tabsModel.remove(0);
++                    if (tab) {
++                        tab.close()
++                    }
++
++                    if (tabsModel.count === 0) {
++                        browser.openUrlInNewTab("", true)
++                    }
++                    event.accepted = true;
++                }
++                break;
++
++            case Qt.Key_Tab:
++                // Ctrl + Tab: Navigate between tabs
++                var tab = tabsModel.get(tabsModel.count - 1)
++                if (tab) {
++                    tab.forceActiveFocus()
++                    tab.load()
++                    tabslist.model.setCurrent(tabsModel.count - 1)
++                }
++                recentView.reset()
++                event.accepted = true;
++                break;
++
++            case Qt.Key_R:
++                // Ctrl + R: Reload current Tab
++                if (currentWebview) {
++                    currentWebview.reload()
++                    event.accepted = true;
++                }
++                break;
++
++            case Qt.Key_D:
++                // Ctrl + D: Bookmark current Tab
++                if (currentWebview) {
++                    if (bookmarksModel.contains(currentWebview.url)) {
++                         bookmarksModel.remove(currentWebview.url)
++                    }
++                    bookmarksModel.add(currentWebview.url, currentWebview.title, currentWebview.title)
++                    event.accepted = true;
++                }
++                break;
++
++            case Qt.Key_H:
++                // Ctrl + H: Show History
++                historyViewComponent.createObject(historyViewContainer);
++                event.accepted = true;
++                break;
++            }
++        } else if (event.modifiers & Qt.AltModifier) {
++            switch(event.key) {
++            case Qt.Key_Left:
++                // Alt + Left Arrow: Goes to the previous page in history
++                if (currentWebview && currentWebview.canGoBack) {
++                    currentWebview.goBack();
++                    event.accepted = true;
++                }
++                break;
++
++            case Qt.Key_Right:
++                // Alt + Right Arrow: Goes to the previous page in history
++                if (currentWebview && currentWebview.canGoForward) {
++                    currentWebview.goForward();
++                    event.accepted = true;
++                }
++                break;
++
++            case Qt.Key_D:
++                // Alt + d: Select the content in the address bar
++                internal.focusAddressBar();
++                event.accepted = true;
++                break;
++            }
++
++        } else if (event.modifiers & Qt.ShiftModifier) {
++            switch(event.key) {
++            case Qt.Key_Backspace:
++                // Shift + Backspace: Goes to the next page in history
++                if (currentWebview && currentWebview.canGoForward) {
++                    currentWebview.goForward();
++                    event.accepted = true;
++                }
++                break;
++            }
++        } else if (event.key & Qt.Key_Backspace) {
++            // Backspace: Goes to the previous page in history
++            if (currentWebview && currentWebview.canGoBack) {
++                currentWebview.goBack();
++                event.accepted = true;
++            }
++        }
++    }
+ }

webbrowser-app

Merge lp:~gang65/webbrowser-app/webbrowser-app-keyboard-shortcuts into lp:webbrowser-app

Commit Message

Description of the Change

Unmerged revisions

Preview Diff

Subscribers