436068 - Basic Navigation

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Christian Sejersen]

Support for back, forward, reload, home

Comment 1

[Christian Sejersen]

As soon as the UI for basic nav (bow or side-control) has reached a point where we can start implementing it, just assign it back to me.

Comment 2

[Madhava Enros [:madhava]]

The "Basic Usage" and "Controls" sections of this page:

http://wiki.mozilla.org/Mobile/UI/Designs/TouchScreen/workingUI

describe how I'd suggest we lay out the UI for basic navigation.

Back and Forward are in the control strip.  Reload (as well as Stop) is in the titlebar.  Home will eventually be in the Bookmarks UI.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: v1 screenshot

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: v1 screenshot in edit mode

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: v1 WIP patch

This patch:
* Removes a lot of unused stuff from browser.xul and browser.js
* Adds structure to browser.xul for the toolbar, sidebar, url list w/ search and notifications
* Updates browser.css with the new style
* Adds browser-ui.js to handle the new chrome behavior
* Adds a simple mono-sidebar.png for the sidebar images

This patch does not hide the toolbar or sidebar, they are always present. We need to add support for panning edge "bumps" so we can show the toolbar and sidebar at the right times.

I want to make sure the code is clean enough and (maybe) hook up support for bookmark lists, then ask for review.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: screenshot of bookmark editor

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: screenshot of bookmark list

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: v2 changes to the UI to match the new working UI

This patch has all that v1 had and:
* Basic Bookmark Editing form (click on a solid star)
* Basic bookmarks list

We still need the panning "bump" feature to show/hide the UI elements, but that can wait for followup bugs. As can updates to the styles and images.

Comment 9

[Johnathan Nightingale [:johnath]]

Comment on attachment 327921 [details] [diff] [review]
v1 WIP patch

>   onLocationChange : function(aWebProgress, aRequest, aLocation) {
>-    
>+
>     this._hostChanged = true;
>-    
>+

This work is looking great, Mark.  For the benefit of those of us following along at home, any chance the next WIP patch can strip out whitespace-only changes?  :)

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: v2 with -w (skipping whitespace changes)

