Device selection menu doesn't remember custom devices with some unicode characters

RESOLVED FIXED in Firefox 58

Status

()

Firefox
Developer Tools: Responsive Design Mode
P2
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: Mikhail E Krasilnikiov, Assigned: Abhinav Koppula, Mentored)

Tracking

({good-first-bug})

54 Branch
Firefox 58
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox57 fix-optional, firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

5 months ago
Created attachment 8884876 [details]
Menu states after steps 3 and 6.

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

Steps to reproduce:

1. Enter responsive design mode (RDM).
2. Open device menu.
3. Deselect all predefined devices.
3. Add custom device with name «1. Смартфон (320×480)».
4. Exit RDM.
5. Enter RDM again.
6. Open device menu.


Actual results:

Menu contains default set of predefined devices and does not contain custom device.


Expected results:

Menu should contain only custom device.

Updated

5 months ago
Component: Untriaged → Developer Tools: Responsive Design Mode
Thanks for the report and sorry for the delay.  I am able to reproduce the issue.
status-firefox57: --- → fix-optional
Priority: -- → P2
Status: UNCONFIRMED → NEW
Ever confirmed: true
I believe this can be fixed by changing http://searchfox.org/mozilla-central/source/devtools/client/responsive.html/actions/devices.js to use getStringPref / setStringPref instead of getCharPref / setCharPref.
Mentor: jryans@gmail.com
Keywords: good-first-bug
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 months ago
Hi Ryan,
I have created a quick mozreview-request for this issue.
I was able to test this issue with my patch applied and it does fix the above mentioned issue.

Thanks,
Abhinav
Comment on attachment 8910877 [details]
Bug 1379687 - Fix for device selection menu not remembering custom devices with unicode characters.

https://reviewboard.mozilla.org/r/182358/#review188030

Thanks for submitting this patch! :) This is exactly the change I had in mind.

I think we should also add some test coverage for this.  I think something like the following should work:

Using the existing custom device test[1], you can borrow from the first task there (adding more task(s) in the same file).  Since we know from the bug that we need to create device with a Unicode name, close and reopen RDM, and then check for the device in the menu, I think two new tasks in the test makes sense (RDM is closed and opened for each `addRDMTask` call):

* Task 1 would add the new device with Unicode name
* Task 2 would verify that it appears in the device selector

If you have any trouble with this, please ask me in the bug and set the needinfo? flag at the bottom to mentor.

[1]: http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/devtools/client/responsive.html/test/browser/browser_device_custom.js#22-67

