1254390 - Flexible sizing for device selector

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Bug 1241714 adds a device selector.  It has a fixed width in the viewport toolbar currently.

Should it have a flexible size to show longer device names more easily?

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Let's ask :helenvholmes.

Comment 2

[Helen V. Holmes (:helenvholmes) (:✨)]

Is this referring to the <select> box at the top?

If so, yes, it probably needs a flexible size.

Comment 3

[Francesco Lodolo [:flod]]

Attachment: Italian screenshot

Yes please. Also note how much available space there is, and how the current label is unnecessarily truncated in Italian. 

Several defaults are also cut (e.g. Alcatel One Touch phones).

Comment 4

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1254390.patch

In this patch, I've tried to use width: -moz-fit-content to assign width according to the content width inside. Apart from that I've changed the background image position from absolute values to percentages. Also, tried to align it is it seemed a tad misaligned (vertically) to the top.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8760508 [details] [diff] [review]
1254390.patch

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

:gl, can you review this one?

Also we should ask :helen for ui-review on this one.

Comment 7

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8760508 [details] [diff] [review]
1254390.patch

It's looking reasonable to me, but I think it would be a nice touch to add the  name to a label to prevent it from being cut off in any scenario, such as we do on the UI buttons.

Comment 8

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1254390.patch [1.0]

Comment 9

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1254390.patch [1.0]

Minor update from previous patch: The icon was still a tad misaligned and I missed it, so I'm submitting a modified patch.

Comment 10

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1254390.mov

Comment 11

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8763689 [details] [diff] [review]
1254390.patch [1.0]

Looks good to me!

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8763689 [details] [diff] [review]
1254390.patch [1.0]

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

::: devtools/client/responsive.html/components/device-selector.js
@@ +76,5 @@
>      return dom.select(
>        {
>          className: selectClass,
>          value: selectedDevice,
> +        title: selectedDevice,

I am not quite sure if we really need the title attribute here or just don't understand the need of it.

::: devtools/client/responsive.html/index.css
@@ +179,2 @@
>    text-align: center;
> +  width: fit-content;

I think we should only keep -moz-fit-content

Comment 13

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1254390.patch [2.0]

The title attribute is used to display hover tooltip label with device name as per the suggestion Helen made later in the bug.
https://bugzilla.mozilla.org/show_bug.cgi?id=1254390#c7

Comment 14

[Carsten Book [:Tomcat]]

has problems to apply:

patching file devtools/client/responsive.html/components/device-selector.js
Hunk #1 FAILED at 71
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/responsive.html/components/device-selector.js.rej
patching file devtools/client/responsive.html/index.css
Hunk #1 FAILED at 171
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/responsive.html/index.css.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1254390.patch

Comment 15

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1254390.patch [2.0]

Re-uploading patch to solve merge issues.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/31e5df99112e
Flexible sizing for device selector to display full device names; r=gl

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31e5df99112e

Comment 18

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue on Firefox 50.0a1 (2016-06-20), localized in IT language, under Windows 10 x64.
The issue is no longer reproducible on Firefox 50.0a1(2016-07-12).
The tests were performed on Windows 10 x64, Mac OS X 10.11.1 and on Ubuntu 14.04 x64.
Also, the bug was verified on localized builds: IT, DE, RU, TR, JA, FR and zh-CN.
I was not able to test this issue on a few localized builds (Eg. PT, Vi), because the strings under test are not localized.
Please note that I logged a new issue, that is a regression of this fix - Bug 1284463.
I am marking this issue Verified Fixed.