Closed Bug 1394325 Opened 7 years ago Closed 7 years ago

"Raw headers" button keep the same style when active

Categories

(DevTools :: Netmonitor, defect, P3)

defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: nchevobbe, Assigned: abhinav.koppula, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 3 obsolete files)

Steps to reproduce:

1. Open the netmonitor panel
2. Open the split console
3. Evaluate `fetch("https://api.github.com")`
4. In the netmonitor panel, select the github api call
5. Select the headers tab
6. Click the "raw headers" button

Expected results: 
The button turns blue to indicate it is active

Actual results:
The button keeps the same style

---

It should have a "checked" class when it is active, as well as an "aria-pressed" attribute to indicate if the button is active or not.
Mentor: odvarko
Keywords: good-first-bug
Priority: -- → P3
Hey, i might be interested in looking at this as a 1st bug... 
I understand that the submitter wants the "Raw headers" button to be colorized when pressed. Same as for the "Headers" tab and the "Network" tab in Devtools.
(even though you get the dotted square on the button when you press the Raw headers button, those dots goes away when focus is taken away from the browser).

After that my understanding goes abit fuzzy. 
In the devtools/client/netmonitor/src/components/headers-panel.js is where u handle the button actions.

and in the devtools/client/themes/common.css is where it says what layout is used to paint the button...  
It uses the normal devtools-button properties. 
So is "simply" to modify devtools/client/themes/common.css so that when selected the button gets a "selected" color?
(In reply to Nicklas Boman from comment #1)
> Hey, i might be interested in looking at this as a 1st bug... 
> I understand that the submitter wants the "Raw headers" button to be
> colorized when pressed. Same as for the "Headers" tab and the "Network" tab
> in Devtools.
> (even though you get the dotted square on the button when you press the Raw
> headers button, those dots goes away when focus is taken away from the
> browser).
> 
> After that my understanding goes abit fuzzy. 
> In the devtools/client/netmonitor/src/components/headers-panel.js is where u
> handle the button actions.
> 
> and in the devtools/client/themes/common.css is where it says what layout is
> used to paint the button...  
> It uses the normal devtools-button properties. 
> So is "simply" to modify devtools/client/themes/common.css so that when
> selected the button gets a "selected" color?

Hello Nicklas, thanks for taking some time to work on this :) !
So here, the only thing that you should modify is the button (http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/headers-panel.js#209-212).
You should apply the "checked" class to the button when it is active. The "checked" class is already defined (http://searchfox.org/mozilla-central/source/devtools/client/themes/common.css#340) and will take care of styling the button.

You can take this component http://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/components/filter-button.js#27-32 as an example of what should be done : adds the "checked" class if `this.state.rawHeadersOpened` is true.
In the same time, you should add a "aria-pressed" prop which will have the same value as `this.state.rawHeadersOpened`.

If you have questions, or simply want to chat with the team, come and say hi in our Slack https://devtools-html-slack.herokuapp.com/ :)
Attached patch bz-1394325-raw-headers.patch (obsolete) — Splinter Review
Hi Nicolas,
I have created a patch for the mentioned changes. Please let me know if I have missed anything.

Also, I'm sorry if this bug is being worked upon by someone else. I couldn't see any activity since 4 days and hence thought of taking a stab at this issue.
Attachment #8903896 - Flags: review?(nchevobbe)
Hi Abhinav,
i was kinda looking at it, but as you say not so actively + was sortof struggling, so i will look at your patch and see what i did wrong :)
br Nicklas
Hi Nicolas,
I have updated the patch with the test included.
I had a small query - In line 43 of the test file, I had to change document.querySelectorAll(".headers-summary .devtools-button")[1]); to
document.querySelectorAll(".headers-summary .devtools-button")[2]);
since I feel that we were targeting a click on the wrong button initially - Edit and Resend. However, the test - testHideRawHeaders - was still passing since .raw-headers-container wasn't available even after 'Edit and Resend' button was clicked.

Please let me know if my understanding is correct.
Attachment #8903896 - Attachment is obsolete: true
Attachment #8903896 - Flags: review?(nchevobbe)
Attachment #8904000 - Flags: review?(nchevobbe)
Assignee: nobody → abhinav.koppula
Comment on attachment 8904000 [details] [diff] [review]
bz-1394325-raw-headers-updated.patch

Review of attachment 8904000 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot Abhinav for working on this.
This is all looking good, and when you'll have addressed the few comments in this review I'll happily r+ it :)

Also, could you rebase your patch against the tip of mozilla-central ? I was unable to apply since it was giving me conflicts.

::: devtools/client/netmonitor/test/browser_net_raw_headers.js
@@ +40,4 @@
>    testShowRawHeaders(getSortedRequests(store.getState()).get(0));
>  
>    EventUtils.sendMouseEvent({ type: "click" },
> +    document.querySelectorAll(".headers-summary .devtools-button")[2]);

