1047132 - DevTools: Panel button should show on hover a pointer cursor

Status: VERIFIED FIXED

(DevTools :: Framework, defect, P3)

Description

[sys.sgx]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release)
Build ID: 20140731030206

Steps to reproduce:

Click on the DevTools network panel, reload the page and click on the "Toggle performance analysis" link. You will get the graphics shown in the attached image.

The mouse cursor should be a "pointer" one when hovering the "Back" button link, since its linking to another "page". Currently, hovering the mouse it has an arrow cursor over the button.

Comment 1

[sys.sgx]

Attachment: Button.png

Comment 2

[Paul Silaghi, QA [:pauly]]

(In reply to sys.sgx from comment #0)
> The mouse cursor should be a "pointer" one when hovering the "Back" button
> link, since its linking to another "page". 
It's probably following the same behavior as the other devtools (click on the titles: inspector, console etc)

Comment 3

[sys.sgx]

The Inspector button resides in the "button area" of the devtools. Here, there is a button that is inside the information area of the network panel, so it's not distinguishable that this is something that can be clicked.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

I could see arguing either way, but I agree it should probably turn into an arrow cursor since it's navigational. There is probably a dev tools UX designer with a strong opinion for this one!

Comment 5

[[e-mail]]

Every clickable button should have the pointer cursor. I'd happy to mentor anyone who want help.

The button we want to have the pointer cursor in netmonitor are:

1. Back button on "Toggle performance analysis" link
Defined in components/statistics_panel http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/components/statistics-panel.js#252

2. The `Edit and Resend` and `Raw headers` button on headers sidebar panel
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/shared/components/headers-panel.js#201

3. The `Reload` and `statistics` icon in request-list-empty view
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/components/request-list-empty.js#40

What we need to do is find related element's classes, add correct style `cursor: pointer;` in netmonitor.css and make sure it does not affect other place.

Comment 6

[u566121]

Hey! I would like to work on this. I already have built Firefox and will look now for the CSS file you mentioned, Fred.

Comment 7

[u566121]

Attachment: | DevTools: Network Panel button should show on hover a pointer cursor

Comment 8

[u566121]

So, I have made the changes, but when I tried to actually push them I got an error saying that someone needs to assign this bug to me.
Sorry, this is my first time sending a patch here, haha. So, can someone assign this to me?
Thanks!

Comment 9

[[e-mail]]

(Thanks Marco to help assign the bug)

Beatriz, thanks for the quick patch! 

With this patch every thing works well, and I think it could be put into common.css so every devtool panels button shows a pointer cursor when hovered.

ntim, How do you think? Do we want this normal button hover affect for all devtools panels?

Comment 10

[Tim Nguyen :ntim]

(In reply to Fred Lin [:gasolin] from comment #9)
> (Thanks Marco to help assign the bug)
> 
> Beatriz, thanks for the quick patch! 
> 
> With this patch every thing works well, and I think it could be put into
> common.css so every devtool panels button shows a pointer cursor when
> hovered.
> 
> ntim, How do you think? Do we want this normal button hover affect for all
> devtools panels?

It's worth pointing out that the rest of the browser UI doesn't have a pointer cursor when hovering over buttons. So I'd let UX decide.

I personally don't have a strong opinion, but whatever we go with, it should be affect the whole toolbox, not just the netmonitor.

[0]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.css#245

Comment 11

[u566121]

Hello guys,

So, I was having a look here at the panel and I agree that we should standardize the feedback for hovered button throughout all devtools, but maybe the cursor pointer thing is not the way to go.

I think that the problem that triggered this bug was that some of the buttons on the netmonitor don't give proper feedback when hovered over. This problem has not appeared on other areas of the devtools because other buttons give better feedback, most of them change color intensity and background color when hovered over.

I think the best thing to do would be to create a standart interaction for all buttons on the devtools, changing color more noticeably when hovered over, etc. And maybe not putting "cursor: pointer" on all of them, because I think that would be too much.

I will give this a try and send a patch, ok?

Comment 12

[[e-mail]]

Thanks ntim's feedback, I changed title and category to reflect this issues should be resolved through whole panels, not netmonitor specific.

I think it make sense to make `button` behave like normal button when we advocate our devtool as devtools.html.


Beatriz, good suggestion! Thank you for volunteer to take a more longer time investigation.

It will help if you can post which parts you want to change, before send a patch. 
With this way people look at this issue can express their opinions and avoid some repeat works.

Comment 13

[u566121]

Ok Fred. 

So I was having a look now at the whole DevTools, searching for design inconsistencies and minor problems and I found some of them which I'll list here.

1. For Stand Alone buttons (like the one for Inspector>Animations>Pick an element form the page, or in Performance>Start Recording Performance, or even the Network>Reload buttons) the hover feedback is not really clear. So I think that for them, we should have a more drastic change of color and background. Also, they look different every time, so I guess we should standardize that. For these kinds of buttons I think the "cursor: pointer" would be useful.

2. In the Debugger tab, both "Collapse Panel" buttons are different than they are in other tabs. The "Pause" and "Step" buttons also are not like any other button on the DevTools. Still on the Debugger tab, everything now has "cursor: pointer", that should be removed once the right interaction for hovering is in place.

Now, these are the button questions that I found. The other ones are about design inconsistencies overall in the DevTools and I don't know if I should open a new bug for them here in Bugzilla or if we could just change this bug's name to something like "DevTools: straightening out design inconsistencies etc". I'll list them here anyways.

1. The Performance and Memory tabs are mostly the same when nothing is happening, but the alignment of the center button is different. Also the "Clear" button should be inactive on the Performance tab when there is nothing there to be cleared, just like on the Memory tab. Finally, we could put a "There are no snapshots yet." message on the Memory tab, just like there is in the Performance.

2. The debugger left-side panel should have the same styles as the Performance and Memory ones, since they are similar and have mostly the same parts.

I guess that's it for now, let me know what you think :)

(I'm sorry for not adding any screenshots here, I tried to, but could not figure out how. I'll look into it later, haha. Still learning how to use Bugzilla...)

Comment 14

[[e-mail]]

Beatriz, Thanks for the detailed report! I saw several valuable investigation here.

There are some actions you can do:

* You could file bugs in related bugzilla components, to make sure they will be tracked (while file the bug, you can put this bug in `see also` field for track)
* The debugger UI consistency is only happened in nightly because its a newly written, different debugger. (https://github.com/devtools-html/debugger.html) We'll ship it to before we get similar UI, so you don't have to file related bugs(and they might already there...)


Since this issue need more time for UX commitment and discussion, I suggest you to pick another one bug and come back later to track the status. 

Great Job!

Comment 15

[u566121]

Will do, Fred!

Thanks for the feedback.

Comment 16

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 6 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.

Comment 17

[Helena Moreno (aka helenatxu)]

(In reply to Patrick Brosset <:pbro> from comment #16)

Hello Patrick,

I may be interested in pick up this bug. Could you help me to see what is left to do to finish this bug? Thank you

Comment 18

[[:Towkir] Ahmed]

Are you still interested in this ?

Comment 19

[perezogayo]

Hello,

Is this bug still available? I would like to work on it.

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Victoria, this bug is about (In reply to u566121 from comment #13)

1. For Stand Alone buttons (like the one for Inspector>Animations>Pick an
   element form the page, or in Performance>Start Recording Performance, or
   even the Network>Reload buttons) the hover feedback is not really clear. So
   I think that for them, we should have a more drastic change of color and
   background. Also, they look different every time, so I guess we should
   standardize that. For these kinds of buttons I think the "cursor: pointer"
   would be useful.

2. In the Debugger tab, both "Collapse Panel" buttons are different than
   they are in other tabs. The "Pause" and "Step" buttons also are not like any
   other button on the DevTools. Still on the Debugger tab, everything now has
   "cursor: pointer", that should be removed once the right interaction for
   hovering is in place.

Now, these are the button questions that I found. The other ones are about
design inconsistencies overall in the DevTools and I don't know if I should
open a new bug for them here in Bugzilla or if we could just change this
bug's name to something like "DevTools: straightening out design
inconsistencies etc". I'll list them here anyways.

1. The Performance and Memory tabs are mostly the same when nothing is
   happening, but the alignment of the center button is different. Also the
   "Clear" button should be inactive on the Performance tab when there is
   nothing there to be cleared, just like on the Memory tab. Finally, we could
   put a "There are no snapshots yet." message on the Memory tab, just like
   there is in the Performance.

2. The debugger left-side panel should have the same styles as the
   Performance and Memory ones, since they are similar and have mostly the same
   parts.

I guess that's it for now, let me know what you think :)

(I'm sorry for not adding any screenshots here, I tried to, but could not
figure out how. I'll look into it later, haha. Still learning how to use
Bugzilla...)

Thanks for the summary!

Most of the things in this summary has been fixed over time, except of the hover button s in the Performance panel I think. But, this panel is subject for significant refactoring so, I think we can solve that hovering elsewhere.

Honza

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: image.png

Victoria, this bug is about cursor for the back button in the Network panel (performance analysis view). See the attached screenshot. It's currently 'pointer' and the suggestion is to use 'hand'.

It looks like we are using pointer for buttons across the DevTools UI so, I tend to say we should keep it but, wanted to check with you first.

Honza

Comment 22

[Victoria Wang [:victoria]]

Yes, the more pressing issue is that the button itself doesn't look like any other buttons.

A quick fix for this could be changing the shape of the button to a photon style button. This indeed should keep the pointer cursor.

Mockup: https://www.figma.com/file/Ni3AVbCkUFl1WEyElPg2Xh/network-performance-analysis?node-id=0%3A1

Comment 23

[aarushivij]

Hello, Can I work on this bug.
So, do we need to change the pointer to hand or as mentioned in comment 22 change the back button UI itself
Aarushi

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Victoria Wang [:victoria] from comment #22)

Yes, the more pressing issue is that the button itself doesn't look like any other buttons.

A quick fix for this could be changing the shape of the button to a photon style button. This indeed should keep the pointer cursor.

Mockup: https://www.figma.com/file/Ni3AVbCkUFl1WEyElPg2Xh/network-performance-analysis?node-id=0%3A1

Thanks Victoria for the mockup!

(In reply to aarushivij from comment #23)

Hello, Can I work on this bug.

Assigned to you, thanks for the help.

So, do we need to change the pointer to hand or as mentioned in comment 22 change the back button UI itself

The cursor should stay the same. We only want to change the back button itself, see Victoria's mockup

Honza

Comment 25

[aarushivij]

Ok, Thanks for assigning :)
Shall get back to you soon
Aarushi

Comment 26

[aarushivij]

Attachment: demo.png

Hello,
I have made the changes in the element.
Is it looking correct?
Also what color should the button be?
Thanks
Aarushi

Comment 27

[aarushivij]

Hey @Honza , please review the last comment so that I can make the changes in the file and make a patch.
Thanks :)

Comment 28

[Victoria Wang [:victoria]]

Attachment: image.png

Hi Aarushi, you can match the button color of the Edit & Resend button in the Network sidebar. There are different colors for dark and light theme. Thanks!

Comment 29

[aarushivij]

Attachment: Screenshot from 2020-02-08 12-13-17.png

Hey :victoria
I was working on the issue.
I know what rules to apply, but couldn't find the appropriate file.
I tried with using back-button class and adding the style into it in https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/netmonitor.css
But the rules did not apply on the back button.
Unlike the Edit and Resend button whose style is specifically mentioned in https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/NetworkDetailsPanel.css

Also about the background color of the button.
Should I use .light-theme .back-button{} style class for it.
Since the similar method is used in edit and resend button styling
So can you please guide me through.
I have attached the screenshot showing the styles to be added in .back-button.devtools-button{}
Thank you.
Aarushi

Comment 30

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to aarushivij from comment #29)

I know what rules to apply, but couldn't find the appropriate file.

The new CSS rules should go to StatisticsPanel.css file
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/StatisticsPanel.css

Also about the background color of the button.
Should I use .light-theme .back-button{} style class for it.
Since the similar method is used in edit and resend button styling

Yes

.edit-and-resend-button button styles are here
https://searchfox.org/mozilla-central/search?q=.edit-and-resend-button+&path=

And there are separate CSS rules for light and dark themes. We need something similar for the back button.
Something like as follows:

.theme-dark  .statistics-panel .devtools-toolbarbutton.back-button {
   // CSS props for dark theme
}

.theme-light  .statistics-panel .devtools-toolbarbutton.back-button {
   // CSS props for light theme
}

I have attached the screenshot showing the styles to be added in .back-button.devtools-button{}

Please attach your current patch so, I can try it on my machine.
You might want to read the docs here:
https://firefox-source-docs.mozilla.org/devtools/contributing/making-prs.html

Honza

Comment 31

[Tim Nguyen :ntim]

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #30)

.theme-dark  .statistics-panel .devtools-toolbarbutton.back-button {
   // CSS props for dark theme
}

.theme-light  .statistics-panel .devtools-toolbarbutton.back-button {
   // CSS props for light theme
}

.devtools-button is what you want, since .devtools-toolbarbutton is for XUL panels.

Also, you probably wouldn't need separate rules for theme-dark/theme-light, given that .devtools-button class + the data-standalone attribute would give you all the correct colors.

So if you need to add CSS rules (though you really don't need to, except maybe for margin), this is what you probably want:

.statistics-panel .back-button {

}

Comment 32

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for simplification Tim!

Honza

Comment 33

[aarushivij]

Thanks @Honza and @Tim
Shall make the changes and share the patch soon.

Comment 34

[aarushivij]

Attachment: Bug 1047132 - DevTools: Panel button should show on hover a pointer cursor. r=Honza

Comment 35

[aarushivij]

Hello
I have made the changes.
The specification for background color for dark-theme wasn't needed. But since in light theme originally the back button had that greyish color on hover only , so I specified it's background color.
Thanks for all the help :)
Shall make the changes if any after review
Aarushi

Comment 36

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Great thanks, commented in Phab.
Honza

Comment 37

[aarushivij]

Attachment: Bug 1047132 - DevTools: Panel button should show on hover a pointer cursor. r=Honza

Comment 38

[aarushivij]

Hey @Honza
Sorry, I submitted a new patch, though I used the amend command to make the changes in my commit. As mentioned in https://docs.firefox-dev.tools/contributing/making-prs.html
What was I doing wrong?
Thanks for the help
Aarushi

Comment 39

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to aarushivij from comment #38)

Hey @Honza
Sorry, I submitted a new patch, though I used the amend command to make the changes in my commit. As mentioned in https://docs.firefox-dev.tools/contributing/making-prs.html
What was I doing wrong?

Do not specify new -m commit message
(the doc needs to be fixed, see also bug 1613578)

Honza

Comment 40

[aarushivij]

Oh okay.
But now if I will amend, it will amend the new patch.
Is the latest patch acceptable.
Made the spacing changes in it.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a504e897ba1
DevTools: Panel button should show on hover a pointer cursor. r=Honza

Comment 42

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9a504e897ba1

Comment 43

[Rares Doghi, Desktop QA]

Hi Jan, After going through all the comments I'm a bit confused about what finally changed with this issue, is it just the Back button from the Network > Performance Analysis ?

if so, I noticed in the old builds that the button itself used to highlight a bit (change to a darker grey) when hovered over and now it does not highlight at all, I do see the Arrow cursor and the tooltip for the button. (This applies to the Dark Theme as well)

Can you please clear things up a bit so we can update the flags accordingly ?

Comment 44

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: image.png

(In reply to Rares Doghi from comment #43)

Hi Jan, After going through all the comments I'm a bit confused about what finally changed with this issue, is it just the Back button from the Network > Performance Analysis ?

Yes, correct.

The button should follow the same UX as the one we have in the Headers side panel Edit And Resend screenshot attached.
(no hover effect)

Honza

Comment 45

[Rares Doghi, Desktop QA]

Great, Based on Comment 44 we can confirm this issue Verified as Fixed in our latest Nightly build on Windows 10, Mac 10.14 and Ubuntu 18.04.