Closed Bug 1202260 Opened 8 years ago Closed 8 years ago

Last event listener entry in event listeners tooltip has unnecessary border-bottom

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
trivial

Tracking

(firefox46 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: arni2033, Assigned: pbro)

References

(
URL
)

Details

(Whiteboard: [lang=css][polish-backlog][difficulty=easy])

Attachments

(3 files, 3 obsolete files)

screenshot - Last event listener entry in event listeners tooltip have unnecessary border-bottom.png
21.33 KB, image/png
Details
bug 1202260 - Result after applying my CSS. Just perfect IMO.png
15.36 KB, image/png
Details
MozReview Request: Bug 1202260 - Remove the last border in the event-listener popup and unnecessary padding
58 bytes, text/x-review-board-request
miker
: review+
Details 
STR:   (Win7_64, Nightly 43, 32bit, ID 20150901030226, new profile, safe mode)
1. Open this "data:" url or click URL in the field above
data:text/html,<body onload="var str=''; for (i=1;i<999;i++)str+='L'; for (i of ['oncut','onpaste','oncopy'] ) document.body.setAttribute(i,str); ">
2. Open devtools->Options, select Light devtools theme
2. Open devtools->Inspector, click (ev) button near <body> element

Result:       In tooltip with event listeners you can see bottom border after each event listener button, even after the last one
Expectations: There should be no border after the last tooltip

Note: I actually use Dark devtools theme, and that border in dark devtools theme makes it look like there's additional indent, which looks like a misaligned button.

Suggestion:   Currently, CSS declares the bottom border of 1px width for every event listeners button. I suggest to change CSS so that there was a border ABOVE every button (border-top), except the 1st one - using ":not(:first-child)" selector.
> ":not(:last-child)" can't be used here for "border-bottom", because all odd elements in that tooltip are buttons, and even elements are code areas, so the last element is code area)
BUT I can see that the border may have been added to separate code area from the button. Therefore, I suggest to make additional border-top with the same color for code area, if it's visible.
Severity: normal → trivial
Summary: Last event listener entry in event listeners tooltip have unnecessary border-bottom → Last event listener entry in event listeners tooltip has unnecessary border-bottom
See Also: → 1202274
So, I suggest the following CSS as a fix. Attached screenshot shows the result after my fixes in this bug and bug 1202274. I've tested it in extension "Stylish". I hope somebody will just apply my CSS (in an appropriate .css-file in browser).
>   .event-tooltip-content-box
>   {
>     border-style: solid;
>     border-width: 1px 0px 0px;
>     border-color: var(--theme-splitter-color);
>   }
>   
>   .devtools-toolbar.event-header
>   {
>     border-width: 1px 0px 0px;
>   }
>   .devtools-toolbar.event-header:first-child
>   {
>     border-width: 0px;
>   }
Thanks Arni. Marking this as a polish bug.
Looks like you've done the research and have some code working, would you like me to assign the bug to you so you can submit a patch for review?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][lang=css][polish-backlog]
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #2)
> would you like me to assign the bug
Unfortunately I can't spent time on compiling stuff for now, so please leave it "free".
If somebody's going to work on this, note that my style from comment 1 may be not good for dark devtools theme: var(--theme-splitter-color) is black and borders between event buttons are dark-gray
>>>   http://ssmaker.ru/ec0fc40d.png   <<<
My CSS was just a draft, and maybe this needs some designer's decision. But I personally think that .event-tooltip-content-box should use the same border color as .devtools-toolbar.event-header
Whiteboard: [good first bug][lang=css][polish-backlog] → [good first bug][lang=css][polish-backlog][difficulty=easy]
Hi! Can I try to fix this bug? This will be my first one. Thanks :)
(In reply to Sanchit Nevgi from comment #4)
> Hi! Can I try to fix this bug? This will be my first one. Thanks :)
Awesome, thanks for wanting to fix this.
Have you already downloaded and built firefox? If not, please make sure you go through our getting-started guide: https://wiki.mozilla.org/DevTools/Hacking

