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?

Let's ask :helenvholmes.

Is this referring to the <select> box at the top? If so, yes, it probably needs a flexible size.

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

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

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

Comment on attachment 8763689 [details] [diff] [review] 1254390.patch [1.0] Looks good to me!

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

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

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

Re-uploading patch to solve merge issues.

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

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.