924238 - SideMenuWidget's arrows don't update with black boxing

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Screen Shot 2013-10-07 at 3.50.27 PM.png

The center column with the editor changes to a dark gray but the arrow doesn't. This makes it look weird.

Comment 1

[Victor Porof [:vporof][:vp]]

We can either hide the arrow completely, or use a different image. I think we should hide it. It's easier.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Marking good first bug.

We define the CSS that makes the arrows here: http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/widgets.css#340 (and in browser/themes/{linux, windows})

If the selected source is black boxed, it will match this selector:

  #sources .selected .side-menu-widget-item-checkbox:not([checked])

In the debugger theme css files[0], hide the arrows when the selected source is black boxed.

[0] http://mxr.mozilla.org/mozilla-central/find?string=debugger.css&tree=mozilla-central&hint=themes

Comment 3

[Thomas Andersen]

Hi 

I would like to take this as an introduction to the code

Thomas

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Sure thing, check out these pages to learn how to hack on our tools:

  https://wiki.mozilla.org/DevTools/GetInvolved
  https://wiki.mozilla.org/DevTools/Hacking

If you have questions, let me know.

Comment 5

[Thomas Andersen]

Attachment: 924238-initial.patch

Thanks Nick!
Here is my first attempt to create a patch
hmm, I not sure where the patch commit message went. Shouldn't it appear in the header of the patch file? I followed the instructions here: 
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Anyway, I think it should be easy to see what's going on in this one.
 
Note: As I do not have a VM at the moment I have not verified this fix on Linux or Windows

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 814924 [details] [diff] [review]
924238-initial.patch

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

Thanks Thomas! This looks good to me. Maybe Benvie can double check windows, and Mihai can double check linux?

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Thomas Andersen from comment #5)
> Created attachment 814924 [details] [diff] [review]
> 924238-initial.patch
> 
> Thanks Nick!
> Here is my first attempt to create a patch
> hmm, I not sure where the patch commit message went. Shouldn't it appear in
> the header of the patch file? I followed the instructions here: 
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F

Commits don't really get translated in patches, just the total sum of changes, which is ok since in 99% of cases we push the whole change as a single commit when it goes to the tree.

Comment 8

[Brandon Benvie [:benvie]]

Looks good on Windows!

Comment 9

[Victor Porof [:vporof][:vp]]

Comment on attachment 814924 [details] [diff] [review]
924238-initial.patch

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

::: browser/themes/linux/devtools/debugger.css
@@ +65,5 @@
>  #sources .side-menu-widget-item-checkbox:not([checked]) ~ .side-menu-widget-item-contents > .dbg-breakpoint {
>    display: none;
>  }
>  
> +#sources .selected .side-menu-widget-item-checkbox:not([checked]) ~ .side-menu-widget-item-arrow  {

This is a bit of a costly selector. Can you please use 

#sources .side-menu-widget-item.selected > .side-menu-widget-item-checkbox:not([checked]) ~ .side-menu-widget-item-arrow

instead?

Comment 10

[Mihai Sucan [:msucan]]

Looks good on Linux!

Comment 11

[Thomas Andersen]

Attachment: 924238-optimized-selector.patch

sure!
This patch should fix this

Comment 12

[Thomas Andersen]

Attachment: Screen Shot 2013-10-09 at 9.50.29 PM.png

Is the .side-menu-widget-item-contents.selected used for anything? 

See the attached screenshot

Comment 13

[Victor Porof [:vporof][:vp]]

In the debugger's case, yes, it contains the source label string.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Doesn't apply to fx-team. Please rebase.

Comment 15

[Victor Porof [:vporof][:vp]]

Attachment: 924238-rebased.patch

Rebased.

Comment 16

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/c792dee47fed

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c792dee47fed