Once done, the file that contains the CSS code for the tooltip is:
\devtools\client\themes\inspector.css
There's a bunch of classes that start with ".event-tooltip" in there.

Suggested workflow:
- Build firefox (will be quite long the first time)
- Launch firefox and make sure your enable the browser toolbox: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
- Open the devtools, and using the browser toolbox, inspect the tooltip to see which css classes/rules/properties need to be fixed
- Make changes in the source code
- Re-build just these changes: ./mach build devtools/client
- And relaunch firefox
Flags: needinfo?(pbrosset)
Thanks Patrick for the guidance. I built Firefox and have modified inspector.css adding changes that were mentioned in Comment 1. The last border is removed (on both themes, hopefully). I had to add !important to override the styles in toolbar.inc.css. Please review my patch and tell me whether I need to make any additional changes.

Thanks for your help!

        Attachment #8672942 -
        Flags: feedback+

    
    

    
  
Comment on attachment 8672942 [details] [diff] [review]
patch to remove last border in tooltip

Setting the R? flag so that I remember to do the review.
Feedback+ is a flag that signifies that someone has reviewed the patch and given a positive feedback about it.

        Attachment #8672942 -
        Flags: feedback+ → review?(pbrosset)

    
    

    
  
Hi Patrick! Did you get a chance to review my patch? Thanks :)
Flags: needinfo?(pbrosset)

    
    

    
  
Comment on attachment 8672942 [details] [diff] [review]
patch to remove last border in tooltip

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

Nice, this works. Thanks for this.
I have however made some comments below. Could you please try to make the changes described and see if this works? I think the code would be simpler this way.

::: devtools/client/themes/inspector.css
@@ +88,5 @@
>    align-items: center;
>    cursor: pointer;
> +  border-style: solid;
> +  border-width: 1px 0px 0px !important;
> +  border-color: var(--theme-splitter-color);

I'm wondering if it's necessary to specify the border style and color here. They're already specified on .devtools-toolbar and we don't want to change them.
Also, why switch the border to be at the top rather than at the bottom.

My impression is that we shouldn't have to change this rule at all. Only add one for the last-child, a bit like you did below.

