XHR labels are not aligned

RESOLVED FIXED in Firefox 60

Status

P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: nchevobbe, Assigned: tim.xie, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 60
good-first-bug

Firefox Tracking Flags

(firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

Details

(URL)

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

a year ago
Steps to reproduce:
1. Go to https://console-html-network-requests.glitch.me/
2. Open the console
3. Make sure the "XHR" filter is on
4. Click on the "Trigger Network requests" button
5. You should see 5 XHR requests (resp. GET, OPTIONS, PUT, POST, DELETE)

Expected results:
All the requests are laid-out nicely

Actual results:
The XHR labels are not aligned, which makes the console looks messy (see attachment)

---

I think this can be solved by adding a fixed width in http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/devtools/client/themes/webconsole.css#981-983 , so the method all have the same size. 
The downsize is that if we only have GET requests, it can feels like the "GET" and "XHR" elements are a bit disconnected.

The other solution would be to place the XHR label before the method name, which could be done in http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/devtools/client/webconsole/new-console-output/components/message-types/NetworkEventMessage.js#101, by inverting `method` and `xhr`
(Reporter)

Updated

a year ago
Has STR: --- → yes
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [console-html][triage]
Hi, is this issue still available.  If so, I can take it?
(Reporter)

Comment 2

a year ago
(In reply to Robert Pirritano from comment #1)
> Hi, is this issue still available.  If so, I can take it?

Yes it is, please do :)
Assignee: nobody → rpirritano
Status: NEW → ASSIGNED
(Reporter)

Comment 3

a year ago
Don't hesitate to ask me anything on that
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> Don't hesitate to ask me anything on that

Thanks, will do!
(Reporter)

Comment 5

a year ago
(In reply to Robert Pirritano from comment #4)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> > Don't hesitate to ask me anything on that
> 
> Thanks, will do!

You can come in our slack https://devtools-html-slack.herokuapp.com/ if you want to chat :)
Posted patch tip.patch (obsolete) — Splinter Review
I used the solution of placing the XHR label before the method name
Attachment #8927686 - Flags: review?(nchevobbe)
(Reporter)

Comment 7

a year ago
Comment on attachment 8927686 [details] [diff] [review]
tip.patch

Hello, thanks for working on this.
Sadly, the patch you uploaded seems invalid.

Furthermore, we are trying to not use splinter review (where you upload patch file to bugzilla) anymore.

Would you mind having a look at http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html and try to push your patch to mozreview ?

Thanks !
Attachment #8927686 - Flags: review?(nchevobbe) → review-
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)
> Comment on attachment 8927686 [details] [diff] [review]
> tip.patch
> 
> Hello, thanks for working on this.
> Sadly, the patch you uploaded seems invalid.
> 
> Furthermore, we are trying to not use splinter review (where you upload
> patch file to bugzilla) anymore.
> 
> Would you mind having a look at
> http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/
> install.html and try to push your patch to mozreview ?
> 
> Thanks !

Oh no, so sorry, this is my first time working with a patch.  Definitely, I will look at the link you provided and use mozreview. Sorry for the inconvenience!
(Reporter)

Comment 9

a year ago
No worries ;) Thank you for working on this
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8927686 - Attachment is obsolete: true
(Reporter)

Comment 11

a year ago
mozreview-review
Comment on attachment 8928319 [details]
Bug 1415211 - XHR label are not aligned.  Inverted the method name and xhr; :nchevobbe

https://reviewboard.mozilla.org/r/199522/#review205304

Thanks for working on that Robert !
Unfortunately, the patch looks weird since it shows the file as "new" when we should only see the `xhr` move.
Can you look at it and see if you can fix this ?

Also, something I did not told you, we need to make sure the tests still pass.
To run our component test you can do :
`cd devtools/client/webconsole && npm install` and then `npm test`.
Attachment #8928319 - Flags: review?(nchevobbe) → review-

Comment 12

a year ago
mozreview-review-reply
Comment on attachment 8928319 [details]
Bug 1415211 - XHR label are not aligned.  Inverted the method name and xhr; :nchevobbe

https://reviewboard.mozilla.org/r/199522/#review205304

oh that is wierd, since I only changed that in the file, let me check it out and I will also run the component tests.
Whiteboard: [console-html][triage]
Comment hidden (mozreview-request)
(Reporter)

Comment 14

a year ago
Posted image Patch applied
Honza, does this look good to you ?
I like the change, it only seems to reveal a minor an alignment issue
Flags: needinfo?(odvarko)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #14)
> Created attachment 8931291 [details]
> Patch applied
> 
> Honza, does this look good to you ?
> I like the change, it only seems to reveal a minor an alignment issue
Looks good, but can we also properly v-center the toggle-arrow icon?
I think that all we need to do is use:
align-items: center;
... for the message flex layout.

Honza


