Closed Bug 1254390 Opened 6 years ago Closed 5 years ago

Flexible sizing for device selector

Categories

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

defect

Tracking

(firefox48 affected, firefox50 verified)

VERIFIED FIXED
Firefox 50
Iteration:
50.2 - Jul 4
Tracking Status
firefox48 --- affected
firefox50 --- verified

People

(Reporter: jryans, Assigned: jbhoosreddy)

References

Details

(Whiteboard: [multiviewport])

Attachments

(3 files, 4 obsolete files)

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?
Flags: qe-verify+
Let's ask :helenvholmes.
Flags: needinfo?(hholmes)
Priority: -- → P3
Whiteboard: [multiviewport][triage] → [multiviewport]
Is this referring to the <select> box at the top?

If so, yes, it probably needs a flexible size.
Flags: needinfo?(hholmes)
Attached image 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).
Assignee: nobody → jaideepb
Status: NEW → ASSIGNED
Attached patch 1254390.patch (obsolete) — Splinter Review
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.
Attachment #8760508 - Flags: review?(jryans)
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.
Attachment #8760508 - Flags: ui-review?(hholmes)
Attachment #8760508 - Flags: review?(jryans)
Attachment #8760508 - Flags: review?(gl)
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.
Attachment #8760508 - Flags: ui-review?(hholmes) → feedback+
Attached patch 1254390.patch [1.0] (obsolete) — Splinter Review
Attachment #8760508 - Attachment is obsolete: true
Attachment #8760508 - Flags: review?(gl)
Attachment #8763683 - Flags: review?(hholmes)
Attached patch 1254390.patch [1.0] (obsolete) — Splinter Review
Minor update from previous patch: The icon was still a tad misaligned and I missed it, so I'm submitting a modified patch.
Attachment #8763683 - Attachment is obsolete: true
Attachment #8763683 - Flags: review?(hholmes)
Attachment #8763689 - Flags: review?(hholmes)
Attached video 1254390.mov
Attachment #8763689 - Flags: ui-review?(hholmes)
Attachment #8763689 - Flags: review?(hholmes)
Attachment #8763689 - Flags: review?(gl)
Comment on attachment 8763689 [details] [diff] [review]
1254390.patch [1.0]

Looks good to me!
Attachment #8763689 - Flags: ui-review?(hholmes) → ui-review+
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
Attachment #8763689 - Flags: review?(gl) → review+
Attached patch 1254390.patch [2.0] (obsolete) — Splinter Review
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
Attachment #8763689 - Attachment is obsolete: true
Attachment #8766023 - Flags: review+
Keywords: checkin-needed
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
Flags: needinfo?(jaideepb)
Keywords: checkin-needed
Re-uploading patch to solve merge issues.
Attachment #8766023 - Attachment is obsolete: true
Attachment #8766405 - Flags: ui-review+
Attachment #8766405 - Flags: review+
Flags: needinfo?(jaideepb)
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/31e5df99112e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.2 - Jul 4
Priority: P3 → P1
QA Contact: mihai.boldan
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-rdm]
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.