Closed Bug 611851 Opened 9 years ago Closed 9 years ago

Restyle the Web Console splitter

Categories

(DevTools :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: pcwalton, Assigned: pcwalton)

References

Details

(Keywords: polish)

Attachments

(2 files, 4 obsolete files)

The Web Console's resize handle looks ugly on the Mac right now. I'd like to suggest that we change it to look more like Apple Mail's pane separator [1].

[1]: http://bit.ly/9UsaO9
Attached a screenshot of the current WIP on Linux.
Summary: Make the Web Console resize handle nicer looking on the Mac → Restyle the Web Console splitter
OS: Mac OS X → All
Attached patch Proposed patch. (obsolete) — Splinter Review
Patch attached.
Attachment #491399 - Flags: feedback?(mihai.sucan)
Comment on attachment 491399 [details] [diff] [review]
Proposed patch.

The patch looks fine to me. Just a minor issue:

       if (splitters[i].getAttribute("class") == "hud-splitter") {
+				splitters[i].nextSibling.classList.remove("webconsole-browser-stack");
         splitters[i].parentNode.removeChild(splitters[i]);
         break;

Indentation seems to be broken.

Visually, there's not much of a difference with the patch applied.
Attachment #491399 - Flags: feedback?(mihai.sucan) → feedback+
Attached patch Proposed patch, version 2. (obsolete) — Splinter Review
New version of the patch addresses feedback. Review requested.
Attachment #491399 - Attachment is obsolete: true
Attachment #491715 - Flags: review?(gavin.sharp)
(In reply to comment #3)
> Visually, there's not much of a difference with the patch applied.

It's more noticeable on the Mac, but it's a minor bit of polish all around.
mass change: filter on PRIORITYSETTING
Blocks: devtools4
Priority: -- → P2
Comment on attachment 491715 [details] [diff] [review]
Proposed patch, version 2.

>     <content>
>       <xul:stringbundle anonid="tbstringbundle" src="chrome://browser/locale/tabbrowser.properties"/>
>       <xul:tabbox anonid="tabbox" class="tabbrowser-tabbox"
>                   flex="1" eventnode="document" xbl:inherits="handleCtrlPageUpDown"
>                   onselect="if (event.target.localName == 'tabpanels') this.parentNode.updateCurrentBrowser();">
>         <xul:tabpanels flex="1" class="plain" selectedIndex="0" anonid="panelcontainer">
>-          <xul:notificationbox flex="1">
>+          <xul:notificationbox flex="1" class="notification-box">

This class is too generic.

>+.notification-box {
>+  background-color: Window;
>+}

You're affecting notification bars here...
Splitters can have a background despite -moz-appearance:splitter using box-shadow, e.g. box-shadow: 0 0 0 10px red inset.
Attached patch Proposed patch, version 3. (obsolete) — Splinter Review
New version of the patch uses box-shadow to avoid the notification box changes.
Attachment #491715 - Attachment is obsolete: true
Attachment #495712 - Flags: review?(dao)
Attachment #491715 - Flags: review?(gavin.sharp)
Comment on attachment 495712 [details] [diff] [review]
Proposed patch, version 3.

>+  // We apply a border to the browser stack when the console is open via
>+  // this CSS class.
>+  splitter.nextSibling.setAttribute("class", "webconsole-browser-stack");

Please avoid this, this code shouldn't mess with the stack.

>--- a/toolkit/themes/gnomestripe/global/webConsole.css
>+++ b/toolkit/themes/gnomestripe/global/webConsole.css

> .hud-box {
>   border-bottom: 1px solid #aaa;
>   text-shadow: none;
>+  background-color: #ffffff;
> }

What's this?
Attached patch Proposed patch, version 4. (obsolete) — Splinter Review
New version of the patch removes all the changes to HUDService.jsm and replaces the browser stack modification with a box shadow. Yay!
Attachment #495712 - Attachment is obsolete: true
Attachment #495858 - Flags: review?(dao)
Attachment #495712 - Flags: review?(dao)
(In reply to comment #10)
> Comment on attachment 495712 [details] [diff] [review]
> Proposed patch, version 3.
> 
> >+  // We apply a border to the browser stack when the console is open via
> >+  // this CSS class.
> >+  splitter.nextSibling.setAttribute("class", "webconsole-browser-stack");
> 
> Please avoid this, this code shouldn't mess with the stack.

Replaced with a box shadow.

> >--- a/toolkit/themes/gnomestripe/global/webConsole.css
> >+++ b/toolkit/themes/gnomestripe/global/webConsole.css
> 
> > .hud-box {
> >   border-bottom: 1px solid #aaa;
> >   text-shadow: none;
> >+  background-color: #ffffff;
> > }
> 
> What's this?

Something no longer necessary!
Comment on attachment 495858 [details] [diff] [review]
Proposed patch, version 4.

>--- a/toolkit/themes/gnomestripe/global/webConsole.css
>+++ b/toolkit/themes/gnomestripe/global/webConsole.css

>+.hud-splitter {
>+  box-shadow: 0 -1px 0 0 ButtonShadow inset, 0 0 0 10px Window inset;

We'd usually use ThreeDShadow and -moz-Dialog. Not sure whether/how ButtonShadow and Window are different.

>--- a/toolkit/themes/pinstripe/global/webConsole.css
>+++ b/toolkit/themes/pinstripe/global/webConsole.css

>+.hud-splitter {
>+  border-bottom: solid #a5a5a5 1px;
>+  height: 8px;

Does this change anythig? splitter.css sets min-height: 9px;.
Attachment #495858 - Flags: review?(dao) → review+
New patch addresses review comments. approval2.0 requested, as this is a nice piece of polish for the Web Console and very low risk (only minor CSS changes).
Attachment #495858 - Attachment is obsolete: true
Attachment #495897 - Flags: approval2.0?
Attachment #495897 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/9c99f0193930
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.