872058 - Move the clear button next to the output/checkbox buttons

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

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

The relationship between "Clear", "Filter output" and "Filter properties" is currently entirely misleading.

Comment 1

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

Attachment: v1

This is probably much much better.

Comment 2

[Mihai Sucan [:msucan]]

Comment on attachment 749316 [details] [diff] [review]
v1

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

I'm not sure if I am the right person to review this change. I can review the code change (r+), but the UI change seems to me without strong basis.

What issue does this patch solve? And why is this new button placement better? We should probably ask shorlander for feedback. If he says this is an improvement, I am all fine with landing this patch.

Also see: http://img.i7m.de/show/tmhnb.png

Now filter properties input height is not equal the filter output input height.

Thanks!

Comment 3

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

(In reply to Mihai Sucan [:msucan] from comment #2)
> 
> I'm not sure if I am the right person to review this change. I can review
> the code change (r+), but the UI change seems to me without strong basis.
>
> What issue does this patch solve? And why is this new button placement
> better? We should probably ask shorlander for feedback. If he says this is
> an improvement, I am all fine with landing this patch.

Like I said in comment #0. I've seem people complaining about this too in #devtools. But ok, I'll ask shorlander for feedback.

> 
> Also see: http://img.i7m.de/show/tmhnb.png
> 
> Now filter properties input height is not equal the filter output input
> height.
> 

How in the world is that even possible? It used to be spacer-then-button and now it's button-then-spacer in the same container. The screenshot is empty afaict, can you please upload it again?

Comment 4

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

Attachment: before

Comment 5

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

Attachment: after

Comment 6

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

Comment on attachment 749815 [details]
after

Shorlander, quick, without testing, what does the Clear button do in the "before" screenshot? Does it remove text from the filter box (which one?) or does it clear all logging?

Now look at "after". Is the relation between between the Clear button and the logging more obvious?

Comment 7

[Mihai Sucan [:msucan]]

(In reply to Victor Porof [:vp] from comment #3)
> > Also see: http://img.i7m.de/show/tmhnb.png
> > 
> > Now filter properties input height is not equal the filter output input
> > height.
> > 
> 
> How in the world is that even possible? It used to be spacer-then-button and
> now it's button-then-spacer in the same container. The screenshot is empty
> afaict, can you please upload it again?

My bad. The height of the input fields is unchanged by the patch. Just double checked. However, the perceived difference is much greater now with the clear button moved to a different location - now the two inputs are side-by-side and they look ugly on Linux having different heights.

Comment 8

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

Attachment: v1.1

This patch also makes toolbar heights look the same in every tool.

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 749866 [details] [diff] [review]
v1.1

Screenshot with this patch: http://img.i7m.de/show/rrycj.png

The two inputs still do not have the same height. You will hate me for this, but I use non-default system font sizes. I suggest you test on Macs/Windows/Linux with non-default system font sizes (make them bigger/smaller).

Fixed pixel sizes are probably not recommended for heights and font sizes. Use percentages or ems.

I understand this bug is not about the input heights. If you want to split the work into a separate bug, please feel free.

I can r+ the patch but I am worried about the fixed pixel sizes. For example min-height 34px for the toolbar makes me question how you found that value? The content of each toolbar should give the height of the toolbar - we shouldn't hard-code a min-height - that's like a hack. To fix different heights in toolbars we should check the paddings and the toolbar content items to find what causes problem.

Did you get a hold of shorlander?

Comment 10

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

(In reply to Mihai Sucan [:msucan] from comment #9)
> 
> The two inputs still do not have the same height. You will hate me for this,
> but I use non-default system font sizes. I suggest you test on
> Macs/Windows/Linux with non-default system font sizes (make them
> bigger/smaller).

Indeed. Results vary by a lot of things (it also depends of the current system font face, and font sizes are incredibly inconsistent on linux when it comes to font faces).

> Fixed pixel sizes are probably not recommended for heights and font sizes.
> Use percentages or ems. 

That means rewriting the common devtools.css (because of things like what's going in see .devtools-sidebar-tabs > tabs > tab). Which I would certainly love.

> I understand this bug is not about the input heights. If you want to split
> the work into a separate bug, please feel free.
> 

Let's just ignore this bug until Paul comes back.

> I can r+ the patch but I am worried about the fixed pixel sizes. For example
> min-height 34px for the toolbar makes me question how you found that value?
> The content of each toolbar should give the height of the toolbar - we
> shouldn't hard-code a min-height - that's like a hack. To fix different
> heights in toolbars we should check the paddings and the toolbar content
> items to find what causes problem.
>

You're 100% right. The fixes in this patch are horrible and made out of pure desperation and general sadness. I think Paul should fix those, I'm too stupid to understand how those toolbars work :) But if I'm reading the CSS right, it seems that every .devtools-toolbar requires a .devtools-toolbarbutton inside it, to achieve a certain height. The assumption that all toolbars have toolbarbuttons is incorrect (e.g. in the VariablesView, Style Editor, NetMonitor etc.), and besides that, the height calculation itself is incredibly awkward (see .devtools-sidebar-tabs > tabs > tab) and maybe wrong (see bug 830684, which doesn't only happen on OS X).

> Did you get a hold of shorlander?

Not yet.

Comment 11

[Stephen Horlander [:shorlander]]

Comment on attachment 749815 [details]
after

I think this is much better, placing it after the input field has always been slightly confusing. It still isn't super obvious what "Clear" is referring to. Out of scope for this bug but it might be worth making it more specific with something like "Clear Log".

Comment 12

[Mihai Sucan [:msucan]]

Comment on attachment 749316 [details] [diff] [review]
v1

Thank you Stephen! That's all I needed to know.

Comment 13

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

(In reply to Mihai Sucan [:msucan] from comment #12)
> Comment on attachment 749316 [details] [diff] [review]
> v1
> 
> Thank you Stephen! That's all I needed to know.

Ok, so we're landing that then? Or waiting on Paul?

Comment 14

[Mihai Sucan [:msucan]]

(In reply to Victor Porof [:vp] from comment #13)
> (In reply to Mihai Sucan [:msucan] from comment #12)
> > Comment on attachment 749316 [details] [diff] [review]
> > v1
> > 
> > Thank you Stephen! That's all I needed to know.
> 
> Ok, so we're landing that then? Or waiting on Paul?

Unfortunately, waiting for Paul for the fixing toolbars will take a long time, and I think this patch + Rob's patch for adding the sidebar toggle button will improve the overall look. I'm not happy about the unequal input heights, but we can fix those later.

Comment 15

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

(In reply to Mihai Sucan [:msucan] from comment #14)

Okay, thanks!

Comment 16

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

Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/2da53b46658c, but with the CSS changes by mistake (v1.1), so I backed them out: https://hg.mozilla.org/integration/mozilla-inbound/rev/b2653134d1b8 leaving only the changes in v1.

Comment 17

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

This is in central but not marked as fixed yet :(
Marking:
https://hg.mozilla.org/mozilla-central/rev/2da53b46658c
https://hg.mozilla.org/mozilla-central/rev/b2653134d1b8