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

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

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.

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

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/urlbar-stop@1.8x.png); > } > } > > +/* 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 on attachment 787746 [details] [diff] [review] patch v.1 Review of attachment 787746 [details] [diff] [review]: ----------------------------------------------------------------- r=jwilde with the changes described earlier.

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

Can you please provide steps to test this?

(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.)

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.

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

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