Status

defect
P1
normal
RESOLVED FIXED
9 years ago
a year ago

People

(Reporter: dangoor, Assigned: pcwalton)

Tracking

({polish})

unspecified
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [strings-checked-in] [patchclean:1130] )

Attachments

(11 attachments, 43 obsolete attachments)

47.28 KB, image/png
Details
55.95 KB, image/png
Details
33.83 KB, image/png
Details
2.08 KB, patch
Pike
: review+
Details | Diff | Splinter Review
66.45 KB, image/png
Details
4.96 KB, patch
Details | Diff | Splinter Review
25.84 KB, patch
Details | Diff | Splinter Review
32.31 KB, image/png
Details
8.85 KB, patch
dangoor
: review+
Details | Diff | Splinter Review
5.21 KB, patch
dao
: review+
Details | Diff | Splinter Review
33.25 KB, patch
Details | Diff | Splinter Review
Reporter

Description

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

Updated

9 years ago
Assignee: nobody → pwalton
Assignee

Comment 1

9 years ago
Posted image Screenshot. (obsolete) —
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
Assignee

Comment 2

9 years ago
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).
Attachment #481740 - Attachment is obsolete: true
Since the controls have drop downs I don't think we can combine them into a single unit.  Otherwise looks great!

Comment 4

9 years ago
I daresay that looks great.
Assignee

Comment 5

9 years ago
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".
Status: NEW → ASSIGNED
Assignee

Updated

9 years ago
blocking2.0: --- → ?
>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.
Assignee

Comment 7

9 years ago
Posted image Linux screenshot.
Here's a new Linux screenshot.
Assignee

Comment 8

9 years ago
Patch part 1 applies the code changes to the HUD Service.
Attachment #484469 - Flags: feedback?(mihai.sucan)
Assignee

Updated

9 years ago
Attachment #484469 - Attachment is patch: true
Attachment #484469 - Attachment mime type: application/octet-stream → text/plain
Assignee

Comment 9

9 years ago
Patch part 2 consists of the Mac changes.
Attachment #484470 - Flags: feedback?(mihai.sucan)
Assignee

Comment 10

9 years ago
Patch part 3 applies the Windows styling.
Attachment #484473 - Flags: feedback?(mihai.sucan)
Assignee

Comment 11

9 years ago
Patch part 4 applies the Linux styling. Feedback requested for all four!
Attachment #484474 - Flags: feedback?(mihai.sucan)
Assignee

Updated

9 years ago
OS: Mac OS X → All
Hardware: x86 → All
Assignee

Updated

9 years ago
Blocks: 601179
Assignee

Updated

9 years ago
Blocks: 601196
Assignee

Comment 12

9 years ago
Posted image Windows screenshot.
Attached a Windows screenshot.
Assignee

Comment 13

9 years ago
Just noticed that the CSS button is one pixel off on Windows...
Reporter

Comment 14

9 years ago
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.
Whiteboard: [strings]
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.
Attachment #484469 - Flags: feedback?(mihai.sucan) → feedback-
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?
Attachment #484470 - Flags: feedback?(mihai.sucan) → feedback+
Attachment #484473 - Flags: feedback?(mihai.sucan) → feedback+
Attachment #484474 - Flags: feedback?(mihai.sucan) → feedback+
Assignee

Comment 17

