Closed Bug 1180734 Opened 4 years ago Closed 4 years ago

Visually tidy the device name field

Categories

(Firefox :: Sync, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
42.2 - Jul 27
Tracking Status
firefox42 --- verified

People

(Reporter: rfeeley, Assigned: eoger)

References

Details

(Whiteboard: [fxsync])

Attachments

(2 files, 4 obsolete files)

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.
Waiting on rfeely approval!
Flags: needinfo?(rfeeley)
Attached patch bug-1180734.patch (obsolete) — Splinter Review
Patch used for above screenshots
Fantastic!
Flags: needinfo?(rfeeley)
Attachment #8630814 - Flags: review?(markh)
Priority: -- → P2
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
Attachment #8630814 - Flags: review?(markh) → review-
(In reply to Mark Hammond [:markh] from comment #4)
> can we get fancy with plain html?

Obviously I meant XUL :)
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?
Flags: needinfo?(rfeeley)
Blocks: 1182288
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).
Flags: needinfo?(rfeeley)
Rank: 15
Attached patch bug-1180734.patch (obsolete) — Splinter Review
BL gone :)
Attachment #8630814 - Attachment is obsolete: true
Attachment #8631851 - Flags: review?(markh)
(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 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"
Attachment #8631851 - Flags: review?(markh) → review+
Attached patch bug-1180734.patch (obsolete) — Splinter Review
Attachment #8631851 - Attachment is obsolete: true
Attachment #8631944 - Flags: review+
Attachment #8631944 - Flags: checkin+
Keywords: checkin-needed
Please run this through Try to make sure there are no test failures lurking :)
Keywords: checkin-needed
Mark, I think you are right. I don't think the 
… clear icon is required. Ha, the special character truncated my comment! Now a bug itself: https://bugzilla.mozilla.org/show_bug.cgi?id=1183171
Attached patch bug-1180734.patch (obsolete) — Splinter Review
Removed the search-textbox XBL binding. We are now a simple textbox again. Trypush incoming.
Attachment #8631944 - Attachment is obsolete: true
Attachment #8632986 - Flags: review?(markh)
Try looks ok!
Comment on attachment 8632986 [details] [diff] [review]
bug-1180734.patch

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

Awesome
Attachment #8632986 - Flags: review?(markh)
Attachment #8632986 - Flags: review+
Attachment #8632986 - Flags: checkin+
Keywords: checkin-needed
Needs rebasing against fx-team tip.
Keywords: checkin-needed
Thank you, here's the rebased patch!
Attachment #8632986 - Attachment is obsolete: true
Attachment #8633821 - Flags: review+
Keywords: checkin-needed
Blocks: 1184243
https://hg.mozilla.org/mozilla-central/rev/75249b446ea9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
The vbox inside the groupbox is not needed and can be removed.
Good catch Alfred, I will bundle it in bug 1184639.
Whiteboard: [fxsync]
Iteration: --- → 42.1 - Jul 13
Iteration: 42.1 - Jul 13 → 42.2 - Jul 27
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]
(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!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.