Closed Bug 1478361 Opened 2 years ago Closed 1 year ago

Removing Host value in "Edit and resend" input set the caret at the end of the input

Categories

(DevTools :: Netmonitor, defect, P3)

defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed, firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: nchevobbe, Assigned: glowka.tom, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, regression, Whiteboard: good-first-bug)

Attachments

(2 files)

**Steps to reproduce**
1. Go to data:text/html,<meta charset=utf8><script>fetch("https://httpstat.us/200")</script>
2. Open netmonitor
3. Reload to see the network entry
4. Select the httpstat.us entry, right click and select "Edit and resend"
5. In request header, on the "Host:" line, select the "httpstat.us" text
6. Hit the delete key

**Expected results**

The host is deleted and I can start typing a new one

**Actual results**

The host is deleted, but the cursor jumps to the end of the input. Usually you are already typing a few letters before you acknowledge the issue, which can be frustrating.

---

See the screencast attached
Attached video broken host editing
The bug was already here in Firefox 60
Thanks for the report!

I can reproduce that bug on my Win10 machine too.

Honza
Priority: -- → P3
jan, Ricky's bugzilla handle indicates that he is currently inactive, will you manage this regression? Thanks
Flags: needinfo?(odvarko)
I don't see anything particular in that regression window...

Here are some instructions for anyone interested in this bug.

1) The code that is responsible for updating the "Request Headers" text area is here:
https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/devtools/client/netmonitor/src/components/CustomRequestPanel.js#102-117

2) The `updateCustomRequestFields` method is called anytime something changes in the "New Request" form.

3) `updateRequest` action (UPDATE_REQUEST) is fired at the end of `updateCustomRequestFields`
https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/devtools/client/netmonitor/src/components/CustomRequestPanel.js#159

It updates `requestsReducer` reducer and triggers re-rendering

4) If I comment out the following code the cursor position is *not* changed.
This might help to find the real issue.

// Remove temp customHeadersValue while query string is parsable
/*if (customHeadersValue === "" ||
  headersArray.length === customHeadersValue.split("\n").length) {
  customHeadersValue = null;
}*/

5) Some more links:

The `updateRequest` action reducer handler is here:
https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/devtools/client/netmonitor/src/reducers/requests.js#80

It also calls `processNetworkUpdates` implemented here:
https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/devtools/client/netmonitor/src/utils/request-utils.js#494

The cycle is using `if (UPDATE_PROPS.includes(key)) {` and UPDATE_PROPS is defined here:
https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/devtools/client/netmonitor/src/constants.js#110

UPDATE_PROPS doesn't have `customHeadersValue`, which might be part of the issue.

-

6) You might also try the regression window in comment #4 to find the issue

Honza
Mentor: odvarko
Flags: needinfo?(odvarko)
Keywords: good-first-bug
Whiteboard: good-first-bug
Assign me please!
Flags: needinfo?(odvarko)
Assignee: nobody → glowka.tom
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Hey Honza,

(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> 4) If I comment out the following code the cursor position is *not* changed.
> This might help to find the real issue.
> 
> // Remove temp customHeadersValue while query string is parsable
> /*if (customHeadersValue === "" ||
>   headersArray.length === customHeadersValue.split("\n").length) {
>   customHeadersValue = null;
> }*/

So there are no re-renderings or faulty reducers, this line turns out to be the real culprit! There is not much more. Ok, maybe a little bit. 

What happens here is that we parse headers from the value of textarea using regex equivalent to /\S+?\:\s*(.+)/. If the teaxtarea's value is parseable we set the value prop of the textarea that way: headers.map(({name, value}) => name + ": " + value).join("\n"). This expression is not fully coherent with the regex, note that we don't require spaces between name and value of the header, while we always add one. When we add a space, the new value of textarea passed by prop does not match the previous one. Only value changes consistent with the textarea changes invoked by user do not reset the cursor (even ignoring the change and not setting value to event.target.value resets the cursor: https://github.com/facebook/react/issues/12762).

So my general conclusion is that we should either:
 - resign from space in `headers.map(({name, value}) => name + ": " + value).join("\n")`, so `": "` is replaced with `":"`
OR
 - set initial value using `headers.map(({name, value}) => name + ": " + value).join("\n")`, but later on only parse and save the value of textarea without overriding it with re-joined result of parsing (now we do not override only if we did not manage to parse it), removing those lines that set `customHeadersValue` to null does exactly that.

I would opt for second option, as it both slightly simplifies the flow and uses the nicer formatting with space, at least in the initial render.
How do you think?
Flags: needinfo?(odvarko)
(In reply to Tom Glowka from comment #9)
> I would opt for second option, as it both slightly simplifies the flow and
> uses the nicer formatting with space, at least in the initial render.
Agree, the second option sounds better.

Honza
Flags: needinfo?(odvarko)
Comment on attachment 9007744 [details]
Bug 1478361 - Simplify custom headers editing to prevent caret reset; r=Honza

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9007744 - Flags: review+
Comment on attachment 9007744 [details]
Bug 1478361 - Simplify custom headers editing to prevent caret reset; r=Honza

Jan Honza Odvarko [:Honza] has requested changes to the revision.
Attachment #9007744 - Flags: review+
Comment on attachment 9007744 [details]
Bug 1478361 - Simplify custom headers editing to prevent caret reset; r=Honza

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9007744 - Flags: review+
Keywords: checkin-needed
Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/536dcedd614e
Simplify custom headers editing to prevent caret reset; r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/536dcedd614e
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Is this worth uplifting to Beta for the DevEdition users?
Flags: needinfo?(glowka.tom)
Flags: in-testsuite+
It is quite annoying bug and changes are small, so I would say yes.
Flags: needinfo?(glowka.tom)
Comment on attachment 9007744 [details]
Bug 1478361 - Simplify custom headers editing to prevent caret reset; r=Honza

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: annoying ux for customizing HTTP headers when resending HTTP request
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: low risky
[Why is the change risky/not risky?]: Small patch, feature only for developers.
[String changes made/needed]: n/a
Attachment #9007744 - Flags: approval-mozilla-beta?
Comment on attachment 9007744 [details]
Bug 1478361 - Simplify custom headers editing to prevent caret reset; r=Honza

Minimal patch with tests affecting only devtools users, uplift approved for 63 beta 9, thanks.
Attachment #9007744 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.