::: devtools/client/responsive.html/actions/devices.js:39
(Diff revision 1)
>      "removed": new Set(),
>    };
>  
>    if (Services.prefs.prefHasUserValue(DISPLAYED_DEVICES_PREF)) {
>      try {
> -      let savedData = Services.prefs.getCharPref(DISPLAYED_DEVICES_PREF);
> +      let savedData = Services.prefs.getStringPref(DISPLAYED_DEVICES_PREF);

At first, I thought we might also need to add a migration step for people who already have a value saved the old way (`setCharPref`), but it looks like it upgrades transparently: existing data from `setCharPref` can be read by `getStringPref` just fine.  (The data will still have some nonsense characters in place of Unicode until it has been saved again by the new `setStringPref`.)
Attachment #8910877 - Flags: review?(jryans) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 months ago
mozreview-review-reply
Comment on attachment 8910877 [details]
Bug 1379687 - Fix for device selection menu not remembering custom devices with unicode characters.

https://reviewboard.mozilla.org/r/182358/#review188030

Hi Ryan,
Thanks for the review. I have enhanced the mentioned test to add two new tasks which checks for unicode character deviceNames.
Please let me know if this is fine.
Comment on attachment 8910877 [details]
Bug 1379687 - Fix for device selection menu not remembering custom devices with unicode characters.

https://reviewboard.mozilla.org/r/182358/#review188440

Thanks, this looks great overall!  Just a few tweaks left for the test, and then it should be ready to land.

Do you have try server access to run the tests on Mozilla CI?  If not, I can send the next patch to try for you.

::: devtools/client/responsive.html/test/browser/browser_device_custom.js:140
(Diff revision 2)
> +  let deviceSelector = document.querySelector(".viewport-device-selector");
> +  let submitButton = document.querySelector("#device-submit-button");
> +
> +  openDeviceModal(ui);
> +
> +  // uncheck all the already checked devices

Nit: Let's start the comment with capital letter to match the style here.

::: devtools/client/responsive.html/test/browser/browser_device_custom.js:144
(Diff revision 2)
> +
> +  // uncheck all the already checked devices
> +  let checkedDevices = document.querySelectorAll(".device-input-checkbox");
> +  checkedDevices.forEach(function (label) {
> +    if (label.checked) {
> +      label.click();

Try using `Simulate.click(label)` here instead.  This tends to avoid intermittent test issues with React.

If that doesn't work for some reason, it's fine to use this instead.

::: devtools/client/responsive.html/test/browser/browser_device_custom.js:151
(Diff revision 2)
> +  });
> +
> +  info("Reveal device adder form, check that defaults match the viewport");
> +  let adderShow = document.querySelector("#device-adder-show");
> +  Simulate.click(adderShow);
> +  testDeviceAdder(ui, {

Let's remove this `testDeviceAdder` block here, since it's already checked by the previous task.

::: devtools/client/responsive.html/test/browser/browser_device_custom.js:180
(Diff revision 2)
> +  info("Look for custom unicode device in device selector");
> +  let selectorOption = [...deviceSelector.options].find(opt =>
> +    opt.value == unicodeDevice.name);
> +  ok(selectorOption, "Custom unicode device option added to device selector");
> +
> +  let select = document.querySelector(".viewport-device-selector");

We can probably stop just before this (without actually selecting the device) since we don't test anything after that.  It should be sufficient to verify that it has appeared in the device selector (which the previous line has done).

::: devtools/client/responsive.html/test/browser/browser_device_custom.js:185
(Diff revision 2)
> +  let select = document.querySelector(".viewport-device-selector");
> +  select.value = unicodeDevice.name;
> +  Simulate.change(select);
> +
> +  // exit RDM by selecting the global exit button
> +  Simulate.click(document.querySelector("#global-exit-button"));

It should be fine to remove this part of clicking the exit button manually: after the task block is done, `addRDMTask` will close RDM automatically.

::: devtools/client/responsive.html/test/browser/browser_device_custom.js:200
(Diff revision 2)
> +  yield waitUntilState(store, state => state.viewports.length == 1
> +    && state.devices.listState == Types.deviceListState.LOADED);
> +
> +  let deviceSelector = document.querySelector(".viewport-device-selector");
> +
> +  info("Look for custom unicode device in device selector");

Let's add a comment here to explain why we're checking the device selector again.  Something about how we want to ensure it is still present after restarting RDM.

::: devtools/client/responsive.html/test/browser/browser_device_custom.js:205
(Diff revision 2)
> +  info("Look for custom unicode device in device selector");
> +  let selectorOption = [...deviceSelector.options].find(opt =>
> +    opt.value == unicodeDevice.name);
> +  ok(selectorOption, "Custom unicode device option added to device selector");
> +
> +  let select = document.querySelector(".viewport-device-selector");

As above, I think we can drop the part about select the device and clicking exit.
Attachment #8910877 - Flags: review?(jryans) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 months ago
mozreview-review-reply
Comment on attachment 8910877 [details]
Bug 1379687 - Fix for device selection menu not remembering custom devices with unicode characters.

https://reviewboard.mozilla.org/r/182358/#review188440

Hi Ryan,
Thanks for the review comments. I have made the suggested changes.
No, I don't have access to try. Can you please push it to try for me?
Comment on attachment 8910877 [details]
Bug 1379687 - Fix for device selection menu not remembering custom devices with unicode characters.

https://reviewboard.mozilla.org/r/182358/#review188856

Thanks for working on this! :)

I'll push this to try and see what our test automation thinks.
Attachment #8910877 - Flags: review?(jryans) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6545e98de070
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
Looks like the last one didn't run anything, let's try again...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2593a71d936a5d5ace632aa1b4640633bf60ad3b
Well, that was quite surprising!  The test change seemed to leak windows on debug builds...  I didn't really have good advice to give, so I decided to investigate locally.

I was able to workaround that issue by ensuring we clone the device object before storing it.  It's a bit unclear to me why we're only hitting this now...

In any case, hopefully this run passes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46e04c3992419944b9adc74777a4e22b129cdaeb

Comment 15

2 months ago
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/af03af04a37d
Fix for device selection menu not remembering custom devices with unicode characters. r=jryans
https://hg.mozilla.org/integration/autoland/rev/fde231f9bb51
Clone device object on add to avoid debug leaks. r=me
Thanks again for your help Abhinav!

Please feel free to look around for other bugs to work on:

http://bugs.firefox-dev.tools/

Although, it looks like you are already aware of it! :)
Ah, feel free to request try access, it should help you double check future patches:

https://www.mozilla.org/en-US/about/governance/policies/commit/
(Assignee)

Comment 18

2 months ago
Thanks Ryan, yes devtools bugs are keeping me busy nowadays.
Will try to pick some more of responsive.html bugs.

Comment 19

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/af03af04a37d
https://hg.mozilla.org/mozilla-central/rev/fde231f9bb51
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(Reporter)

Comment 20

2 months ago
Guys you are awesome! Many thanks!
You need to log in before you can comment on or make changes to this bug.