1180734 - Visually tidy the device name field

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P2)

Description

[Ryan Feeley [:rfeeley]]

The fantastic new device name field looks quite cramped up against the sync datatype selections, and is missing the clear (x) button to the right.

It looks as though the XUL is missing a groupbox or at least hbox, which may work out the spacing issues, not requiring any style code changes.

Comment 1

[Edouard Oger [:eoger]]

Attachment: tidy_sync_screenshots.zip

Waiting on rfeely approval!

Comment 2

[Edouard Oger [:eoger]]

Attachment: bug-1180734.patch

Patch used for above screenshots

Comment 3

[Ryan Feeley [:rfeeley]]

Fantastic!

Comment 4

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8630814 [details] [diff] [review]
bug-1180734.patch

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

I'm not convinced we should be using xbl here - can we get fancy with plain html? If we did want to use xbl, I think we'd want the binding to be more general purpose (and thus, possibly inside textbox.xml) but that should probably be our worst-case scenario.

::: browser/components/preferences/in-content/sync.xml
@@ +6,5 @@
> +<!DOCTYPE bindings>
> +<bindings xmlns="http://www.mozilla.org/xbl"
> +          xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +          xmlns:xbl="http://www.mozilla.org/xbl">
> +  <binding id="devicename-textbox" extends="chrome://global/content/bindings/textbox.xml#search-textbox">

I don't think we want to reuse the search-textbox - I assume you did just to get the image, but it comes with other baggage.

@@ +15,5 @@
> +    ]]>
> +    </handler>
> +    <handler event="keypress" keycode="VK_ESCAPE">
> +    <![CDATA[
> +      document.getElementById("fxaCancelChangeDeviceName").click();

these would prevent the binding being used anywhere else, which isn't really the idea of xbl

Comment 5

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #4)
> can we get fancy with plain html?

Obviously I meant XUL :)

Comment 6

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Hey Ryan - what makes this textbox special in terms of "the clear (x) button"? TBH, first time I saw it I thought it might be "cancel" and I'm wondering why if it's worthwhile here that we aren't doing for all similar textboxes in Preferences?

Comment 7

[Ryan Feeley [:rfeeley]]

Quite the contrary, it's used on about:preferences#applications so I copied it from there. I just want to follow whatever is standard (which is still up for debate in prefs I think).

Comment 8

[Edouard Oger [:eoger]]

Attachment: bug-1180734.patch

BL gone :)

Comment 9

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Ryan Feeley from comment #7)
> Quite the contrary, it's used on about:preferences#applications so I copied
> it from there. I just want to follow whatever is standard (which is still up
> for debate in prefs I think).

I think about:preferences#applications is a bit of a special case - it's a field used to filter (ie, it's a "search field") - so the "x" is a way of saying "clear the filter". It also has the "search" icon which helps reinforce that.

Note however that on the "General" tab, the field for your homepage is a plain-old textbox like our device name one, and this *does not* have that affordance. I strongly suspect that if additional text fields were added they too wouldn't get it. However, if we added another textfield used to filter a list it would.

I'm yet to look at eoger's patch and I won't flag this as a specific problem (ie, I'm not going to object to this big of candy), but I think my point still remains - if the "x" makes sense here it makes sense on the "home page" field for general. Conversely, if it doesn't make sense there it also doesn't make sense here.

Comment 10

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8631851 [details] [diff] [review]
bug-1180734.patch

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

LGTM with the requested changes.

::: browser/components/preferences/in-content/sync.js
@@ +139,5 @@
>    },
>  
> +  _focusComputerNameTextbox: function() {
> +    let textbox = document.getElementById("fxaSyncComputerName");
> +    textbox.focus();

We should probably use textbox.setSelectionRange() here (I remain skeptical this textbox is so special it needs this special behaviour other textboxes don't get, but whatever...)

@@ +266,5 @@
> +    setEventListener("fxaSyncComputerName", "keypress", function (e) {
> +      if (e.keyCode == KeyEvent.DOM_VK_RETURN) {
> +        e.preventDefault();
> +        document.getElementById("fxaSaveChangeDeviceName").click();
> +      }

Please "cuddle" the else here (ie, "} else if ... {")

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +385,5 @@
>   */
>  
> +#fxaSyncComputerName {
> +  margin-left: 0px;
> +  -moz-binding: url("chrome://global/content/bindings/textbox.xml#search-textbox");

let's add a comment here indicating why we are using a searchbox for something that's not a searchbox :) eg, something simple like "we use a searchbox so we get a trailing "x" that clears the field because that what UX asked for"

@@ +393,5 @@
>    background-color: transparent;
>    opacity: 1;
>  }
> +
> +#fxaSyncComputerName.plain .textbox-search-icons {

and a trivial comment here saying "... but our box should *not* have the search icon"

Comment 11

[Edouard Oger [:eoger]]

Attachment: bug-1180734.patch

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please run this through Try to make sure there are no test failures lurking :)

Comment 13

[Ryan Feeley [:rfeeley]]

Mark, I think you are right. I don't think the 

Comment 14

[Ryan Feeley [:rfeeley]]

… clear icon is required. Ha, the special character truncated my comment! Now a bug itself: https://bugzilla.mozilla.org/show_bug.cgi?id=1183171

Comment 15

[Edouard Oger [:eoger]]

Attachment: bug-1180734.patch

Removed the search-textbox XBL binding. We are now a simple textbox again. Trypush incoming.

Comment 16

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad4533cf9e90

Comment 17

[Edouard Oger [:eoger]]

Try looks ok!

Comment 18

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8632986 [details] [diff] [review]
bug-1180734.patch

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

Awesome

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing against fx-team tip.

Comment 20

[Edouard Oger [:eoger]]

Attachment: bug-1180734.patch

Thank you, here's the rebased patch!

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/75249b446ea9

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/75249b446ea9

Comment 23

[Alfred Kayser]

The vbox inside the groupbox is not needed and can be removed.

Comment 24

[Edouard Oger [:eoger]]

Good catch Alfred, I will bundle it in bug 1184639.

Comment 25

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 42.0a1 (2015-07-06) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Beta 42.0b3!

Build ID: 20151001142456
User Agent: Mozilla/5.0 (X11; Linux i686; rv:42.0) Gecko/20100101 Firefox/42.0

[testday-20151002]

Comment 26

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Khalid Syfullah Zaman [:khalid32] from comment #25)
> I have reproduced this bug on Nightly 42.0a1 (2015-07-06) on ubuntu 14.04
> LTS, 32 bit!
> 
> The bug's fix is now verified on Latest Beta 42.0b3!

Thanks!