Flexible sizing for device selector

VERIFIED FIXED in Firefox 50

Status

P1
normal
VERIFIED FIXED
3 years ago
4 months ago

People

(Reporter: jryans, Assigned: jbhoosreddy)

Tracking

Trunk
Firefox 50
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox50 verified)

Details

(Whiteboard: [multiviewport])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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+
(Reporter)

Comment 1

3 years ago
Let's ask :helenvholmes.
Flags: needinfo?(hholmes)

Updated

3 years ago
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)
Created attachment 8733936 [details]
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).
(Reporter)

Updated

2 years ago
Assignee: nobody → jaideepb

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
Created attachment 8760508 [details] [diff] [review]
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.
Attachment #8760508 - Flags: review?(jryans)
(Reporter)

Comment 5

2 years ago
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)
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1277382
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+
(Assignee)

Comment 8

2 years ago
Created attachment 8763683 [details] [diff] [review]
1254390.patch [1.0]
Attachment #8760508 - Attachment is obsolete: true
Attachment #8760508 - Flags: review?(gl)
Attachment #8763683 - Flags: review?(hholmes)
(Assignee)

Comment 9

2 years ago
Created attachment 8763689 [details] [diff] [review]
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.
Attachment #8763683 - Attachment is obsolete: true
Attachment #8763683 - Flags: review?(hholmes)
Attachment #8763689 - Flags: review?(hholmes)
(Assignee)

Comment 10

2 years ago
Created attachment 8765506 [details]
1254390.mov
(Assignee)

Updated

2 years ago
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+
(Assignee)

Comment 13

2 years ago
Created attachment 8766023 [details] [diff] [review]
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
Attachment #8763689 - Attachment is obsolete: true
Attachment #8766023 - Flags: review+
(Assignee)

Updated

2 years ago
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
(Assignee)

Comment 15

2 years ago
Created attachment 8766405 [details] [diff] [review]
1254390.patch [2.0]

Re-uploading patch to solve merge issues.
Attachment #8766023 - Attachment is obsolete: true
Attachment #8766405 - Flags: ui-review+
Attachment #8766405 - Flags: review+
(Assignee)

Updated

2 years ago
Flags: needinfo?(jaideepb)
Keywords: checkin-needed

Comment 16

2 years ago
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

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/31e5df99112e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Updated

2 years ago
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]
status-firefox50: fixed → verified
Flags: qe-verify+

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.