diff --git a/devtools/client/themes/webconsole.css b/devtools/client/themes/webconsole.css
--- a/devtools/client/themes/webconsole.css
+++ b/devtools/client/themes/webconsole.css
@@ -24,16 +24,17 @@ a {
 *:visited { }

 * {
   box-sizing: border-box;
 }

 .message {
   display: flex;
+  align-items: center;
   padding: 0 7px;
   width: 100%;
   box-sizing: border-box;
 }

 .message > .prefix,
 .message > .timestamp {
   flex: none;
Flags: needinfo?(odvarko)
(Reporter)

Comment 16

a year ago
mozreview-review
Comment on attachment 8928319 [details]
Bug 1415211 - XHR label are not aligned.  Inverted the method name and xhr; :nchevobbe

https://reviewboard.mozilla.org/r/199522/#review207834

Okay, we need to fix the alignment issue, and then it should be good !
Attachment #8928319 - Flags: review?(nchevobbe) → review+
(Assignee)

Comment 17

a year ago
Hi, 
Is this bug still open? There doesn't seem to be anything left to do, but no one is here for finish it up...

Tim
Flags: needinfo?(nchevobbe)
(Reporter)

Comment 18

a year ago
Yes, this bug is still open.
Robert, would you have the time to finish this up ? If not, do you mind if someone else does ?

Tim, would you want to finish it ?
Flags: needinfo?(tim.xie)
Flags: needinfo?(rpirritano)
Flags: needinfo?(nchevobbe)
Sure, Tim can finish it since I wont be able to get to it for a while :)
Flags: needinfo?(rpirritano)
Summary: XHR label are not aligned → XHR labels are not aligned
(Assignee)

Comment 20

a year ago
Hi Nicolas, 
I would like to finish this bug! (or what's left of it) If I understood this correctly, the only part left is this: 

(In reply to Jan Honza Odvarko [:Honza] from comment #15)
> Looks good, but can we also properly v-center the toggle-arrow icon?
> I think that all we need to do is use:
> align-items: center;
> ... for the message flex layout.
> 
> Honza

Tim
Flags: needinfo?(tim.xie) → needinfo?(nchevobbe)
Comment hidden (mozreview-request)
(Reporter)

Comment 22

a year ago
Thanks for working on this Tim !
So, the XHR label is now aligned with the arrow and the text, but other messages are off (see non-xhr requests and groups).

I think here we should only act on the xhr label to align it with the other parts of a message
Flags: needinfo?(nchevobbe)
(Reporter)

Updated

a year ago
Attachment #8928319 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 24

a year ago
XHR label is a bit taller than the line height, so I reduced the vertical padding this time. The toggle button is a bit hard to align so I used relative positioning. 

Tim
(Reporter)

Comment 25

a year ago
It sill looks a bit off http://prntscr.com/il8pil

Basically what we would need is to align-items: baseline
I tested it quickly and there's an issue with the arrow we need to figure out, but aside from that, it should be good
Assignee: rpirritano → tim.xie
(Reporter)

Comment 26

a year ago
Plase ignore my previous comment, I didn't build with your patch … (need another coffee :D)
With you actual patch, this is looking good !

So before we can land this, could you do 2 things: 
 - squash your 2 commits together, there's no need for separate commits here
 - append "; r=nchevobbe." at the end of the first line of your commit message. That's something that should be done everytime so it ping the reviewer that they have a review to do.

After that, we'll push to TRY to run all the tests of the repo, and if everything is okay, we'll land the patch :)

Thanks !
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8954661 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8954927 - Attachment is obsolete: true
(Assignee)

Comment 28

a year ago
I squash the commits together (I think), and pushed it to review. Please let me know if there is a problem. 

Thank you!
Tim
(Reporter)

Comment 29

a year ago
It does look good :)
I have one very last thing I should have noticed before: the XHR pill has an inline-start margin that we don't need anymore.

So in this rule: https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/devtools/client/themes/webconsole.css#1007-1013 , we should override this property: https://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#295 with `margin-inline-start: 0`.

Also, I am going to push the patch to TRY (our CI infra) so we can see if we need to update a test.
(Assignee)

Comment 30

a year ago
It seems like a few tests failed on TRY, but I have no idea what the results are showing....
(Reporter)

Comment 31

a year ago
I don't think they are related to your patch.
Most of the time TRY shows Intermittent failures (aka "Oranges"), which are the result of racy tests / features.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8955613 - Attachment is obsolete: true
Attachment #8955613 - Flags: review?(nchevobbe)
(Assignee)

Comment 33

a year ago
I just updated the margin-inline-start property mentioned above and pushed to review.
(Reporter)

Comment 34

a year ago
mozreview-review
Comment on attachment 8957473 [details]
Bug 1415211 - Realigned XHR label, set property margin-inline-start of XHR to 0;

https://reviewboard.mozilla.org/r/226376/#review232268

That's perfect Tim,thanks a lot !
I pushed to TRY one more time to be extra careful, but when the jobs are over I think we'll be able to land this
Attachment #8957473 - Flags: review?(nchevobbe) → review+

Comment 35

a year ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6bf3a552fc51
Realigned XHR label, set property margin-inline-start of XHR to 0; r=nchevobbe
(Assignee)

Comment 36

a year ago
Thank you very much!

Comment 37

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6bf3a552fc51
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
status-firefox58: affected → wontfix
status-firefox59: --- → wontfix
I have reproduced this bug with Nightly 58.0a1 (2017-11-07) on Windows 10, 64 Bit!

This bug's fix is verified with latest Beta!

Build ID  : 20180322152034
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
QA Whiteboard: [bugday-20180321]

Updated

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