1319597 - Clicking "DPR" text should open DPI menu

Status: RESOLVED FIXED

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

Description

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

Adapted from comment on Hacks post[1].

Clicking on the DPR text should open the DPR menu.  This will also help enlarge the hit box, for easier clicking.

[1]: https://hacks.mozilla.org/2016/11/new-responsive-design-mode-rdm-lands-in-firefox-dev-tools/#comment-20346

Comment 1

[Abhinav Koppula]

Hi Ryan,
I tried to work on this issue.
I tried simulating a click on the select element whenever label is clicked, but it turns out that there's no clean way of opening the select and showing the options through javascript(I may be wrong). 
We can do similar to "No Throttling" and have the first option (default selected) to be "DPR".

Your thoughts?

Comment 2

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

(In reply to Abhinav Koppula from comment #1)
> Hi Ryan,
> I tried to work on this issue.
> I tried simulating a click on the select element whenever label is clicked,
> but it turns out that there's no clean way of opening the select and showing
> the options through javascript(I may be wrong). 
> We can do similar to "No Throttling" and have the first option (default
> selected) to be "DPR".

I agree that it seems simplest to achieve this by adding the DPR into the menu.

There are separate bugs about the "default" not having its own menu option, so let's not worry about that part here.

For this bug, let's move the DPR text menu all the menu options, so they'd become "DPR: 1" etc.  (I think adding the colon is a bit more clear than it is currently.)

Comment 3

[Abhinav Koppula]

Attachment: Bug 1319597 - Move DPR text into the DPR menu options.

Review commit: https://reviewboard.mozilla.org/r/197254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197254/

Comment 4

[Abhinav Koppula]

Hi Ryan,
Thanks for the clarification. 
I have quickly put together a patch and have fixed the failing test(due to this change).
Can you check once?

Comment 5

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

Comment on attachment 8926018 [details]
Bug 1319597 - Move DPR text into the DPR menu options.

https://reviewboard.mozilla.org/r/197254/#review202956

Thanks for working on this! :) Overall, it's the right direction.  A few things to clean up, and we should be ready to go.

::: devtools/client/responsive.html/components/DprSelector.js:21
(Diff revision 1)
>  const PIXEL_RATIO_PRESET = [1, 2, 3];
>  
>  const createVisibleOption = value =>
>    dom.option({
>      value,
> -    title: value,
> +    title: `${DPR}: ${value}`,

Rather repeating this concantenation in 4 places, let's add a function to do it.  Also, I think it would be better to make this a localizable string, in case there are locales that would want to reorder things.

So, add a new string to responsive.properties, something like:

```
responsive.devicePixelRatioOption=DPR: %1$S
```

(with an appropriate comment for localizers).

Then over here, we can add something like:

```
const labelForOption = value =>
  getFormatStr("responsive.devicePixelRatioOption", value);
```

::: devtools/client/responsive.html/components/DprSelector.js:112
(Diff revision 1)
>  
>      if (state == Types.deviceListState.LOADED) {
>        listContent = listContent.concat(hiddenOptions.map(createHiddenOption));
>      }
>  
> -    return dom.label(
> +    return dom.select(

I think it makes sense to remove the `label` wrapping element like you've done here, but it means there is also some CSS we need to update.

All of the styles[1] like `#global-dpr-selector > select` will need some adjustment, since the `select` is no longer a child element.

[1]: http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/devtools/client/responsive.html/index.css#179-212

Comment 6

[Abhinav Koppula]

Comment on attachment 8926018 [details]
Bug 1319597 - Move DPR text into the DPR menu options.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197254/diff/1-2/

Comment 7

[Abhinav Koppula]

Hi Ryan,
Thanks for the review.
I have updated my review-request.

Comment 8

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

Comment on attachment 8926018 [details]
Bug 1319597 - Move DPR text into the DPR menu options.

https://reviewboard.mozilla.org/r/197254/#review204192

Okay, we're very close now! :) Thanks for your continued work.  Just a few tweaks left, and we should ready to land with the next round.

::: devtools/client/responsive.html/components/DprSelector.js:19
(Diff revision 2)
> +const labelForOption = value => getFormatStr("responsive.devicePixelRatioOption", value);
>  
>  const PIXEL_RATIO_PRESET = [1, 2, 3];
>  
>  const createVisibleOption = value =>
>    dom.option({

l10n APIs can be slow, so let's avoid computing the same label twice.

Add something like `let label = labelForOption(...)` and then use that in both places.

::: devtools/client/responsive.html/index.css
(Diff revision 2)
>  }
>  
> -#global-dpr-selector > select {
> -  padding: 0 8px 0 0;
> -  margin-left: 2px;
> -  max-width: 5em;

This `max-width` is here to keep the UI compact if the DPR changes to a repeating decimal value.  This can happen if you try to zoom the UI (Cmd + Plus / Minus on macOS for example).

We should preserve this effect, but it needs some tweaks.  Fold it into `#global-dpr-selector` and let's use `8em` (since it now has to fit "DPR: " as well).

A comment above the property about why we have this would be a nice bonus for the next person (I should have added one the first time). :)

::: devtools/client/responsive.html/index.css:180
(Diff revision 2)
> -  margin-left: 2px;
> -  max-width: 5em;
> -}
> -
>  #global-dpr-selector {
>    margin: 0 8px;

Spacing between things in the global toolbar has always been a bit chaotic...  I think we need a few tweaks to it preserve roughly the same appearance after this markup change.

I would suggest:

1. Remove the `margin` property here on the DPR selector
2. Add the following new rule:

```
#global-toolbar > .toolbar-button:first-of-type {
  margin-inline-start: 8px;
}
```

near the existing `#global-toolbar > .toolbar-button` rules.

Comment 9

[Abhinav Koppula]

Comment on attachment 8926018 [details]
Bug 1319597 - Move DPR text into the DPR menu options.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197254/diff/2-3/

Comment 10

[Abhinav Koppula]

Comment on attachment 8926018 [details]
Bug 1319597 - Move DPR text into the DPR menu options.

https://reviewboard.mozilla.org/r/197254/#review204192

Hi Ryan,
Thanks for the detailed review. I have addressed the comments.

Comment 11

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

Comment on attachment 8926018 [details]
Bug 1319597 - Move DPR text into the DPR menu options.

https://reviewboard.mozilla.org/r/197254/#review204980

Great, this looks ready to land to me! :)

Please ensure all the RDM tests are passing.  Do you have try access?  For a change such as this, I would think a local test run (`./mach test devtools/client/responsive.html`) is probably sufficient.

Comment 12

[Abhinav Koppula]

Comment on attachment 8926018 [details]
Bug 1319597 - Move DPR text into the DPR menu options.

https://reviewboard.mozilla.org/r/197254/#review204980

Yes, I have pushed to TRY
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8efd970712090d54bffdc828141d1a62e38feacc
I ran the responsive.html test-suite locally before pushing the patch and it looked fine. Let's wait for TRY to complete as well.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0bd3937d98a
Move DPR text into the DPR menu options. r=jryans

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/e0bd3937d98a

Comment 16

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 53.0a1 (2016-11-22) on Ubuntu 12.04 (64 Bit) LTS!
This bug's fix is Verified with latest Nightly !

Build   ID    20171120222519
User Agent    Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0

Comment 17

[Sajedul Islam]

I have reproduced this bug with Nightly 53.0a1 (2016-11-22) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build   ID    20171121100129
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:59.0) Gecko/20100101 Firefox/59.0

[bugday-20171115]