Good catch. You did the right thing here, we were clicking the "edit and resend" button, which was hiding the "raw headers" panel for sure, but this isn't quite what we wanted :)

May I suggest that we add in this file a small function `getRawHeadersButton`, which would look like the following : 
```
function getRawHeadersButton() {
  return document.querySelectorAll(".headers-summary .devtools-button")[2]);
}
```

and call it whenever we need it.

@@ +60,5 @@
> +    if (checked) {
> +      is(rawHeadersButton.classList.contains("checked"), true,
> +        "The 'Raw Headers' button should have a 'checked' class.");
> +      is(rawHeadersButton.getAttribute("aria-pressed"), "true",
> +        "The 'Raw Headers' button should set 'aria-pressed' = true.");

nit: replace "The 'Raw Headers' button should set 'aria-pressed' = true" with "The 'Raw Headers' button should have the 'aria-pressed' attribute set to true"

@@ +65,5 @@
> +    } else {
> +      is(rawHeadersButton.classList.contains("checked"), false,
> +        "The 'Raw Headers' button should not have a 'checked' class.");
> +      is(rawHeadersButton.getAttribute("aria-pressed"), "false",
> +        "The 'Raw Headers' button should set 'aria-pressed' = false.");

nit: replace "The 'Raw Headers' button should set 'aria-pressed' = false" with "The 'Raw Headers' button should have the 'aria-pressed' attribute set to false"
Attachment #8904000 - Flags: review?(nchevobbe) → review-
Hi Nicolas,
Thanks for the review.
Have incorporated the review-feedback and have rebased with the latest code.
Attachment #8904000 - Attachment is obsolete: true
Attachment #8905239 - Flags: review?(nchevobbe)
Comment on attachment 8905239 [details] [diff] [review]
bz-1394325-raw-headers-review-feedback.patch

The patch looks good to me.
The only thing you need is a valid commit message which should look similar to something like : 

"Bug 1394325 - Add an active state for the "Raw headers" button. r=nchevobbe"

All the patch that you submit should have this same pattern.

I pushed the patch to TRY, which is our CI and will run all the tests in the repo to make sure your patch does not break something elsewhere : https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b757f0fd0a7c691a4e423faf906ae2248c6410f 

When this is done and we see that there is no issues, we'll be able to land the patch :)
Attachment #8905239 - Flags: review?(nchevobbe) → review+
TRY is not over but we can already see this failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b757f0fd0a7c691a4e423faf906ae2248c6410f&selectedJob=129484547https://treeherder.mozilla.org/logviewer.html#?job_id=129484547&repo=try&lineNumber=264

In short, it's an ESLint failure, that means the patch does not match the style required in devtools code.
You can either run eslint from the command line, or have it run directly in your editor. You might want to check http://docs.firefox-dev.tools/contributing/eslint.html to see how to install ESLint and the various ways of running it.

So for this patch, you need to: 
- update the commit message
- fix the ESLint error

Then we'll push it again to TRY and land it if everything is good :)
Comment on attachment 8906227 [details]
Bug 1394325 - Add an active state for the "Raw headers" button.

https://reviewboard.mozilla.org/r/177982/#review183676

Looks good. I pushed to TRY again so we'll land this when its done if everything is fine.
Thanks !
Attachment #8906227 - Flags: review?(nchevobbe) → review+
Comment on attachment 8905239 [details] [diff] [review]
bz-1394325-raw-headers-review-feedback.patch

New patch in https://reviewboard.mozilla.org/r/177982/
Attachment #8905239 - Attachment is obsolete: true
Everything looks good in TRY.
There is a failure : https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5395e54e7801d98a0c3842188825428225ea5af&selectedJob=130298086 , but this is unrelated to your patch (what we call an intermittent, or orange).
Let's land your patch !
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28713eaae8c2
Add an active state for the "Raw headers" button. r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/28713eaae8c2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
I have reproduced this bug according to (2017-8-28)

Fixing bug is verified on Latest Nightly--
Build ID    :20170915220136
User Agent  :Mozilla/5.0 (Windows NT 6.1; rv:57.0) Gecko/20100101 Firefox/57.0

Tested OS-- WIndows7 32bit
QA Whiteboard: [bugday-20170913]
(In reply to Saheda Reza [:Antora] from comment #16)
> I have reproduced this bug according to (2017-8-28)
> 
> Fixing bug is verified on Latest Nightly--
> Build ID    :20170915220136
> User Agent  :Mozilla/5.0 (Windows NT 6.1; rv:57.0) Gecko/20100101
> Firefox/57.0
> 

Also verified on Latest Beta--
Build ID    :20171218174357
User Agent  :Mozilla/5.0 (Windows NT 6.1; rv:58.0) Gecko/20100101 Firefox/58.0

> Tested OS-- WIndows7 32bit
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.