601667 - Web Console toolbar styling

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Kevin Dangoor]

The Web Console's checkboxes are functional, but we can likely do better with the styling.

Comment 1

[Patrick Walton (:pcwalton)]

Attachment: Screenshot.

Here's the current state on the Mac.

Currently, the buttons with dropdowns aren't functional (though the dropdowns themselves are), because I need to get an event for the toolbar button itself that *doesn't* fire when the dropdown is clicked. That means I need to add an event listener to the toolbar button part, but not to the dropdown part. Those nodes are anonymous nodes added via an XBL binding. We construct the XUL nodes dynamically, and Gecko seems to be nondeterministic as to when the XUL binding actually takes place [1]. That means that document.getAnonymousNodes() won't work, so I have no way of getting a handle to just the button part. The only solution I can think of is to avoid XBL entirely and simply manually create the dropdown, which shouldn't be too terrible, since I'm already overriding all the CSS styling anyhow.

That rambling probably made little sense: suffice it to say it's coming along :) Feedback welcome.

[1]: http://groups.google.com/group/mozilla.dev.tech.xbl/browse_thread/thread/2730b00699176c18/bfe819ab8b7fee6a

Comment 2

[Patrick Walton (:pcwalton)]

Attachment: Screenshot, version 2.

Here's a new screenshot.

 * XBL issues have been ironed out. The buttons now actually work.
 * "Clear" has been added back to the toolbar and moved to the far right so that the filter bar/clear button combo looks analogous to the search bar/bookmarks button above it.
 * "Close" has been moved to the toolbar and to the left (since that's where close goes on the Mac).

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

Since the controls have drop downs I don't think we can combine them into a single unit.  Otherwise looks great!

Comment 4

[David Dahl :ddahl]

I daresay that looks great.

Comment 5

[Patrick Walton (:pcwalton)]

Nominating to block Firefox 4, because the current series of check boxes is not only ugly, but also confusing (if it confuses bz, it's sure to confuse our users).

NB: There is a high probability that this will result in a string change: the addition of "Web Developer".

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

>NB: There is a high probability that this will result in a string change: the
>addition of "Web Developer".

We have that string in the Firefox menu, so shouldn't be an issue.

Comment 7

[Patrick Walton (:pcwalton)]

Attachment: Linux screenshot.

Here's a new Linux screenshot.

Comment 8

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1: Code changes.

Patch part 1 applies the code changes to the HUD Service.

Comment 9

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2: Mac styling.

Patch part 2 consists of the Mac changes.

Comment 10

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3: Windows styling.

Patch part 3 applies the Windows styling.

Comment 11

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4: Linux styling.

Patch part 4 applies the Linux styling. Feedback requested for all four!

Comment 12

[Patrick Walton (:pcwalton)]

Attachment: Windows screenshot.

Attached a Windows screenshot.

Comment 13

[Patrick Walton (:pcwalton)]

Just noticed that the CSS button is one pixel off on Windows...

Comment 14

[Kevin Dangoor]

cc'ing l10n. This includes a new string ("Web Developer"). Also, in the properties file "clearConsoleCmd.label" has become "btnClear" (with the string itself changing from "Clear Console" to "Clear").

The string "Web Developer" is representative of log output from the JavaScript "console" object. This button serves basically the same function as the collection of checkboxes that go with the "Console" label in the current Web Console.

Comment 15

[Mihai Sucan [:msucan]]

Comment on attachment 484469 [details] [diff] [review]
Proposed patch, part 1: Code changes.

The patch looks fine, and especially the new UI. Much improved! I like it a lot.

Comments on the code:

- In makeFilterToolbar() you are changing the association of buttons to new prefKeys. That's semi-fine - we should do this, but you've introduced at least one problem. The CSS button does no longer disable CSS warnings (errors) - please see bug 589162. Other than that, Network > Log ... doesn't do anything - I believe this is expected?

Please take a look at bug 601177 comment 16 ... and at the patch. We should try to be in sync with the changes we are making. There I point out the problems we have with the current prefKeys.

If you are making changes to prefKeys, I am glad, but we should not confuse things even further. Currently we know one thing: prefKeys are used as-is in classNames. hud-cssparser, hud-exception, etc ... Your code changes this "convention" as well.

The test for bug 589162 does not catch this regression because it doesn't click the button, it calls the setFilterState() method directly - and that works fine.


+  makeCloseButton: function HUD_makeCloseButton(aToolbar)
+  {
+    function HUD_closeButton_onCommand() {
+      let tab = this.ownerDocument.defaultView.gBrowser.selectedTab;
+      HUDService.deactivateHUDForContext(tab);
     }

Nitpicking: this always closes the Web Console from the currently selected tab. 

+  makeClearConsoleButton: function HUD_makeClearConsoleButton(aToolbar)
+  {
+    let hudId = this.hudId;
+    function HUD_clearButton_onCommand() {
+      HUDService.clearDisplay(hudId);
     }

I would prefer that to be let self = this; followed by self.jsterm.clearOutput(). The HUDService.clearDisplay() method relies on finding the Web Console again, which is prone to errors (see bug 603176).

- You are adding new strings changes. This should perhaps be a beta7 blocker.

The patch is really good, but I have to give it f- for the CSS filter regression. You have f+ with that fixed, and better prefkeys/classnames.

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 484470 [details] [diff] [review]
Proposed patch, part 2: Mac styling.

This looks good to me (tested only on Linux!). Nitpicking: almost every property has !important. Are all these really needed?

Comment 17

[Patrick Walton (:pcwalton)]

(In reply to comment #16)
> Comment on attachment 484470 [details] [diff] [review]
> Proposed patch, part 2: Mac styling.
> 
> This looks good to me (tested only on Linux!). Nitpicking: almost every
> property has !important. Are all these really needed?

I could have gone through every property and checked whether !important was needed, but this was very tedious :(

I did test each Mac property and most of them were needed.

Comment 18

[Patrick Walton (:pcwalton)]

(In reply to comment #15)
> Comment on attachment 484469 [details] [diff] [review]
> Proposed patch, part 1: Code changes.
> 
> The patch looks fine, and especially the new UI. Much improved! I like it a
> lot.
> 
> Comments on the code:
> 
> - In makeFilterToolbar() you are changing the association of buttons to new
> prefKeys. That's semi-fine - we should do this, but you've introduced at least
> one problem. The CSS button does no longer disable CSS warnings (errors) -
> please see bug 589162. Other than that, Network > Log ... doesn't do anything -
> I believe this is expected?

The CSS is a bug, working on it.

> Please take a look at bug 601177 comment 16 ... and at the patch. We should try
> to be in sync with the changes we are making. There I point out the problems we
> have with the current prefKeys.

The only differences between this patch and your proposal are:
 * Network requests that aren't errors are logged at the log level. This is because "info" messages have a little "i" icon, and I thought it would be distracting to see this for each message.
 * Exceptions and errors being logged at the wrong level seems to me to be out of scope for this bug.
 * Per bz's comments following your proposal, I've combined "Errors" and "Exceptions".

> If you are making changes to prefKeys, I am glad, but we should not confuse
> things even further. Currently we know one thing: prefKeys are used as-is in
> classNames. hud-cssparser, hud-exception, etc ... Your code changes this
> "convention" as well.

I don't see how it does. The HUD messages are still given CSS classes in that format. See sendToHUDService() for error/warn/info/log, for example.

> The test for bug 589162 does not catch this regression because it doesn't click
> the button, it calls the setFilterState() method directly - and that works
> fine.

The CSS regression, you mean?

Comment 19

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 0: String changes.

Patch part 0 splits out the string changes, as requested by beltzner.

Comment 20

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 2: Code changes.

New patch fixes the CSS issue, thanks for pointing it out.

Comment 21

[Mihai Sucan [:msucan]]

Comment on attachment 484850 [details] [diff] [review]
Proposed patch, part 0: String changes.

These changes look fine. However the comments say "this *will be used*" ... I think they should say "This is used". Additionally mentioning a new feature (the restyled toolbar) might be confusing. Strings and code always refer to a new thing - either a bug fix, a new feature, a reimplemented something, a redesigned part, etc. Comments should only refer to the "current state" of things.

Comment 22

[Axel Hecht [:Pike]]

Comment on attachment 484850 [details] [diff] [review]
Proposed patch, part 0: String changes.

Nit-picking on the comments here. "restyling" isn't going to make any sense in a year from now, comments should really be describing the end result, and not the difference from what was, in general.

AFAICT, this is about web pages that do console.log ?

I'm confused to attribute them to the developer and not the page, but that's just me. It might be good to open this up for localization to choose between person and page, as different cultures handle references to individuals rather differently. In the end, it's not like a web developer is sending you SMS text messages or tweets into the browser ;-)

Comment 23

[Mihai Sucan [:msucan]]

Comment on attachment 484921 [details] [diff] [review]
Proposed patch, part 1, version 2: Code changes.

Indeed, the CSS filter regression is fixed.

Comment 24

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 0, version 2: String changes (checked in)

New patch changes the l10n notes. Review requested.

Comment 25

[Axel Hecht [:Pike]]

Comment on attachment 485096 [details] [diff] [review]
Proposed patch, part 0, version 2: String changes (checked in)

beautiful, thanks.

Comment 26

[Patrick Walton (:pcwalton)]

Comment on attachment 485096 [details] [diff] [review]
Proposed patch, part 0, version 2: String changes (checked in)

Requesting approval for beta7 for the first patch, so that we don't have to break string freeze.

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 484473 [details] [diff] [review]
Proposed patch, part 3: Windows styling.

Please use two-space indentation.

You're using !important almost everywhere -- please rip them all out and add them only where needed.

Avoid the descendant selector: https://developer.mozilla.org/en/Writing_Efficient_CSS#Avoid_the_descendant_selector

>+.hud-filter-button:not([type="menu-button"]) {
>+    padding: 3px 0px 1px 0px !important;

nit: write this as 'padding: 3px 0 1px;'

>+.hud-filter-button .toolbarbutton-text {
>+    padding-left: 12px !important;
>+}

This should probably be -moz-padding-start.

>+/* Close button, based on findBar.css. */
>+.hud-close-button {
>+    border: 1px solid transparent !important;
>+    padding: 4px !important;
>+    background: -moz-image-rect(url("chrome://global/skin/icons/close.png"),
>+        0, 16, 16, 0) 0px 0px no-repeat !important;
>+    margin-bottom: 3px !important;
>+    -moz-margin-start: 4px !important;
>+    -moz-appearance: none !important;
>+}
>+
>+.hud-close-button:hover {
>+    background: -moz-image-rect(url("chrome://global/skin/icons/close.png"),
>+        0, 32, 16, 16) 0px 0px no-repeat !important;
>+}
>+
>+.hud-close-button:hover:active {
>+    background: -moz-image-rect(url("chrome://global/skin/icons/close.png"),
>+        0, 48, 16, 32) 0px 0px no-repeat !important;
> }

Why are you using background instead of list-style-image as used for .jsterm-close-button?

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 484474 [details] [diff] [review]
Proposed patch, part 4: Linux styling.

The comments on the Windows patch apply here too.

This looks completely broken:

>+.hud-close-button .toolbarbutton-text {
>+	background: url("moz-icon://stock/gtk-close?size=menu") 0px 0px no-repeat
>+		!important;
>+    padding: 8px !important;
>+    margin: 0px !important;
>+}
>+
>+.hud-close-button:hover {
>+	background: -moz-image-rect(url("chrome://global/skin/icons/close.png"),
>+		0, 32, 16, 16) 0px 0px no-repeat !important;
>+}
>+
>+.hud-close-button:hover:active {
>+	background: -moz-image-rect(url("chrome://global/skin/icons/close.png"),
>+		0, 48, 16, 32) 0px 0px no-repeat !important;
>+}

Comment 29

[Dão Gottwald [:dao]]

Comment on attachment 484470 [details] [diff] [review]
Proposed patch, part 2: Mac styling.

Most of the comments on the Windows patch apply here too.

Many of the margin / padding uses here don't seem to be ready for right-to-left UI; make sure to use -moz-margin/padding-start/end.

>+.hud-filter-button, .hud-clear-console-button {

nit: break the line after the comma

>-.jsterm-close-button {
>-    list-style-image: url("chrome://global/skin/icons/console-close.png");

This makes console-close.png unused, please remove the file.

Comment 30

[Patrick Walton (:pcwalton)]

(In reply to comment #27)
> Comment on attachment 484473 [details] [diff] [review]
> Proposed patch, part 3: Windows styling.
> 
> Please use two-space indentation.

All the files use four-space indentation. Should I reformat the whole file or switch the bits I'm using to two-space indentation (making it inconsistent)?

Comment 31

[Patrick Walton (:pcwalton)]

The reason we use !important so much is that we load the style sheet via the nsIStyleSheetService as an agent sheet. This places the Web Console's style sheet at the bottom of the CSS cascade and leads to an explosion of !important. There are two options here:

(a) We could add the Web Console style sheet to browser.xul.
(b) We could insert the Console's style sheet programmatically by adding an @import rule like 

    let styleSheets = chromeDocument.styleSheets;
    let firstStyleSheet = styleSheets.item(styleSheets.length - 1);
    firstStyleSheet.insertRule("@import url(" + HUD_STYLESHEET_URI + ")", 0);

This would place the Console style sheet near the top of the CSS cascade. Based on testing it seems that this could eliminate every usage of !important cluttering up our style sheet.

Thoughts on this approach?

Comment 32

[Rob Campbell [:rc] (:robcee)]

cc'ing gavin for input as I do remember some issues with web console styling overriding chunks of browser.xul styling around the title bars.

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #30)
> (In reply to comment #27)
> > Comment on attachment 484473 [details] [diff] [review] [details]
> > Proposed patch, part 3: Windows styling.
> > 
> > Please use two-space indentation.
> 
> All the files use four-space indentation. Should I reformat the whole file or
> switch the bits I'm using to two-space indentation (making it inconsistent)?

I fixed this here: http://hg.mozilla.org/mozilla-central/rev/7e51b7e79704

(In reply to comment #31)
> The reason we use !important so much is that we load the style sheet via the
> nsIStyleSheetService as an agent sheet. This places the Web Console's style
> sheet at the bottom of the CSS cascade and leads to an explosion of !important.
> There are two options here:
> 
> (a) We could add the Web Console style sheet to browser.xul.

This makes sense to me, but before we put the stylesheet in browser.xul by default, it needs to get overhauled according to <https://developer.mozilla.org/en/Writing_Efficient_CSS>. ".hud-output-node *" seems very much unacceptable.

Comment 34

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 2: Mac styling.

Mac styling patch updated per review.

Comment 35

[David Dahl :ddahl]

Pushed part 0: http://hg.mozilla.org/mozilla-central/rev/df7ea854a68c

Comment 36

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 3: Code changes.

Patch part 1, version 3 changes the toolbar type from "text" to "full", which simplifies the CSS.

Comment 37

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 3: Mac styling.

Patch part 2, version 3 makes the Mac styling simpler by using list-style-image instead of background hacks.

Comment 38

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 2: Windows styling.

Patch part 3, version 2 updates the Windows styling per review.

Comment 39

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 2: Linux styling.

Patch part 4, version 2 updates the Linux styling per review.

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 486528 [details] [diff] [review]
Proposed patch, part 1, version 3: Code changes.

>+    let closeButton = this.makeXULNode("button");
>+    closeButton.classList.add("hud-close-button");
>+    closeButton.addEventListener("command", HUD_closeButton_onCommand, false);
>+
>+    aToolbar.appendChild(closeButton);


Why is this a button rather than a toolbarbutton?

Comment 41

[Patrick Walton (:pcwalton)]

(In reply to comment #40)
> Why is this a button rather than a toolbarbutton?

That was needed to make the icon show up when the toolbar was set to text mode. It can probably be changed back to a toolbarbutton now that the toolbar is in icons-and-text mode.

Comment 42

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 4: Code changes.

Patch part 1, version 4 makes the close button a toolbarbutton instead of a button.

Comment 43

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 4: Mac styling.

Patch part 2, version 4 updates the Mac styling wrt the toolbarbutton change and also fixes an issue whereby the "hover" state for the close button was overriding the "active" state.

Comment 44

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 3: Windows styling.

Patch part 3, version 3 updates the Windows styling wrt the toolbarbutton change and fixes an off-by-one-pixel error in the close button styling.

Comment 45

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 3: Linux styling.

Patch part 4, version 3 updates the Linux styling wrt the toolbarbutton change.

Comment 46

[Dão Gottwald [:dao]]

Comment on attachment 486697 [details] [diff] [review]
Proposed patch, part 4, version 3: Linux styling.

This appears to be identical to the previous version of this patch.

Comment 47

[Patrick Walton (:pcwalton)]

(In reply to comment #46)
> Comment on attachment 486697 [details] [diff] [review]
> Proposed patch, part 4, version 3: Linux styling.
> 
> This appears to be identical to the previous version of this patch.

Err, right, it is. I didn't make any changes because it looked fine; I just forgot that I hadn't made any changes :)

Comment 48

[Dão Gottwald [:dao]]

Comment on attachment 486697 [details] [diff] [review]
Proposed patch, part 4, version 3: Linux styling.

Well, but I expected changes. At the very least, -moz-appearance: toolbarbutton shouldn't be needed anymore:

>+.hud-close-button {
>+  min-width: 0px;
>+  width: 28px;
>+  margin-top: 1px;
>+  margin-bottom: 1px;
>+  -moz-margin-start: 3px;
>+  -moz-margin-end: 0px;
>+  list-style-image: url("moz-icon://stock/gtk-close?size=menu");
>+  -moz-appearance: toolbarbutton;

Comment 49

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 4: Linux styling.

Patch part 4, version 4 removes the -moz-appearance: toolbarbutton from the Linux styling.

Comment 50

[Kevin Dangoor]

The string change has been checked in and the rest of the work can wait past beta7.

Re-requesting blocking for this. This change to the toolbar is not just polish. It makes the toolbar far more functional, making the filtering categories more understandable and the clear function discoverable.

Comment 51

[Dão Gottwald [:dao]]

Comment on attachment 486716 [details] [diff] [review]
Proposed patch, part 4, version 4: Linux styling.

>+.hud-filter-button > toolbarbutton,

This doesn't match anything, does it?

>+/* Network button */
>+.hud-filter-button-net {
>+  -moz-image-region: rect(0px, 40px, 10px, 30px);
>+}
>+
>+/* CSS button */
>+.hud-filter-button-css {
>+  -moz-image-region: rect(10px, 40px, 20px, 30px);
>+}

Could you use an attribute for the "net", "css" etc. distinction?
E.g. .hud-filter-button[category="net"]

I'd also prefer it if we didn't introduce new classes using the "hud" prefix. We should use webConsole or webconsole instead.

Comment 52

[Johnathan Nightingale [:johnath]]

How does this bug play against bug 599498? They both seem to involve UI styling changes. Is this one toolbar only and that one content only?

Comment 53

[Kevin Dangoor]

This bug is one piece of the work (probably the most significant) for the overall of bug 599498. bug 599498 is actually a meta bug, generally covering landing of toolbar and output area restyling (bug 605621).

Comment 54

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 5: Code changes.

Patch part 1, version 5 changes hud- to webconsole- and uses attributes instead of CSS class names to style the individual buttons.

Comment 55

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 5: Mac styling.

Patch part 2, version 5 makes the reviewer-requested changes to the Mac styling.

Comment 56

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 4: Windows styling.

Patch part 3, version 4 makes the reviewer-requested changes to the Windows styling.

Comment 57

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 5: Linux styling.

Patch part 4, version 5 makes the reviewer-requested changes to the Linux styling.

Comment 58

[Patrick Walton (:pcwalton)]

(In reply to comment #51)
> Comment on attachment 486716 [details] [diff] [review]
> Proposed patch, part 4, version 4: Linux styling.
> 
> >+.hud-filter-button > toolbarbutton,
> 
> This doesn't match anything, does it?

Toolbarbuttons of type "menu-button" have another toolbarbutton embedded inside [1].

[1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/toolbarbutton.xml#56

Comment 59

[Dão Gottwald [:dao]]

(In reply to comment #58)
> (In reply to comment #51)
> > Comment on attachment 486716 [details] [diff] [review] [details]
> > Proposed patch, part 4, version 4: Linux styling.
> > 
> > >+.hud-filter-button > toolbarbutton,
> > 
> > This doesn't match anything, does it?
> 
> Toolbarbuttons of type "menu-button" have another toolbarbutton embedded inside

Ah! Please use the toolbarbutton-menubutton-button class then.

Comment 60

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 6: Mac styling.

Patch part 2, version 6 replaces toolbarbutton with .toolbarbutton-menubutton-button in the Mac style sheet.

Comment 61

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 5: Windows styling.

Patch part 3, version 5 replaces toolbarbutton with .toolbarbutton-menubutton-button in the Windows style sheet.

Comment 62

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 6: Linux styling.

Patch part 4, version 6 replaces toolbarbutton with .toolbarbutton-menubutton-button in the Linux style sheet.

Comment 63

[Shawn Wilsher :sdwilsh]

Comment on attachment 487668 [details] [diff] [review]
Proposed patch, part 1, version 5: Code changes.

>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>+    const buttons = [
usually constants are prefixed with a k or are in all caps.  Pick one and run with it.

There are a lot of UI logic changes here.  Do we really have test coverage on all this already?

Comment 64

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 6: Code changes.

Patch part 1, version 6 fixes the spelling of "BUTTONS" and adds some 80 new tests.

Comment 65

[Steve Roussey (:sroussey)]

I started to follow this bug since I thought the first screenshot was possible in XUL. I am to take it that it is not possible, which is what led to third screenshot?

Comment 66

[Shawn Wilsher :sdwilsh]

Comment on attachment 487772 [details] [diff] [review]
Proposed patch, part 1, version 6: Code changes.

My review queue has some big stuff in it at the moment, so I'm going to have ddahl do a feedback cycle on this first (since a bunch of new tests were added).  Please request review from me again once you've addressed any comments from his feedback.

Comment 67

[Patrick Walton (:pcwalton)]

(In reply to comment #65)
> I started to follow this bug since I thought the first screenshot was possible
> in XUL. I am to take it that it is not possible, which is what led to third
> screenshot?

No, it was a real screenshot made with styled XUL. It was changed for UX reasons: see comment 3.

Comment 68

[David Dahl :ddahl]

Comment on attachment 487772 [details] [diff] [review]
Proposed patch, part 1, version 6: Code changes.


>+  /**
>+   * The event handler that is called whenever a user switches a filter on or
>+   * off.
>+   *
>+   * @returns boolean
>+   */
>+  toggleFilter: function UIC_toggleFilter(ev) {
>+    let hudId = this.getAttribute("hudId");
>+    switch (this.tagName) {
>+      case "toolbarbutton": {
>+        let originalTarget = ev.originalTarget;

nit: ev should be 'aEvent' to conform to style guidelines. f+ with that change.

Comment 69

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 7: Code changes.

Patch part 1 version 7 addresses the feedback comments. Review requested.

Comment 70

[Shawn Wilsher :sdwilsh]

Note: comment 63 has not been fully addressed in the latest patch

Comment 71

[Patrick Walton (:pcwalton)]

(In reply to comment #70)
> Note: comment 63 has not been fully addressed in the latest patch

What else did you want to see in particular?

Comment 72

[Shawn Wilsher :sdwilsh]

er, my bad.  Looked at the wrong version it would seem.

Comment 73

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 8: Code changes.

Patch part 1, version 8 rebases to trunk.

Comment 74

[Dão Gottwald [:dao]]

Comment on attachment 487711 [details] [diff] [review]
Proposed patch, part 4, version 6: Linux styling.

>+.webconsole-filter-button {
>+  -moz-margin-start: 6px;
>+}
>+
>+.webconsole-clear-console-button {
>+  -moz-margin-end: 6px;
>+  -moz-margin-start: 6px;
>+}
>+
>+.webconsole-filter-button > .toolbarbutton-text,
>+.webconsole-clear-console-button > .toolbarbutton-text {
>+  -moz-padding-end: 3px;
>+}
>+
>+.webconsole-filter-button:not([type="menu-button"]) {
>+  padding: 0px 3px;
>+}

>+/* Close button, based on findBar.css. */
>+.webconsole-close-button {
>+  min-width: 0px;
>+  width: 28px;
>+  margin-top: 1px;
>+  margin-bottom: 1px;
>+  -moz-margin-start: 3px;
>+  -moz-margin-end: 0px;
>+  list-style-image: url("moz-icon://stock/gtk-close?size=menu");
> }

It seems that each of these margin and padding adjustments should be dropped -- native toolbar buttons usually don't have margin and the default styling should work just fine, except for the horizontal alignment in the clear button, for which you should add this:

.webconsole-clear-console-button > .toolbarbutton-icon {
  display: none;
}

These lines also appear to be no-ops:

>+.webconsole-filter-button:not([type="menu-button"]) {
>+  padding: 0px 3px;

>+/* Close button, based on findBar.css. */
>+.webconsole-close-button {
>+  min-width: 0px;
>+  width: 28px;

Comment 75

[Patrick Walton (:pcwalton)]

Attachment: Linux screenshot without padding.

Here's a screenshot of what the toolbar looks like on Linux without the padding.

Comment 76

[Patrick Walton (:pcwalton)]

(In reply to comment #74)
> It seems that each of these margin and padding adjustments should be dropped --
> native toolbar buttons usually don't have margin and the default styling should
> work just fine, except for the horizontal alignment in the clear button, for
> which you should add this:

To me the buttons look a little too close together without the margin and padding. (See screenshot.) Also, the clear button looks slightly off, even with the "display: none" change you suggested.

Thoughts?

Comment 77

[Dão Gottwald [:dao]]

(In reply to comment #76)
> (In reply to comment #74)
> > It seems that each of these margin and padding adjustments should be dropped --
> > native toolbar buttons usually don't have margin and the default styling should
> > work just fine, except for the horizontal alignment in the clear button, for
> > which you should add this:
> 
> To me the buttons look a little too close together without the margin and
> padding. (See screenshot.) Also, the clear button looks slightly off, even with
> the "display: none" change you suggested.

You mean the space left to the Clear button? That's the margin of the textbox, which seems right (at least consistent with other toolbars with buttons and textboxes). Adding the same margin on the right of the button would make it harder to hit the button (cf. bug 338443).

The inside of the button is evenly spaced, as far as I can see.

Comment 78

[Patrick Walton (:pcwalton)]

(In reply to comment #77)
> (In reply to comment #76)
> > (In reply to comment #74)
> > > It seems that each of these margin and padding adjustments should be dropped --
> > > native toolbar buttons usually don't have margin and the default styling should
> > > work just fine, except for the horizontal alignment in the clear button, for
> > > which you should add this:
> > 
> > To me the buttons look a little too close together without the margin and
> > padding. (See screenshot.) Also, the clear button looks slightly off, even with
> > the "display: none" change you suggested.
> 
> You mean the space left to the Clear button? That's the margin of the textbox,
> which seems right (at least consistent with other toolbars with buttons and
> textboxes). Adding the same margin on the right of the button would make it
> harder to hit the button (cf. bug 338443).

Oh no, I just mean the vertical height of the clear button compared to the vertical height of the text box next to it. The clear button sticks out a little more to the bottom.

> The inside of the button is evenly spaced, as far as I can see.

Yup, I agree.

Also, what do you think about the spacing between the Net/CSS/JS/Web Developer buttons in that screenshot (attachment 490236 [details])? Good, or too narrow? I think it's a little narrow, but my feelings aren't that strong either way, now that I look at it again.

Comment 79

[Dão Gottwald [:dao]]

(In reply to comment #78)
> Oh no, I just mean the vertical height of the clear button compared to the
> vertical height of the text box next to it. The clear button sticks out a
> little more to the bottom.

Could this be due to -moz-box-align: center on the toolbar? What happens if you remove that?

> Also, what do you think about the spacing between the Net/CSS/JS/Web Developer
> buttons in that screenshot (attachment 490236 [details])? Good, or too narrow? I think
> it's a little narrow, but my feelings aren't that strong either way, now that I
> look at it again.

Looks okay to me.

Comment 80

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 9: Code changes.

Patch part 1, version 9 rebases to trunk.

Comment 81

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 7: Linux styling.

Patch part 4, version 7 addresses review comments for the Linux styling.

Comment 82

[Dão Gottwald [:dao]]

Comment on attachment 491057 [details] [diff] [review]
Proposed patch, part 4, version 7: Linux styling.

>+.hud-group:first-child > .hud-divider {
>+  display: none;
> }

Is this dead? I failed finding a "hud-divider".

>+/* Close button, based on findBar.css. */

I think this can go away. You're just using the same generic icon as the find bar.

Comment 83

[Axel Hecht [:Pike]]

Shawn, Dao, as this is a bug with pre-landed strings, can I offer a cookie for the reviews so that we get those strings in action? I'd really like to close our string bugs out.

Comment 84

[Shawn Wilsher :sdwilsh]

Comment on attachment 491048 [details] [diff] [review]
Proposed patch, part 1, version 9: Code changes.

globally: s/@returns/@return/

Sorry this took so long.  r=sdwilsh

Comment 85

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 10: Code changes.

Proposed patch, part 1, version 10 addresses review comments.

Comment 86

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 7: Mac styling.

Proposed patch, part 2, version 7 removes the unused ".hud-divider" rule from the Mac styling.

Comment 87

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 6: Windows styling.

Patch part 3, version 6 removes the unused ".hud-divider" rule from the Windows CSS.

Comment 88

[Patrick Walton (:pcwalton)]

Attachment: [checked-in] Proposed patch, part 4, version 8: Linux styling.

Proposed patch, part 4, version 8 addresses review comments for the Linux CSS.

Comment 89

[Patrick Walton (:pcwalton)]

NB: The code changes can't be checked in until all the CSS patches have r+, because they completely change the CSS classes used for the Console and would cause the output to become completely broken.

Comment 90

[Dão Gottwald [:dao]]

Comment on attachment 491709 [details] [diff] [review]
Proposed patch, part 2, version 7: Mac styling.

>+.webconsole-filter-button > .toolbarbutton-menubutton-button >
>+    .toolbarbutton-text {

>+.webconsole-filter-button > .toolbarbutton-menubutton-button >
>+    .toolbarbutton-text {

>+.webconsole-filter-button > .toolbarbutton-menubutton-button >
>+    .toolbarbutton-icon {

nit: remove the line breaks.

Also, replace 0px with 0.

Comment 91

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 92

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 93

[Kevin Dangoor]

mass change: filter mail on BLOCKERSETTING

Comment 94

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 11: Code changes.

Patch part 1, version 11 rebases to trunk.

Comment 95

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 8: Mac styling.

Patch part 2, version 8 rebases to trunk.

Dão, do you have an ETA on a review for the Windows styling by any chance?

Comment 96

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 9: Mac styling.

Patch part 2, version 9 addresses review comments.

Comment 97

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 7: Windows styling.

Patch part 3, version 7 replaces 0px with 0.

Comment 98

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 12: Code changes.

I realized that Gecko emits both CSS errors and warnings, and so we need to give the user the ability to toggle both on or off. Patch part 1, version 12 implements this. Re-review requested.

Comment 99

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 10: Mac styling.

Proposed patch, part 2, version 10 makes the changes needed to add a dropdown to the CSS button on the Mac. Re-review requested.

Comment 100

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 13: Code changes.

Patch part 1, version 13 adds "jswarn" to the updateDefaultDisplayPrefs() method.

Comment 101

[Patrick Walton (:pcwalton)]

Attachment: [checked-in] Proposed patch, part 1, version 14: Code changes.

Patch part 1, version 14 fixes a typo in the jswarn line.

Comment 102

[Dão Gottwald [:dao]]

Comment on attachment 492874 [details] [diff] [review]
Proposed patch, part 2, version 10: Mac styling.

>+/* .webconsole-filter-button > .toolbarbutton-icon, */

What's this?

Comment 103

[Dão Gottwald [:dao]]

Comment on attachment 492738 [details] [diff] [review]
Proposed patch, part 3, version 7: Windows styling.

I've yet to see this in action, but it seems like most of the comments made on the Linux patch apply here as well.

Comment 104

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 11: Mac styling.

Sorry, I let a comment sneak in there. Patch part 2, version 11 removes it.

Comment 105

[Patrick Walton (:pcwalton)]

Attachment: Windows screenshot without margins and padding.

Unfortunately, the Windows toolbar looks like the attached screenshot if I make the same changes I made to the Linux version (removing margins). The Clear button is misaligned, and mousing over the "X" close button results in the standard toolbar appearance, which is wrong on Windows. (Note that findBar.css styles this manually.) The filter buttons are also all squished together.

Comment 106

[Rob Campbell [:rc] (:robcee)]

Patrick, the screenshot's got the wrong encoding on it. (Says it's a patch, not a png).

Comment 107

[Patrick Walton (:pcwalton)]

(In reply to comment #106)
> Patrick, the screenshot's got the wrong encoding on it. (Says it's a patch, not
> a png).

Fixed.

Comment 108

[Dão Gottwald [:dao]]

Comment on attachment 493040 [details] [diff] [review]
Proposed patch, part 2, version 11: Mac styling.

>+.webconsole-filter-button:not([type="menu-button"]) {

>+.webconsole-clear-console-button > .toolbarbutton-text {
>+  margin: 0;
>+}

Are these obsolete now?

>+.webconsole-filter-button:active,
>+.webconsole-clear-console-button:active {

>+.webconsole-close-button:active {

s/:active/:hover:active

Comment 109

[Dão Gottwald [:dao]]

Comment on attachment 492738 [details] [diff] [review]
Proposed patch, part 3, version 7: Windows styling.

>+.webconsole-filter-button > .toolbarbutton-text,

>+.webconsole-filter-button:not([type="menu-button"]) {

Same question as for pinstripe.

>+.webconsole-close-button {
>+  border: 1px solid transparent;

What's the point of this border?

>+  min-width: 0;

no-op

>+  margin-top: 1px;
>+  margin-bottom: 1px;
>+  -moz-margin-start: 3px;
>+  -moz-margin-end: 0;

Similar to findBar.css, the button should touch the edge of the toolbar, i.e. you should use padding instead of margin in order to add space.

>+.webconsole-close-button:hover:active {

>+  margin-top: -1px;
>+  -moz-margin-start: 2px;
>+  -moz-margin-end: 1px;

I don't think this is supposed to happen, the icon shouldn't move when pressing the button.

Comment 110

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 12: Mac styling.

New version of the Mac styling patch removes the following useless selectors:

* .webconsole-clear-console-button > .toolbarbutton-menubutton-button
* .webconsole-filter-button:not([type="menu-button"])

The ".webconsole-clear-console-button > .toolbarbutton-text" selector is actually used, but it looked like it wasn't due to the useless ".webconsole-clear-console-button > .toolbarbutton-menubutton-button" selector. The clear console button isn't a menu button.

Comment 111

[Patrick Walton (:pcwalton)]

(In reply to comment #109)
> >+.webconsole-close-button {
> >+  border: 1px solid transparent;
> 
> What's the point of this border?

I'm not sure. I copied it from findBar.css. Should I be doing something different?

Comment 112

[Patrick Walton (:pcwalton)]

(In reply to comment #111)
> (In reply to comment #109)
> > >+.webconsole-close-button {
> > >+  border: 1px solid transparent;
> > 
> > What's the point of this border?
> 
> I'm not sure. I copied it from findBar.css. Should I be doing something
> different?

To clarify: By default, there's a border that needs to be suppressed. As to why it's not border: none, I'm not sure.

Comment 113

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 8: Windows styling.

Updated the Windows styling per review. I made the close button styling identical to that in findBar.css.

Comment 114

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 13: Mac styling.

Patch part 2 version 13 removes a few more useless selectors.

Comment 115

[Dão Gottwald [:dao]]

(In reply to comment #112)
> > I'm not sure. I copied it from findBar.css. Should I be doing something
> > different?
> 
> To clarify: By default, there's a border that needs to be suppressed. As to why
> it's not border: none, I'm not sure.

The border seems to act as an extra padding. So instead of that, the real padding should be increased and the border should be "none". The !important flag for the padding shouldn't be needed either.

Bogus code doesn't become less bogus just because it exists elsewhere ;-) ... In fact, I would appreciate it if you could make said changes in findBar.css as well.

Comment 116

[Dão Gottwald [:dao]]

Comment on attachment 493877 [details] [diff] [review]
Proposed patch, part 2, version 13: Mac styling.

>+.webconsole-filter-button > .toolbarbutton-menubutton-button > .toolbarbutton-text {
>+  margin-top: 0;
>+  margin-bottom: 0;
>+  -moz-margin-start: 3px;
>+  -moz-margin-end: 3px;

margin: 0 3px;

Comment 117

[Patrick Walton (:pcwalton)]

Attachment: [checked-in] Proposed patch, part 2, version 14: Mac styling.

Proposed patch, part 2, version 14 addresses the review comment.

Comment 118

[Patrick Walton (:pcwalton)]

Attachment: [checked-in] Proposed patch, part 3, version 9: Windows styling.

Patch part 3, version 9 removes the transparent border.

Comment 119

[Patrick Walton (:pcwalton)]

(In reply to comment #115)
> (In reply to comment #112)
> > > I'm not sure. I copied it from findBar.css. Should I be doing something
> > > different?
> > 
> > To clarify: By default, there's a border that needs to be suppressed. As to why
> > it's not border: none, I'm not sure.
> 
> The border seems to act as an extra padding. So instead of that, the real
> padding should be increased and the border should be "none". The !important
> flag for the padding shouldn't be needed either.
> 
> Bogus code doesn't become less bogus just because it exists elsewhere ;-) ...
> In fact, I would appreciate it if you could make said changes in findBar.css as
> well.

Bug 615735 filed.

Comment 120

[Kevin Dangoor]

Comment on attachment 494098 [details] [diff] [review]
[checked-in] Proposed patch, part 2, version 14: Mac styling.

carrying forward r+

Comment 121

[Patrick Walton (:pcwalton)]

Attachment: Interdiff for patch part 1, version 9 to version 14.

Interdiff attached.

Comment 122

[Shawn Wilsher :sdwilsh]

Comment on attachment 492880 [details] [diff] [review]
[checked-in] Proposed patch, part 1, version 14: Code changes.

I don't feel like I need to review these changes.  Feel free to land.

Comment 123

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 492880 [details] [diff] [review]
[checked-in] Proposed patch, part 1, version 14: Code changes.

http://hg.mozilla.org/mozilla-central/rev/d48937dcda74

Comment 124

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 494098 [details] [diff] [review]
[checked-in] Proposed patch, part 2, version 14: Mac styling.

http://hg.mozilla.org/mozilla-central/rev/841893070e47

Comment 125

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 494148 [details] [diff] [review]
[checked-in] Proposed patch, part 3, version 9: Windows styling.

http://hg.mozilla.org/mozilla-central/rev/5a61fc1bd8f0

Comment 126

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 491713 [details] [diff] [review]
[checked-in] Proposed patch, part 4, version 8: Linux styling.

http://hg.mozilla.org/mozilla-central/rev/a39d56b54623