9 years ago
(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.
Assignee

Comment 18

9 years ago
(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?
Assignee

Comment 19

9 years ago
Patch part 0 splits out the string changes, as requested by beltzner.
Attachment #484850 - Flags: feedback?(mihai.sucan)
Assignee

Comment 20

9 years ago
New patch fixes the CSS issue, thanks for pointing it out.
Attachment #484469 - Attachment is obsolete: true
Attachment #484921 - Flags: feedback?(mihai.sucan)
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.
Attachment #484850 - Flags: feedback?(mihai.sucan) → feedback+
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 on attachment 484921 [details] [diff] [review]
Proposed patch, part 1, version 2: Code changes.

Indeed, the CSS filter regression is fixed.
Attachment #484921 - Flags: feedback?(mihai.sucan) → feedback+
Assignee

Comment 24

9 years ago
New patch changes the l10n notes. Review requested.
Attachment #484850 - Attachment is obsolete: true
Attachment #485096 - Flags: review?(community)
Assignee

Updated

9 years ago
Attachment #485096 - Flags: review?(community) → review?(l10n)
Assignee

Updated

9 years ago
Attachment #484921 - Flags: review?(gavin.sharp)
Assignee

Updated

9 years ago
Attachment #484470 - Flags: review?(dao)
Assignee

Updated

9 years ago
Attachment #484473 - Flags: review?(dao)
Assignee

Updated

9 years ago
Attachment #484474 - Flags: review?(dao)
Comment on attachment 485096 [details] [diff] [review]
Proposed patch, part 0, version 2: String changes (checked in)

beautiful, thanks.
Attachment #485096 - Flags: review?(l10n) → review+
Assignee

Comment 26

9 years ago
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.
Attachment #485096 - Flags: approval2.0?
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?
Attachment #484473 - Flags: review?(dao) → review-
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;
>+}
Attachment #484474 - Flags: review?(dao) → review-
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.
Attachment #484470 - Flags: review?(dao) → review-
Assignee

Comment 30

9 years ago
(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)?
Assignee

Comment 31

9 years ago
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?
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.
(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.
Assignee

Updated

9 years ago
Depends on: 607163
Assignee

Comment 34

9 years ago
Mac styling patch updated per review.
Attachment #484470 - Attachment is obsolete: true
Attachment #486229 - Flags: review?(dao)
blocking2.0: ? → beta7+
Assignee

Comment 36

9 years ago
Patch part 1, version 3 changes the toolbar type from "text" to "full", which simplifies the CSS.
Attachment #484921 - Attachment is obsolete: true
Attachment #486528 - Flags: review?(gavin.sharp)
Attachment #484921 - Flags: review?(gavin.sharp)
Assignee

Comment 37

9 years ago
Patch part 2, version 3 makes the Mac styling simpler by using list-style-image instead of background hacks.
Attachment #486529 - Flags: review?(dao)
Assignee

Updated

9 years ago
Attachment #486229 - Attachment is obsolete: true
Attachment #486229 - Flags: review?(dao)
Assignee

Comment 38

9 years ago
Patch part 3, version 2 updates the Windows styling per review.
Attachment #484473 - Attachment is obsolete: true
Attachment #486530 - Flags: review?(dao)
Assignee

Comment 39

9 years ago
Patch part 4, version 2 updates the Linux styling per review.
Attachment #484474 - Attachment is obsolete: true
Attachment #486531 - Flags: review?(dao)
Assignee

Updated

9 years ago
Attachment #485096 - Flags: approval2.0?
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?
Assignee

Comment 41

9 years ago
(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.
Assignee

Comment 42

9 years ago
Patch part 1, version 4 makes the close button a toolbarbutton instead of a button.
Attachment #486528 - Attachment is obsolete: true
Attachment #486689 - Flags: review?(gavin.sharp)
Attachment #486528 - Flags: review?(gavin.sharp)
Assignee

Comment 43

9 years ago
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.
Attachment #486529 - Attachment is obsolete: true
Attachment #486693 - Flags: review?(dao)
Attachment #486529 - Flags: review?(dao)
Assignee

Updated

9 years ago
Attachment #486693 - Attachment description: Proposed patch, part 2, version 4. → Proposed patch, part 2, version 4: Mac styling.
Assignee

Comment 44

9 years ago
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.
Attachment #486530 - Attachment is obsolete: true
Attachment #486695 - Flags: review?(dao)
Attachment #486530 - Flags: review?(dao)
Assignee

Comment 45

9 years ago
Patch part 4, version 3 updates the Linux styling wrt the toolbarbutton change.
Attachment #486531 - Attachment is obsolete: true
Attachment #486697 - Flags: review?(dao)
Attachment #486531 - Flags: review?(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.
Attachment #486697 - Flags: review?(dao)
Assignee

Comment 47

9 years ago
(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 :)
Assignee

Updated

9 years ago
Attachment #486697 - Flags: review?(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;
Assignee

Comment 49

9 years ago
Patch part 4, version 4 removes the -moz-appearance: toolbarbutton from the Linux styling.
Attachment #486697 - Attachment is obsolete: true
Attachment #486716 - Flags: review?(dao)
Attachment #486697 - Flags: review?(dao)
Attachment #485096 - Attachment description: Proposed patch, part 0, version 2: String changes. → Proposed patch, part 0, version 2: String changes (checked in)
Whiteboard: [strings]
Reporter

Comment 50

9 years ago
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.
blocking2.0: beta7+ → ?
Whiteboard: [strings-checked-in]
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.
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?
Reporter

Comment 53

9 years ago
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).
Assignee

Comment 54

9 years ago
Patch part 1, version 5 changes hud- to webconsole- and uses attributes instead of CSS class names to style the individual buttons.
Attachment #486689 - Attachment is obsolete: true
Attachment #487668 - Flags: review?(sdwilsh)
Attachment #486689 - Flags: review?(gavin.sharp)
Assignee

Comment 55

9 years ago
Patch part 2, version 5 makes the reviewer-requested changes to the Mac styling.
Attachment #486693 - Attachment is obsolete: true
Attachment #487669 - Flags: review?(dao)
Attachment #486693 - Flags: review?(dao)
Assignee

Comment 56

9 years ago
Patch part 3, version 4 makes the reviewer-requested changes to the Windows styling.
Attachment #486695 - Attachment is obsolete: true
Attachment #487670 - Flags: review?(dao)
Attachment #486695 - Flags: review?(dao)
Assignee

Comment 57

9 years ago
Patch part 4, version 5 makes the reviewer-requested changes to the Linux styling.
Attachment #486716 - Attachment is obsolete: true
Attachment #487671 - Flags: review?(dao)
Attachment #486716 - Flags: review?(dao)
Assignee

Comment 58

9 years ago
(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
(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.
blocking2.0: ? → betaN+
Assignee

Comment 60

9 years ago
Patch part 2, version 6 replaces toolbarbutton with .toolbarbutton-menubutton-button in the Mac style sheet.
Attachment #487669 - Attachment is obsolete: true
Attachment #487709 - Flags: review?(dao)
Attachment #487669 - Flags: review?(dao)
Assignee

Comment 61

9 years ago
Patch part 3, version 5 replaces toolbarbutton with .toolbarbutton-menubutton-button in the Windows style sheet.
Attachment #487670 - Attachment is obsolete: true
Attachment #487710 - Flags: review?(dao)
Attachment #487670 - Flags: review?(dao)
Assignee

Comment 62

9 years ago
Patch part 4, version 6 replaces toolbarbutton with .toolbarbutton-menubutton-button in the Linux style sheet.
Attachment #487671 - Attachment is obsolete: true
Attachment #487711 - Flags: review?(dao)
Attachment #487671 - Flags: review?(dao)
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?
Assignee

Comment 64

9 years ago
Patch part 1, version 6 fixes the spelling of "BUTTONS" and adds some 80 new tests.
Attachment #487668 - Attachment is obsolete: true
Attachment #487772 - Flags: review?(sdwilsh)
Attachment #487668 - Flags: review?(sdwilsh)
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?
Reporter

Updated

9 years ago
Blocks: 598519
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.
Attachment #487772 - Flags: review?(sdwilsh) → feedback?(ddahl)
Assignee

Comment 67

9 years ago
(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 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.
Attachment #487772 - Flags: feedback?(ddahl) → feedback+
Assignee

Comment 69

9 years ago
Patch part 1 version 7 addresses the feedback comments. Review requested.
Attachment #487772 - Attachment is obsolete: true
Attachment #489231 - Flags: review?(sdwilsh)
Assignee

Updated

9 years ago
Blocks: 605621
Note: comment 63 has not been fully addressed in the latest patch
Assignee

Comment 71

9 years ago
(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?
er, my bad.  Looked at the wrong version it would seem.
Assignee

Comment 73

9 years ago
Patch part 1, version 8 rebases to trunk.
Attachment #489231 - Attachment is obsolete: true
Attachment #489598 - Flags: review?(sdwilsh)
Attachment #489231 - Flags: review?(sdwilsh)
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;
Attachment #487711 - Flags: review?(dao) → review-
Assignee

Comment 75

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

Comment 76

9 years ago
(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?
(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.
Assignee

Comment 78

9 years ago
(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.
(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.
Assignee

Comment 80

9 years ago
Patch part 1, version 9 rebases to trunk.
Attachment #489598 - Attachment is obsolete: true
Attachment #491048 - Flags: review?(sdwilsh)
Attachment #489598 - Flags: review?(sdwilsh)
Assignee

Comment 81

9 years ago
Patch part 4, version 7 addresses review comments for the Linux styling.
Attachment #487711 - Attachment is obsolete: true
Attachment #491057 - Flags: review?(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.
Attachment #491057 - Flags: review?(dao) → review+
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 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
Attachment #491048 - Flags: review?(sdwilsh) → review+
Assignee

Comment 85

9 years ago
Proposed patch, part 1, version 10 addresses review comments.
Attachment #491048 - Attachment is obsolete: true
Assignee

Comment 86

9 years ago
Proposed patch, part 2, version 7 removes the unused ".hud-divider" rule from the Mac styling.
Attachment #487709 - Attachment is obsolete: true
Attachment #491709 - Flags: review?(dao)
Attachment #487709 - Flags: review?(dao)
Assignee

Comment 87

9 years ago
Patch part 3, version 6 removes the unused ".hud-divider" rule from the Windows CSS.
Attachment #487710 - Attachment is obsolete: true
Attachment #491710 - Flags: review?(dao)
Attachment #487710 - Flags: review?(dao)
Assignee

Comment 88

9 years ago
Proposed patch, part 4, version 8 addresses review comments for the Linux CSS.
Attachment #491057 - Attachment is obsolete: true
Assignee

Comment 89

9 years ago
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 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.
Attachment #491709 - Flags: review?(dao) → review+
Reporter

Comment 91

9 years ago
mass change: filter on PRIORITYSETTING
Priority: -- → P1
Reporter

Comment 92

9 years ago
mass change: filter on PRIORITYSETTING
Blocks: devtools4
Reporter

Comment 93

9 years ago
mass change: filter mail on BLOCKERSETTING
Severity: normal → blocker
Assignee

Comment 94

9 years ago
Patch part 1, version 11 rebases to trunk.
Attachment #491706 - Attachment is obsolete: true
Assignee

Comment 95

9 years ago
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?
Attachment #491709 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Attachment #492729 - Attachment description: Proposed patch, part 2, version 11: Code changes. → Proposed patch, part 1, version 11: Code changes.
Assignee

Updated

9 years ago
Whiteboard: [strings-checked-in] → [strings-checked-in] [patchclean:1123]
Assignee

Comment 96

9 years ago
Patch part 2, version 9 addresses review comments.
Attachment #492732 - Attachment is obsolete: true
Assignee

Comment 97

9 years ago
Patch part 3, version 7 replaces 0px with 0.
Attachment #491710 - Attachment is obsolete: true
Attachment #491710 - Flags: review?(dao)
Assignee

Comment 98

9 years ago
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.
Attachment #492729 - Attachment is obsolete: true
Attachment #492873 - Flags: review?(gavin.sharp)
Assignee

Comment 99

9 years ago
Proposed patch, part 2, version 10 makes the changes needed to add a dropdown to the CSS button on the Mac. Re-review requested.
Attachment #492736 - Attachment is obsolete: true
Attachment #492874 - Flags: review?(dao)
Assignee

Comment 100

9 years ago
Patch part 1, version 13 adds "jswarn" to the updateDefaultDisplayPrefs() method.
Attachment #492873 - Attachment is obsolete: true
Attachment #492876 - Flags: review?(gavin.sharp)
Attachment #492873 - Flags: review?(gavin.sharp)
Assignee

Comment 101

9 years ago
Patch part 1, version 14 fixes a typo in the jswarn line.
Attachment #492876 - Attachment is obsolete: true
Attachment #492880 - Flags: review?
Attachment #492876 - Flags: review?(gavin.sharp)
Assignee

Updated

9 years ago
Attachment #492880 - Flags: review? → review?(gavin.sharp)
Comment on attachment 492874 [details] [diff] [review]
Proposed patch, part 2, version 10: Mac styling.

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

What's this?
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.
Assignee

Updated

9 years ago
Attachment #492880 - Flags: review?(gavin.sharp) → review?(sdwilsh)
Assignee

Comment 104

9 years ago
Sorry, I let a comment sneak in there. Patch part 2, version 11 removes it.
Attachment #492874 - Attachment is obsolete: true
Attachment #493040 - Flags: review?(dao)
Attachment #492874 - Flags: review?(dao)
Assignee

Comment 105

9 years ago
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.
Patrick, the screenshot's got the wrong encoding on it. (Says it's a patch, not a png).
Assignee

Updated

9 years ago
Attachment #493136 - Attachment is patch: false
Assignee

Updated

9 years ago
Attachment #493136 - Attachment mime type: text/plain → image/png
Assignee

Comment 107

9 years ago
(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 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 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.
Assignee

Comment 110

9 years ago
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.
Attachment #493040 - Attachment is obsolete: true
Attachment #493798 - Flags: review?(dao)
Attachment #493040 - Flags: review?(dao)
Assignee

Comment 111

9 years ago
(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?
Assignee

Comment 112

9 years ago
(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.
Assignee

Comment 113

9 years ago
Updated the Windows styling per review. I made the close button styling identical to that in findBar.css.
Attachment #492738 - Attachment is obsolete: true
Attachment #493872 - Flags: review?(dao)
Assignee

Comment 114

9 years ago
Patch part 2 version 13 removes a few more useless selectors.
Attachment #493798 - Attachment is obsolete: true
Attachment #493877 - Flags: review?(dao)
Attachment #493798 - Flags: review?(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 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;
Attachment #493877 - Flags: review?(dao) → review+
Assignee

Comment 117

9 years ago
Proposed patch, part 2, version 14 addresses the review comment.
Attachment #493877 - Attachment is obsolete: true
Assignee

Comment 118

9 years ago
Patch part 3, version 9 removes the transparent border.
Attachment #493872 - Attachment is obsolete: true
Attachment #494148 - Flags: review?(dao)
Attachment #493872 - Flags: review?(dao)
Assignee

Updated

9 years ago
Whiteboard: [strings-checked-in] [patchclean:1123] → [strings-checked-in] [patchclean:1130]
Assignee

Comment 119

9 years ago
(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.
Attachment #494148 - Flags: review?(dao) → review+
Reporter

Comment 120

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

carrying forward r+
Attachment #494098 - Flags: review+
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.
Attachment #492880 - Flags: review?(sdwilsh)
Assignee

Updated

9 years ago
Whiteboard: [strings-checked-in] [patchclean:1130] → [strings-checked-in] [patchclean:1130] [checkin-needed]
Assignee

Updated

9 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [strings-checked-in] [patchclean:1130] [checkin-needed] → [strings-checked-in] [patchclean:1130] [checking-in]
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
Attachment #492880 - Attachment description: Proposed patch, part 1, version 14: Code changes. → [checked-in] Proposed patch, part 1, version 14: Code changes.
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
Attachment #494098 - Attachment description: Proposed patch, part 2, version 14: Mac styling. → [checked-in] Proposed patch, part 2, version 14: Mac styling.
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
Attachment #494148 - Attachment description: Proposed patch, part 3, version 9: Windows styling. → [checked-in] Proposed patch, part 3, version 9: Windows styling.
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
Attachment #491713 - Attachment description: Proposed patch, part 4, version 8: Linux styling. → [checked-in] Proposed patch, part 4, version 8: Linux styling.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [strings-checked-in] [patchclean:1130] [checking-in] → [strings-checked-in] [patchclean:1130]
Severity: blocker → normal

Updated

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