902713 - Defect - "mode" in urlbar is overloaded, split editing & loading apart

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Input, defect, P2)

Description

[Allison Naaktgeboren :ally]

and has caused some problems (such as beginEditing() being called but not endEditing() see bug 894811).

Comment 1

[Jim Mathies [:jimm]]

css, mode=

http://mxr.mozilla.org/mozilla-central/search?string=mode%3D&find=%2Fbrowser%2Fmetro%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

bcast_urlbarState / urlbarState

http://mxr.mozilla.org/mozilla-central/search?string=urlbarState&find=%2Fbrowser%2Fmetro%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 3

[Jim Mathies [:jimm]]

We seemed to have an awful lot of these strewn around that weren't needed (or I'm just plain missing something.) I went through css and the code looking for references to 'mode', but only a found a few related to the urlbar buttons.

Comment 4

[Jim Mathies [:jimm]]

Note, I've built this on top of the work in bug 851130, so it'll land with those patches.

Comment 5

[Jonathan Wilde [:jwilde]]

Comment on attachment 787746 [details] [diff] [review]
patch v.1

Review of attachment 787746 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, looks good. Some tweaks requested below. Will change r flag once my build finishes.

::: browser/metro/base/content/browser-ui.js
@@ +703,5 @@
>      }
>    },
>  
>    _updateToolbar: function _updateToolbar() {
> +    Elements.loadingState.setAttribute("loading", Browser.selectedTab.isLoading());

We should replace this with a setAttribute/removeAttribute conditional block per the comments in browser.css.

::: browser/metro/base/content/browser.xul
@@ -175,5 @@
>      <key id="key_selectLastTab" oncommand="BrowserUI.selectTabAtIndex(-1);" key="9" modifiers="accel"/>
>    </keyset>
>  
>    <stack id="stack" flex="1">
> -    <observes element="bcast_urlbarState" attribute="*"/>

It looks like there's an autocomplete="true" attribute added to bcast_urlbarState when the half-height autocomplete shows up and a CSS selector that hides the overlay buttons on #stack[autocomplete], so we should leave this in for now.

@@ -249,5 @@
>            </hbox>
>  
>            <stack id="toolbar-contextual">
>              <observes element="bcast_windowState" attribute="*"/>
> -            <observes element="bcast_urlbarState" attribute="*"/>

We also need this here. This allows us to hide the excess page-specific buttons on autocomplete and shows the close button that lets the user step out of autocomplete.

::: browser/metro/theme/browser.css
@@ +672,5 @@
>      list-style-image: url(chrome://browser/skin/images/[e-mail]);
>    }
>  }
>  
> +/* one button out of three - when editing: go, when !editing, loading: stop, when !editing, !loading: reload */ 

Nit: space.

@@ +680,5 @@
> +}
> +
> +#urlbar[mode="edit"] > #go-button,
> +#urlbar[mode="view"][loading="true"] > #stop-button,
> +#urlbar[mode="view"][loading="false"] > #reload-button {

Regarding loading:

I think this would be more in line with the boolean attribute conventions in the codebase if we had loading="true" for true and removed the loading attribute entirely for false.

Regarding mode:

As far as I can tell, this is boolean. (Autocompletion is signified with a separate "autocomplete" attribute added to bcast_urlbarState.) Let's rename mode to "editing" and have it follow the same boolean attribute conventions as for loading?

We'll have to make some additional changes to browser.css and urlbar.xml, but I think it'll be a little more robust and tidy.

We can use the following selector for showing the buttons:

#urlbar[editing] > #go-button,
#urlbar:not([editing])[loading] > #stop-button,
#urlbar:not([editing]):not([loading]) > #reload-button

There'd be an inherent default button state (the reload button showing) if we don't have any attributes added to #urlbar.

Comment 6

[Jonathan Wilde [:jwilde]]

Comment on attachment 787746 [details] [diff] [review]
patch v.1

Review of attachment 787746 [details] [diff] [review]:
-----------------------------------------------------------------

r=jwilde with the changes described earlier.

Comment 7

[Jim Mathies [:jimm]]

Updated per comments - 

https://hg.mozilla.org/integration/fx-team/rev/48879353a32c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48879353a32c

Comment 9

[Samvedana (:Samvedana)]

Can you please provide steps to test this?

Comment 10

[Jim Mathies [:jimm]]

(In reply to Samvedana (:Samvedana) from comment #9)
> Can you please provide steps to test this?

hmm, maybe just the navigation button states - looking at the icon on the right of the nav bar edit:

1) tap navbar edit -> go button
2) loading a page -> stop button
3) page loaded, not editing -> reload button

we should never display more than one icon in that place as well (it's actually three buttons that get shown/hidden at the right time.)

Comment 11

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130826074752
Built from http://hg.mozilla.org/mozilla-central/rev/14b1e8c2957e

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment10 and got expected result. I saw all buttons when required and didn't see any extra buttons.

Comment 12

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #13 testing without any issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-09-05-03-02-06-mozilla-central/

- Went through the test cases added in comment # 10 without any issues
- Tapped on the URL bar and ensured that the "->" button appeared and worked without any issues
- Ensured that when a website is initially loading, the "x" button appears and works without any issues
- Ensured when the website finishes loading, the "refresh" button appears and works without any issues
- Ensured that when the website is being refreshed, the "x" button appears and works without any issues
- Ensured that all of the above test cases also worked in filled view without issues

Comment 13

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #16 testing without issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-10-23-03-02-05-mozilla-central/

- Went through the test case(s) added in comment # 10 without any issues
- Went through the test case(s) added in comment # 12 without any issues