Multiple Select request not shown properly in DevTools

VERIFIED FIXED in Firefox 53

Status

P1
normal
VERIFIED FIXED
2 years ago
6 months ago

People

(Reporter: denys, Assigned: gasolin)

Tracking

(Blocks: 1 bug, {regression})

53 Branch
Firefox 54
x86_64
Linux
regression
Dependency tree / graph

Firefox Tracking Flags

(firefox51 unaffected, firefox52 unaffected, firefox53+ verified, firefox54+ verified)

Details

(Whiteboard: [netmonitor])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170204084039

Steps to reproduce:

Minimal HTML to reproduce the issue:
<form method="post" action="http://requestb.in/sgxy1zsg">
    <select id="m1" name="multiple1[]" multiple>
      <option value="1">1</option>
      <option value="2">2</option>
      <option value="3">3</option>
      <option value="4">4</option>
      <option value="5">5</option>
    </select>


    <button type="submit">Submit</button>
  </form>

(Or visit https://denv.it/multiple-example.html)


Actual results:

multiple1[] when multiple entries are selected 

Selecting multiple elements like 1 and 3, then clicking submit sends to the server this:

FORM/POST PARAMETERS

multiple1[]: 1
multiple1[]: 3

In DevTools (under Network => (select the request) => Form Data) only mulitple1[]: 3 appears.
See http://i.imgur.com/ZA1WuOI.png for the result.


Expected results:

multiple1[]: 1
multiple1[]: 3

should appear for the request
(Reporter)

Updated

2 years ago
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
(Reporter)

Comment 1

2 years ago
53.0a2 (2017-02-04) (64-bit) (FF Dev Edition)
(Reporter)

Comment 2

2 years ago
This happens also when the POST data is like the following:
custom[0][name]:custom[invoice_custom_fattdesc]
custom[0][value]:test
custom[1][name]:custom[invoice_custom_fatttype]
custom[1][value]:14

Comment 3

2 years ago

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8f3b24109e3412b36f97277e31ad66856dc609d6&tochange=acd4177b28d7862528f63edb40e3d5a5f0b7c8b3

Ricky Chien — Bug 1317650 - Implement Params Panel r=Honza,jsnajdr
Blocks: 1317650
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
status-firefox53: --- → affected
status-firefox54: --- → affected
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
Component: Untriaged → Developer Tools: Netmonitor
Ever confirmed: true
Flags: needinfo?(rchien)
Keywords: regression
Priority: -- → P2
Blocks: 1307743
Flags: qe-verify?
Priority: P2 → --
Whiteboard: [netmonitor] [triage]
(Assignee)

Comment 4

2 years ago
I can reproduce it
Assignee: nobody → gasolin
Flags: needinfo?(rchien)
(Assignee)

Comment 5

2 years ago
The reason is there are two `multiple1[]` params exist with different values.
We use object to render the treeview, which means after we assigned `multiple1[]` as a key with first value, the first value is overwritten by second value when we met `multiple1[]` as a key again.

```
multiple1[]: 3 (overwrite 1)
```

Because of dup key, it also means the previous UI is not possible with current implementation

```
multiple1[]: 1
multiple1[]: 3
```


I try to construct these same name params as a list so it could be rendered by rep list

```
multiple1[]: [2]
  0: 1
  1: 3
```

I think it looks not ideal.
Do you think it looks better if we put the params value at the same line?

```
multiple1[]: 1, 3
```

or 

```

multiple1[]: [1, 3]
```

`[1, 3]` is a string rendered by rep string
Flags: needinfo?(odvarko)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
The PR implemented 

```
multiple1[]: [2]
  0: 1
  1: 3
```

for reference
(Reporter)

Comment 8

2 years ago
In my opinion, the way it was implemented in your PR is the way to go.

A comma separated string wouldn't be easy to understand since there are many libraries that send a comma concatenated string instead of an array, thus that may create confusion.
Comment on attachment 8834800 [details]
Bug 1337015 - show multiple select params correctly;

https://reviewboard.mozilla.org/r/110636/#review111938

Thanks for quick patch Fred!

I like the way how it's presented in the UI now.

R+ assuming try is green.

Honza

::: devtools/client/netmonitor/shared/components/params-panel.js:66
(Diff revision 1)
>        parseQueryString(query)
>          .reduce((acc, { name, value }) =>
>            name ? Object.assign(acc, { [name]: value }) : acc
>          , {});
>    }
>    // Form Data section