@@ +92,5 @@
> +  border-color: var(--theme-splitter-color);
> +}
> +
> +.event-header:first-child {
> +  border-width: 0px !important;

If we keep the border to be displayed at the bottom like .devtools-toolbar does, then this should be .event-header:last-child instead.

Also, it'd be really better if you could avoid using !important. This makes it really hard in the future to make style changes to the element.

I'm guessing it's just a matter of .devtools-toolbar taking precedence because it's defined after this rule?

Why not use:
devtools-tooltip-events-container .event-header:last-child
to make sure this rule overrides the one in toolbar.inc.css ?

        Attachment #8672942 -
        Flags: review?(pbrosset)

    
    

    
  
Ah, also, we need the border between the header and the content to still be there. Moving the border to the top removes it. So we need it to stay at the bottom, but in case of the last-child, it should only be there if the content is visible (and actually, this should be last-of-type, I was wrong before when I mentioned we should use last-child).
Flags: needinfo?(pbrosset)

    
    

    
  
Maybe you could use this to make sure there's a border between the header and the content on the last item:

.event-tooltip-content-box:last-of-type {
  border-top: 1px solid var(--theme-splitter-color);
}

    
    

    
  
I still think that my suggestion is better, BUT anyway I noticed that dark-theme doesn't use var(--theme-splitter-color) for border-color of ".event-header"s while light-theme uses it:
chrome://devtools/skin/themes/dark-theme.css : line ~202
>   .devtools-toolbar {
>      border-color: hsla(210,8%,5%,.6);    /*  not var(--theme-splitter-color)  */
It's important here because we're going to color content areas' borders (I mentioned this in comment 3)

    
    

    
  
Hello, can you assign me this bug as this will be an interesting one to debug?

    
    

    
  
H Sanchit, are you planning on working on this bug still? I provided some feedback in comment 10 and comment 11. There is also some input from arni in comment 13.
If not, then obongofon can probably take over.
Flags: needinfo?(sanchit.nevgi)

    
    

    
  
PLease where can I get the source code for this bug?

    
    

    
  
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #16)
> H Sanchit, are you planning on working on this bug still? I provided some
> feedback in comment 10 and comment 11. There is also some input from arni in
> comment 13.
> If not, then obongofon can probably take over.

Hey Patrik! I'm a little preoccupied at the moment so I won't be able to work on this bug. Let obongofon work on it. :)
Flags: needinfo?(sanchit.nevgi)

    
    

    
  
(In reply to obongofon from comment #17)
> PLease where can I get the source code for this bug?
See comment 5, everything is documented here: https://wiki.mozilla.org/DevTools/Hacking
(note however that unlike what I said in comment 5, re-building Firefox after you've made changes should be done with './mach build faster' now).

    
    

    
  
I am done building the firefox. Is it okay if I continue with this
devtools/client/themes/inspector.css
then start working on it?

    
    

    
  
(In reply to obongofon from comment #20)
> I am done building the firefox. Is it okay if I continue with this
> devtools/client/themes/inspector.css
> then start working on it?
Yes, please feel free to start from the previous patch and work from there.

    
    

    
  
hello patrick, 
I am interested in working on this bug, i have built the build the firefox all I need is your permission to start working on the bug.

    
    

    
  
(In reply to Babatunde Salaam from comment #22)
> hello patrick, 
> I am interested in working on this bug, i have built the build the firefox
> all I need is your permission to start working on the bug.
No one needs my permission to work on a bug. Feel free to submit a patch if you have one.
Note however that obongofon was interested too, so may they are working on it already, not sure.

    
    

    
  
Does the .eventheader class control the scroll in the attached image? If so, adding overflow hidden eliminates it.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        eventlistner.patch (obsolete)
        — 
          — Splinter Review
      

    


  
hello patrick i have read comment10  and took over from the last patch, i have also made all the changes you suggested but the problem am having now is how to get rid of using important.
can you also please take a look at my patch and see if any changes could be made. 

thanks

    
    

    
  
(In reply to Babatunde Salaam from comment #25)
> Created attachment 8697537 [details] [diff] [review]
> eventlistner.patch
> 
> hello patrick i have read comment10  and took over from the last patch, i
> have also made all the changes you suggested but the problem am having now
> is how to get rid of using important.
> can you also please take a look at my patch and see if any changes could be
> made. 
> 
> thanks
Did you test your patch locally, by building your changes and running Firefox?
Your changes include this css selector: ".event-header;last-child", which is invalid, so there's no way it can work.
Can you please give it a try locally first and submit a new patch. When you do, can you please flag me for review (on the upload page, you can change the review drop-down to '?' and then enter my name in the input field next to it, so that I remember to take a look at it)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        tooltip_removeborder.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Hey Patrick, I have followed the instructions set out for this bug, and created a patch including the additional recommended lines: 

.event-tooltip-content-box:last-of-type {
  border-top: 1px solid var(--theme-splitter-color);
}

Can you please have a look, and get back to me if I need to make any further adjustments. 

Thank you

Bahno Mirzad

        Attachment #8697566 -
        Flags: review?(pbrosset)

        Attachment #8697566 -
        Flags: feedback?

    
    

    
  
Comment on attachment 8697566 [details] [diff] [review]
tooltip_removeborder.patch

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

Several comments here:

- First of all, sorry for the delay answering here, last week was a big mozilla event, and I'm just now going back to my bugzilla review queue.
- 3 persons have submitted patches here. Please be considerate of other people's work, there's not much sense all working on the same bug, there are many other bugs that are good to get to know the code base when you want to contribute for the first time, I suggest asking on our IRC channel (more info on our wiki [1]) or visiting http://firefox-dev.tools
- Finally, this patch is invalid, it has been generated incorrectly somehow, because it doesn't apply cleanly for me when I import it. Information about this on this MDN page [2]

[1] https://wiki.mozilla.org/DevTools/GetInvolved
[2] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

        Attachment #8697566 -
        Flags: review?(pbrosset)

        Attachment #8697566 -
        Flags: feedback?

    
    

    
  
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #28)
> Comment on attachment 8697566 [details] [diff] [review]
> tooltip_removeborder.patch
> 
> Review of attachment 8697566 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Several comments here:
> 
> - First of all, sorry for the delay answering here, last week was a big
> mozilla event, and I'm just now going back to my bugzilla review queue.
> - 3 persons have submitted patches here. Please be considerate of other
> people's work, there's not much sense all working on the same bug, there are
> many other bugs that are good to get to know the code base when you want to
> contribute for the first time, I suggest asking on our IRC channel (more
> info on our wiki [1]) or visiting http://firefox-dev.tools
> - Finally, this patch is invalid, it has been generated incorrectly somehow,
> because it doesn't apply cleanly for me when I import it. Information about
> this on this MDN page [2]
> 
> [1] https://wiki.mozilla.org/DevTools/GetInvolved
> [2]
> https://developer.mozilla.org/en-US/docs/Mercurial/
> Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F

No worries, and I understand, just wanted to see if I was doing the process correctly. 
Thank you for the advice, I will look at the other recommended bugs.

    
  
Has STR: --- → yes

    
    

    
  
Hi, can someone assign me to this bug please.

    
    

    
  
Hi Matt, even if nobody is assigned to this bug at the moment, there are already several patches for it. Please see comment 28.

Patrick, I believe somebody of the previous contributors should be set as assignee to avoid confusions and be asked to finish the patch. Could you please choose one?

Sebastian
Flags: needinfo?(pbrosset)

    
  
Flags: needinfo?(pbrosset)

    
  
Assignee: nobody → pbrosset
Mentor: pbrosset
Status: NEW → ASSIGNED
Whiteboard: [good first bug][lang=css][polish-backlog][difficulty=easy] → [lang=css][polish-backlog][difficulty=easy]

    
  

        Attachment #8672942 -
        Attachment is obsolete: true

    
  

        Attachment #8697537 -
        Attachment is obsolete: true

    
  

        Attachment #8697566 -
        Attachment is obsolete: true

    
    

    
  
Cleaned this bug up a bit. Sorry about this, but this bug wasn't going anywhere, so I just took and uploaded a patch for review.

@Mike: for info, the patch adds the border to the top of the header, instead of bottom (except for the first header), and also adds the border to the top of the content area.
It also removes a weird bottom margin below the content area.

    
    

    
  
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #32)
> Created attachment 8706945 [details]
> Remove the last border in the event-listener popup and unnecessary padding
Well actually I filed another bug 1202274 about the unnecessary padding.
If I understand correctly, your patch will fix it as well.
Blocks: 1202274

    
    

    
  
(In reply to arni2033 from comment #34)
> (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #32)
> > Created attachment 8706945 [details]
> > Remove the last border in the event-listener popup and unnecessary padding
> Well actually I filed another bug 1202274 about the unnecessary padding.
> If I understand correctly, your patch will fix it as well.
Yes, it should fix this too. Sorry, I didn't see that other bug.

    
    

    
  
Comment on attachment 8706945 [details]
MozReview Request: Bug 1202260 - Remove the last border in the event-listener popup and unnecessary padding

https://reviewboard.mozilla.org/r/30541/#review27307

As long as long lists of events still scroll fine and individual events are still expandable then this is fine.

        Attachment #8706945 -
        Flags: review?(mratcliffe) → review+

    
  
No longer blocks: 1202274

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/7737b44d63ee
Status: ASSIGNED → RESOLVED
Closed: 8 years ago

          status-firefox46:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

    
    

    
  
[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
STR: clear.
Test successful

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
No border to last element of Event listener.

Actual Results: 
As expected

    
  
Product: Firefox → DevTools

          You need to log in
          before you can comment on or make changes to this bug.