Closed Bug 902713 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 26

People

(Reporter: ally, Assigned: jimm)

References

Details

(Whiteboard: feature=defect c=tbd u=tbd p=1)

Attachments

(1 file)

and has caused some problems (such as beginEditing() being called but not endEditing() see bug 894811).
OS: Windows 8 → Windows 8 Metro
Assignee: nobody → jmathies
Blocks: metrov1it12
Priority: -- → P2
QA Contact: jbecerra
Summary: "mode" in urlbar is overloaded, split editing & loading apart → Defect - "mode" in urlbar is overloaded, split editing & loading apart
Whiteboard: feature=defect c=tbd u=tbd p=1
Status: NEW → ASSIGNED
Blocks: 851130
Attached patch patch v.1Splinter Review
Attachment #787746 - Flags: review?(jwilde)
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.
Attachment #787746 - Flags: review?(jwilde) → review+
https://hg.mozilla.org/mozilla-central/rev/48879353a32c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Can you please provide steps to test this?
Flags: needinfo?(ally)
Flags: needinfo?(jmathies)
(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.)
Flags: needinfo?(jmathies)
Flags: needinfo?(ally)
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.
Status: RESOLVED → VERIFIED
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
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.