nit: please add new empty line above the // Form Data section comment.

::: devtools/client/netmonitor/shared/components/params-panel.js:77
(Diff revision 1)
> -          name ? Object.assign(acc, { [name]: value }) : acc
> -        , {});
> +          let val = map[obj.name];
> +          // Deal with dup key case (ex: multiple selection)
> +          if (val) {
> +            if (typeof val !== 'object') {
> +              map[obj.name] = [val];
> +              console.log('conv to list', map[obj.name]);

Remove forgotten console.log
Attachment #8834800 - Flags: review+
Just clearing the NI
Honza
Flags: needinfo?(odvarko)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8834800 [details]
Bug 1337015 - show multiple select params correctly;

https://reviewboard.mozilla.org/r/110636/#review111954

::: devtools/client/netmonitor/shared/components/params-panel.js:72
(Diff revision 1)
>    if (formDataSections && formDataSections.length > 0) {
>      let sections = formDataSections.filter((str) => /\S/.test(str)).join("&");
>      object[PARAMS_FORM_DATA] =
>        parseQueryString(sections)
> -        .reduce((acc, { name, value }) =>
> -          name ? Object.assign(acc, { [name]: value }) : acc
> +        .reduce((map, obj) => {
> +          let val = map[obj.name];

nit: val -> value

::: devtools/client/netmonitor/shared/components/params-panel.js:73
(Diff revision 1)
>      let sections = formDataSections.filter((str) => /\S/.test(str)).join("&");
>      object[PARAMS_FORM_DATA] =
>        parseQueryString(sections)
> -        .reduce((acc, { name, value }) =>
> -          name ? Object.assign(acc, { [name]: value }) : acc
> -        , {});
> +        .reduce((map, obj) => {
> +          let val = map[obj.name];
> +          // Deal with dup key case (ex: multiple selection)

nit: dup -> duplicate
Status: NEW → ASSIGNED
Iteration: --- → 54.2 - Feb 20
Priority: -- → P1
Whiteboard: [netmonitor] [triage] → [netmonitor]
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
Thank you Denys, your instruction and examples are very clear!
(Assignee)

Comment 14

2 years ago
tree green, thanks
Keywords: checkin-needed

Comment 15

2 years ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7ea2fe0db27
show multiple select params correctly;r=Honza
Keywords: checkin-needed

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7ea2fe0db27
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Tracking this user facing regression for 53/54+.
tracking-firefox53: ? → +
tracking-firefox54: ? → +
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(gasolin)
(Assignee)

Comment 19

2 years ago
Comment on attachment 8834800 [details]
Bug 1337015 - show multiple select params correctly;

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1317650
[User impact if declined]: The multiple selection params will be shown incorrectly
[Is this code covered by automated tests?]: N
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, follow the bug report 
[List of other uplifts needed for the feature/fix]: N
[Is the change risky?]: N
[Why is the change risky/not risky?]: The change only affect multiple selection case
[String changes made/needed]: N
Flags: needinfo?(gasolin)
Attachment #8834800 - Flags: approval-mozilla-aurora?
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu
Hi Florin,
could you help find someone to verify if this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(florin.mezei)
Ciprian works on netmonitor bugs. Moving this to him.
Flags: needinfo?(florin.mezei) → needinfo?(ciprian.georgiu)
Comment on attachment 8834800 [details]
Bug 1337015 - show multiple select params correctly;

Let's uplift, we can verify it on 53 aurora.
Attachment #8834800 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 23

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/232dd77a7c38
status-firefox53: affected → fixed
Reproduced this issue following str from comment 0, using Nightly 54.0a1 (2017-02-06).

This is verified fixed on latest Aurora 53.0a2 (2017-02-20) and latest Nightly 54.0a1 (2017-02-20) across platforms:
- Windows 10 x64
- Ubuntu 16.04 x64 LTS
- Mac OS X 10.11.6
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
status-firefox54: fixed → verified
Flags: qe-verify+
Flags: needinfo?(ciprian.georgiu)

Updated

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