Same as v2, but without whitespace changes for easier reading

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 328044 [details] [diff] [review]
v2 changes to the UI to match the new working UI

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+  _titleChanged : function(aEvent) {

>+      this._caption.value = aEvent.target.title;

nit: indentation's off.

>+  _linkAdded : function(aEvent) {
>+    var link = aEvent.originalTarget;
>+    var rel = link.rel && link.rel.toLowerCase();
>+    if (!link || !link.ownerDocument || !rel || !link.href)
>+      return;

Would have thrown already if !link. Looks like the browser code this is copied from has the same problem :/

>+      this._setIcon(link.href);

>+  _setIcon : function(aURI) {

>+    var faviconURI = ios.newURI(aURI, null, null);

>+    this._favicon.setAttribute("src", faviconURI.spec);

This needs a security check - see bug 290036.

>+  update : function(aState) {

>+    var toolbar = document.getElementById("toolbar-main");
>+    if (aState == TOOLBARSTATE_LOADING) {

>+      this._throbber.setAttribute("src", "chrome://browser/skin/images/throbber.gif");

Can happen in a followup, but would be better to use an attribute/class + CSS to style this rather than hardcoding the image URI.

>+  /* Set the location to the current content */
>+  setURI : function() {

>+    // Check for a bookmarked page
>+    var star = document.getElementById("tool-star");
>+    star.removeAttribute("starred");

Put this in an else below, to avoid removing/setting the attribute unnecessarily?

>+    if (bookmarks.length > 0) {
>+      star.setAttribute("starred", "true");
>+    }

>+  goToURI : function(aURI) {

>+    try {
>+      aURI = this._URIFixup.createFixupURI(aURI, 0);
>+      aURI = this._URIFixup.createExposableURI(aURI);
>+    }

Not sure this is right for loading... createExposableURI will strip passwords from URIs, e.g. Why both, and why in that order? Can revisit this later.

>+  search : function() {

We'll need a bug on having this use the search service, I guess?

>+  _showPlaces : function(aItems) {
>+    var list = document.getElementById("urllist-items");
>+    while (list.childNodes.length > 0) {
>+      list.removeChild(list.childNodes[0]);

I think while(list.firstChild) { list.removeChild(list.firstChild) } is faster? Use firstChild instead of childNodes[0] either way.

>+    if (aItems.length > 0) {
>+      for (var i=0; i<aItems.length; i++) {

the if is unneeded, right?

>+  handleEvent: function (aEvent) {

Would be nice to have comments on what the expected targets of these listeners are... or just inline the handlers in the markup for the ones that delegate to another function.

>+      case "error":
>+        this._favicon.setAttribute("src", "chrome://browser/skin/images/default-favicon.png");

Same comment about styling, can fix that later.

>+  doCommand : function(cmd) {

>+    switch (cmd) {
>+      case "cmd_back":
>+        browser.stop();
>+        browser.goBack();
>+        break;
>+      case "cmd_forward":
>+        browser.stop();
>+        browser.goForward();
>+        break;

Are the stop()s really needed?

>+var BookmarkHelper = {

>+  save : function() {

>+      // Update the tags
>+      var taglist = document.getElementById("hudbookmark-tags").value;
>+      var currentTags = this._tagsvc.getTagsForURI(this._uri, {});
>+      var tags = taglist.split(" ");;

nit: ;;

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>-      case "cmd_addons":

>-      case "cmd_downloads":

Probably best to just leave this for now, assuming they work OK.

>diff --git a/chrome/jar.mn b/chrome/jar.mn

Don't forget to hg remove the files being removed here.

>diff --git a/chrome/locale/en-US/browser.dtd b/chrome/locale/en-US/browser.dtd

Should go through these and make sure there aren't any unused strings once this chrome work settles down a bit.

>diff --git a/chrome/skin/browser.css b/chrome/skin/browser.css

>+#toolbar-main toolbarbutton {

>+#browser-controls toolbarbutton {

Could factor the styles these have in common out, and use classes rather than the descendant selector.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #11)
> (From update of attachment 328044 [details] [diff] [review])
> >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
> 
> >+  _titleChanged : function(aEvent) {
> 
> >+      this._caption.value = aEvent.target.title;
> 
> nit: indentation's off.

fixed

> 
> >+  _linkAdded : function(aEvent) {
> >+    var link = aEvent.originalTarget;
> >+    var rel = link.rel && link.rel.toLowerCase();
> >+    if (!link || !link.ownerDocument || !rel || !link.href)
> >+      return;
> 
> Would have thrown already if !link. Looks like the browser code this is copied
> from has the same problem :/

moved 'if' check to prevent throw problem (and remove !rel from check)

> 
> >+      this._setIcon(link.href);
> 
> >+  _setIcon : function(aURI) {
> 
> >+    var faviconURI = ios.newURI(aURI, null, null);
> 
> >+    this._favicon.setAttribute("src", faviconURI.spec);
> 
> This needs a security check - see bug 290036.

added a check for "javascript" scheme

> 
> >+  /* Set the location to the current content */
> >+  setURI : function() {
> 
> >+    // Check for a bookmarked page
> >+    var star = document.getElementById("tool-star");
> >+    star.removeAttribute("starred");
> 
> Put this in an else below, to avoid removing/setting the attribute
> unnecessarily?

fixed

> >+  goToURI : function(aURI) {
> 
> >+    try {
> >+      aURI = this._URIFixup.createFixupURI(aURI, 0);
> >+      aURI = this._URIFixup.createExposableURI(aURI);
> >+    }
> 
> Not sure this is right for loading... createExposableURI will strip passwords
> from URIs, e.g. Why both, and why in that order? Can revisit this later.

createExposableURI alone would not allow "bugzilla.mozila.org" to work. createFixupURI will add the "http" so createExposableURI won't choke.

> 
> >+  search : function() {
> 
> We'll need a bug on having this use the search service, I guess?

Yeah, part of bug 431842

> 
> >+  _showPlaces : function(aItems) {
> >+    var list = document.getElementById("urllist-items");
> >+    while (list.childNodes.length > 0) {
> >+      list.removeChild(list.childNodes[0]);
> 
> I think while(list.firstChild) { list.removeChild(list.firstChild) } is faster?

changed

> >+    if (aItems.length > 0) {
> >+      for (var i=0; i<aItems.length; i++) {
> 
> the if is unneeded, right?

right

> 
> >+  handleEvent: function (aEvent) {
> 
> Would be nice to have comments on what the expected targets of these listeners
> are... or just inline the handlers in the markup for the ones that delegate to
> another function.

added comments for now

> 
> Are the stop()s really needed?

Didn't look like it. Testing still worked fine

> >diff --git a/chrome/content/browser.js b/chrome/content/browser.js
> 
> >-      case "cmd_addons":
> >-      case "cmd_downloads":
> 
> Probably best to just leave this for now, assuming they work OK.

added them back

> 
> >diff --git a/chrome/jar.mn b/chrome/jar.mn
> 
> Don't forget to hg remove the files being removed here.

removed unused files

> >diff --git a/chrome/skin/browser.css b/chrome/skin/browser.css
> 
> Could factor the styles these have in common out, and use classes rather than
> the descendant selector.
> 

changed

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: v3 patch for landing

Patch with review fixes. Carrying r+

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

changeset:   43:80443d83ae0c
changeset:   42:97ed9d8da283
changeset:   41:eeb6db31bd9e

Comment 15

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.3a1pre) Gecko/20090817 Fennec/1.0a3pre


Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20090817
Fennec/1.0b3pre

and it's supported on litmus over smoketest testcases:
https://litmus.mozilla.org/show_test.cgi?id=7087
https://litmus.mozilla.org/show_test.cgi?id=7083
https://litmus.mozilla.org/show_test.cgi?id=7091