Last Comment Bug 687702 - Skin the Style Editor according to shorlander's mockups
: Skin the Style Editor according to shorlander's mockups
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 11
Assigned To: Paul Rouget [:paul]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 583041 713835
Blocks: 708257 709006
  Show dependency treegraph
 
Reported: 2011-09-19 16:00 PDT by Cedric Vivier [:cedricv]
Modified: 2012-01-04 06:30 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
StyleEditor Design Specs (OSX) - Horizontal Layout - 01 (774.65 KB, image/jpeg)
2011-10-20 09:28 PDT, Stephen Horlander [:shorlander]
no flags Details
StyleEditor Design Specs (OSX) - Horizontal Layout - 02 (840.73 KB, image/jpeg)
2011-10-20 12:51 PDT, Stephen Horlander [:shorlander]
no flags Details
StyleEditor Design Specs (OSX) - Vertical Layout - 01 (396.69 KB, image/jpeg)
2011-10-20 12:52 PDT, Stephen Horlander [:shorlander]
no flags Details
StyleEditor Design Specs (Windows) - Horizontal Layout - 01 (866.50 KB, image/jpeg)
2011-10-21 22:01 PDT, Stephen Horlander [:shorlander]
no flags Details
StyleEditor Design Specs (Windows) - Vertical Layout - 01 (390.96 KB, image/jpeg)
2011-10-21 22:02 PDT, Stephen Horlander [:shorlander]
no flags Details
WIP (53.12 KB, patch)
2011-12-12 17:20 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
devtools style - WIP (45.95 KB, patch)
2011-12-13 01:36 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.1 (83.55 KB, patch)
2011-12-13 05:23 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.2 (194.62 KB, patch)
2011-12-14 10:21 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch A - devtools.css - v1 (99.75 KB, patch)
2011-12-14 11:23 PST, Paul Rouget [:paul]
jwalker: feedback+
Details | Diff | Splinter Review
patch B - styleeditor theme - v1 (97.81 KB, patch)
2011-12-14 11:25 PST, Paul Rouget [:paul]
cedricv: feedback+
Details | Diff | Splinter Review
patch B - styleeditor theme - v1.1 (98.96 KB, patch)
2011-12-15 04:33 PST, Paul Rouget [:paul]
jwalker: feedback+
Details | Diff | Splinter Review
patch A - devtools.css - v1.1 (99.69 KB, patch)
2011-12-15 04:33 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch B - styleeditor theme - v1.2 (99.11 KB, patch)
2011-12-15 07:54 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch A - devtools.css - v2 (42.21 KB, patch)
2011-12-18 11:05 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch B - styleeditor theme - v2 (96.73 KB, patch)
2011-12-18 11:07 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch A - devtools.css - v2.1 (38.45 KB, patch)
2011-12-18 12:11 PST, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Splinter Review
patch B - styleeditor theme - v2.1 (96.68 KB, patch)
2011-12-19 03:45 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch B - styleeditor theme - v2.2 (95.10 KB, patch)
2011-12-19 05:08 PST, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Splinter Review
patch A - devtools.css - final (38.58 KB, patch)
2011-12-19 06:11 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description Cedric Vivier [:cedricv] 2011-09-19 16:00:17 PDT

    
Comment 1 Kevin Dangoor 2011-09-19 19:02:55 PDT
Marking as a P1... polish is important (especially in light of how the rest of the tools are shaping up!)
Comment 2 Cedric Vivier [:cedricv] 2011-10-13 10:50:31 PDT
Layout is ready in add-on 0.4.0. Colors/images/borders to go.
Comment 3 Stephen Horlander [:shorlander] 2011-10-20 09:28:06 PDT
Created attachment 568417 [details]
StyleEditor Design Specs (OSX) - Horizontal Layout - 01

Also see HTML/CSS mockup here: http://people.mozilla.com/~shorlander/style-editor/style-editor.html

Still finishing up the Windows version and the sidebar version. Should be able to pickup a lot of the rules from the Highlighter.
Comment 4 Stephen Horlander [:shorlander] 2011-10-20 12:51:00 PDT
Created attachment 568482 [details]
StyleEditor Design Specs (OSX) - Horizontal Layout - 02
Comment 5 Stephen Horlander [:shorlander] 2011-10-20 12:52:54 PDT
Created attachment 568485 [details]
StyleEditor Design Specs (OSX) - Vertical Layout - 01

Vertical specific rules. Also updated live mockups:

http://people.mozilla.com/~shorlander/style-editor/style-editor-osx-horizontal.html

http://people.mozilla.com/~shorlander/style-editor/style-editor-osx-vertical.html
Comment 6 Stephen Horlander [:shorlander] 2011-10-21 22:01:32 PDT
Created attachment 568843 [details]
StyleEditor Design Specs (Windows) - Horizontal Layout - 01
Comment 7 Stephen Horlander [:shorlander] 2011-10-21 22:02:53 PDT
Created attachment 568844 [details]
StyleEditor Design Specs (Windows) - Vertical Layout - 01

Live mockups:

http://people.mozilla.com/~shorlander/style-editor/style-editor-windows-horizontal.html

http://people.mozilla.com/~shorlander/style-editor/style-editor-windows-vertical.html
Comment 8 Cedric Vivier [:cedricv] 2011-10-31 02:05:59 PDT
What is our RTL story? Especially considering list items? (cf discussion on bug 583041)
Comment 9 Gary Kwong [:gkw] [:nth10sd] 2011-11-19 22:51:49 PST
Question from MozCamp Asia - say, one doesn't like the black background of the Style Editor.

Will one be able to use the Style Editor to change the background color of the Style Editor?
Comment 10 Rob Campbell [:rc] (:robcee) 2011-11-21 08:59:21 PST
You will be able to modify it via userChrome or themeing. I expect you'll be able to do that by loading the StyleEditor xul file into content and opening a Style Editor on it. Not really a first-order concern yet though.
Comment 11 Cedric Vivier [:cedricv] 2011-12-06 02:18:58 PST
Let's split this in two.
This bug is strictly about skinning / color theme.
Opened bug 707906 for the docking feature as in the mockups.
Comment 12 Paul Rouget [:paul] 2011-12-12 17:20:26 PST
Created attachment 581119 [details] [diff] [review]
WIP
Comment 13 Paul Rouget [:paul] 2011-12-12 17:51:06 PST
About attachment 581119 [details] [diff] [review],

I need some help to pass browser_styleeditor_sv_keynav.js.

Since the markup has deeply changed, I also need some help to make sure I didn't break any JS (test should, in theory be enough, but it's the theory).

Todo:
- we can't go through all the richlistitem in the right order with keyup/keydown. It is related to ordinal-group (keys follow the ordinal group, not the display order).
- the ":" after the title is added in JS. In RTL, it's added on the wrong side of the work.
- the "* " before the name is added in JS, so it gets cropped on overflow.

Can be in follup-bugs:
- the horizontal resizer doesn't work in LTR (apparently, dir=end is buggy).
- Sometimes, I get this error, I can't reproduce:
>  Error: getFocusedItemWithin(this._nav) is null
>  Source File: resource:///modules/devtools/SplitView.jsm
>  Line: 107
- Searching for "save" and "rules" will select all the items (because the search is based on the actual content of the <label>s)
- the CSS files should be much less aggressive for the selectors (parent selectors are often useless)
- add line returns
- (see https://wiki.mozilla.org/DevTools/CSSTips)
Comment 14 Paul Rouget [:paul] 2011-12-12 17:52:18 PST
Todo:
- windows and Mac version
Comment 15 Cedric Vivier [:cedricv] 2011-12-13 01:06:04 PST
(In reply to Paul Rouget [:paul] from comment #13)

Unfortunately could not get all close with that the patch myself as it doesn't apply and after multiple manual fix ups some files are still missing (the binaries are not in the patch? - and I can't manage to find non-landed devtools.css via bugzilla search :/ )

Anyways, from a 'pure' feedback point, indeed the patch is quite invasive and risky (changing from HTML to XUL certainly will bring new bugs and/or intermittents).

I completely understand and agree with changing the toolbar to an actual xul toolbar and toolbar buttons, since we can then reuse with other tools. Also this change does not seem risky at all.

I'm not so convinced at all for the other elements, using xul doesn't seem to provide any tangible benefit except "purity" imho :
- we don't use richlistbox anywhere and there is no plan to afaik, so there's no reuse problem
- it adds more code to handle things that can be handled by CSS only (:before/:after)
- as you've pointed out in the todo, it breaks some things that would need to be reimplemented
- we'll get 'funny XUL bugs' - that are less likely to happen with HTML


Maybe we could first focus on skinning the 'chrome' around the original HTML list  (with xul:toolbar and all) and having the resizers right?
It seems to be that's the hardest part, and is actually quite advanced already :)

Then, we can figure out if we can't just change the original 'white background' list's colors... which would make the patch much less invasive and avoid having to reimplement a bunch of stuff - doesn't look like the list really need any markup change to match the design.

Thoughts?
Comment 16 Paul Rouget [:paul] 2011-12-13 01:36:52 PST
Created attachment 581203 [details] [diff] [review]
devtools style - WIP
Comment 17 Paul Rouget [:paul] 2011-12-13 05:23:06 PST
Created attachment 581236 [details] [diff] [review]
patch v0.1
Comment 18 Paul Rouget [:paul] 2011-12-13 05:26:55 PST
Todo:
- test browser_styleeditor_readonly.js doesn't pass
- text doesn't crop
- windows & mac version
- resize on landscape is too aggressive (doesn't block)
Comment 19 Paul Rouget [:paul] 2011-12-14 10:21:57 PST
Created attachment 581702 [details] [diff] [review]
patch v0.2
Comment 20 Paul Rouget [:paul] 2011-12-14 10:24:56 PST
Todo:
- test browser_styleeditor_readonly.js doesn't pass
- text doesn't crop
Comment 21 Paul Rouget [:paul] 2011-12-14 11:23:11 PST
Created attachment 581722 [details] [diff] [review]
patch A - devtools.css - v1

This patch creates a shared CSS file for the devtools - to avoid duplicating the CSS code for the styleeditor. I also add a style for a dark searchbox, add a textured background for the toolbars for Linux and Mac, and add a 3px margin for Windows.
Comment 22 Paul Rouget [:paul] 2011-12-14 11:25:01 PST
Created attachment 581723 [details] [diff] [review]
patch B - styleeditor theme - v1

This patch implements the actual theme. splitview.css override some devtools.css style to make the toolbars a bit small and with a fixed height and font-sizes.
Comment 23 Paul Rouget [:paul] 2011-12-14 11:41:32 PST
follow up:
- ellipsis not present: bug 710811
- resizer bug on RTL: bug 710816
Comment 24 Paul Rouget [:paul] 2011-12-14 11:47:29 PST
https://tbpl.mozilla.org/?tree=Try&rev=14f614039ca0
Comment 25 Cedric Vivier [:cedricv] 2011-12-15 01:19:48 PST
Comment on attachment 581723 [details] [diff] [review]
patch B - styleeditor theme - v1

Looks pretty awesome! :)
And the patch now looks pretty minimal outside of CSS.

Some reservations (which I guess are anyways in your todo) :
- fonts and buttons really look small - shouldn't we NOT define size in the CSS and let default sizes kick in (Dao commented on that in one of my previous patches)
- the "there is no style sheet" case is not skinned well.
- the Save "button" doesn't float on the right of the list in horizontal mode.
Comment 26 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-15 02:21:58 PST
Comment on attachment 581722 [details] [diff] [review]
patch A - devtools.css - v1

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

Summary: cursor, display and -moz-box-flex are probably content CSS.
Other than that - I like it.

::: browser/themes/gnomestripe/browser.css
@@ -1987,5 @@
>  
>  /* Highlighter toolbar */
>  
>  #inspector-toolbar {
> -  -moz-appearance: none;

Thinking aloud:

In previous reviews we've come down slightly on the side of -moz-appearance being content rather than theme because you're giving some hints as to functionallity not just look.

However -moz-appearance: none; is probably a counter example where you're saying 'ignore browser preconceptions'.

So I agree with what you've done here, I think.

@@ -2043,5 @@
> -
> -#inspector-inspect-toolbutton[checked],
> -#inspector-tools > toolbarbutton[checked],
> -#devtools-sidebar-toolbar > toolbarbutton[checked] {
> -  color: hsl(208,100%,60%) !important;

Why does this need to be !important?
Could we achieve the same thing using
toolbarbutton#inspector-inspect-toolbutton[checked]? (or similar)

My concern is that !important is the nuclear option in a specificity war. As soon as you press the button, no-one else gets a vote.

We use !important in many places though and changing this could be lots of work, so I guess this is more food for thought.

@@ -2065,5 @@
> -/* Highlighter - toolbar resizers */
> -
> -.inspector-resizer {
> -  -moz-appearance: none;
> -  cursor: n-resize;

I think cursor tells you how something functions, so it should probably be in a content sheet.

::: browser/themes/gnomestripe/devtools/devtools.css
@@ +88,5 @@
> +}
> +
> +.devtools-resizer-anchor {
> +  -moz-appearance: none;
> +  cursor: n-resize;

Again - content css

@@ +105,5 @@
> +/* Search input */
> +
> +.devtools-searchinput {
> +  -moz-appearance: none;
> +  -moz-box-flex: 1;

I think this is layout, and therefore content css?

@@ +127,5 @@
> +  padding: 0 18px 0 12px;
> +}
> +
> +.devtools-searchinput > .textbox-input-box > .textbox-search-icons {
> +  display: none;

Again - content?
I don't think it's reasonable for a different theme to do anything else here
Comment 27 Paul Rouget [:paul] 2011-12-15 03:54:17 PST
(In reply to Cedric Vivier [cedricv] from comment #25)
> Comment on attachment 581723 [details] [diff] [review]
> patch B - styleeditor theme - v1
> 
> Looks pretty awesome! :)
> And the patch now looks pretty minimal outside of CSS.
> 
> Some reservations (which I guess are anyways in your todo) :
> - fonts and buttons really look small

It's based on Shorlander's spec.

> - shouldn't we NOT define size in the CSS and let default sizes kick in (Dao commented on that in one of my previous patches)

I know - using fixed height is bad. But here, we need the two toolbars to have the exact same height to give the illusion that they are just one bar. Using fixed heights make this much easier.

> - the "there is no style sheet" case is not skinned well.
> - the Save "button" doesn't float on the right of the list in horizontal
> mode.

ok
Comment 28 Paul Rouget [:paul] 2011-12-15 04:11:46 PST
(In reply to Joe Walker from comment #26)
> Comment on attachment 581722 [details] [diff] [review]
> patch A - devtools.css - v1
> 
> Review of attachment 581722 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Summary: cursor, display and -moz-box-flex are probably content CSS.
> Other than that - I like it.
> 
> ::: browser/themes/gnomestripe/browser.css
> @@ -1987,5 @@
> >  
> >  /* Highlighter toolbar */
> >  
> >  #inspector-toolbar {
> > -  -moz-appearance: none;
> 
> Thinking aloud:
> 
> In previous reviews we've come down slightly on the side of -moz-appearance
> being content rather than theme because you're giving some hints as to
> functionallity not just look.
> 
> However -moz-appearance: none; is probably a counter example where you're
> saying 'ignore browser preconceptions'.
> 
> So I agree with what you've done here, I think.
> 
> @@ -2043,5 @@
> > -
> > -#inspector-inspect-toolbutton[checked],
> > -#inspector-tools > toolbarbutton[checked],
> > -#devtools-sidebar-toolbar > toolbarbutton[checked] {
> > -  color: hsl(208,100%,60%) !important;
> 
> Why does this need to be !important?

Because !important is used in the earlier toolbarbutton definition (gnomestripe only iirc) - this has been discussed already in the toolbar design implementation.


> @@ -2065,5 @@
> > -/* Highlighter - toolbar resizers */
> > -
> > -.inspector-resizer {
> > -  -moz-appearance: none;
> > -  cursor: n-resize;
> 
> I think cursor tells you how something functions, so it should probably be
> in a content sheet.

I am removing these lines.

> ::: browser/themes/gnomestripe/devtools/devtools.css
> @@ +88,5 @@
> > +}
> > +
> > +.devtools-resizer-anchor {
> > +  -moz-appearance: none;
> > +  cursor: n-resize;
> 
> Again - content css
> 

Correct. I actually don't need to define cursor:n-resize.

> @@ +105,5 @@
> > +/* Search input */
> > +
> > +.devtools-searchinput {
> > +  -moz-appearance: none;
> > +  -moz-box-flex: 1;
> 
> I think this is layout, and therefore content css?

Correct.

> @@ +127,5 @@
> > +  padding: 0 18px 0 12px;
> > +}
> > +
> > +.devtools-searchinput > .textbox-input-box > .textbox-search-icons {
> > +  display: none;
> 
> Again - content?
> I don't think it's reasonable for a different theme to do anything else here

I don't think this is content. We might want to keep the default icons in the theme.

So apparently, just need to add this to content:

.devtools-searchinput {
 -moz-appearance: none;
 -moz-box-flex: 1;
}

But I don't have any file yet (can't use browser.css or highlighter.css).
Comment 29 Paul Rouget [:paul] 2011-12-15 04:33:02 PST
Created attachment 581931 [details] [diff] [review]
patch B - styleeditor theme - v1.1

addressed Cedric's comments
Comment 30 Paul Rouget [:paul] 2011-12-15 04:33:59 PST
Created attachment 581932 [details] [diff] [review]
patch A - devtools.css - v1.1

Addressed Joe's comments
Comment 31 Paul Rouget [:paul] 2011-12-15 04:39:04 PST
Comment on attachment 581932 [details] [diff] [review]
patch A - devtools.css - v1.1

Dao, this patch moves all the button & toolbar related devtools style to its own CSS file: devtools.css (theme/ only) - to avoid duplicating the CSS code for the styleeditor.

I also add a style for a dark searchbox, add a textured background for the toolbars for Linux and Mac, and add a 3px margin for Windows.

Thank you!
Comment 32 Paul Rouget [:paul] 2011-12-15 07:27:17 PST
(In reply to Paul Rouget [:paul] from comment #23)
> follow up:
> - ellipsis not present: bug 710811
> - resizer bug on RTL: bug 710816

- clean the CSS files: bug 711059
Comment 33 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-15 07:50:36 PST
Comment on attachment 581931 [details] [diff] [review]
patch B - styleeditor theme - v1.1

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

Thanks Paul.

::: browser/devtools/styleeditor/splitview.css
@@ +96,5 @@
>    display: -moz-box;
>  }
> +
> +.splitview-portrait-resizer {
> +    display: none;

This is picky, but 2 space indents?

::: browser/themes/gnomestripe/devtools/splitview.css
@@ +129,5 @@
> +/* Resizers */
> +
> +.splitview-landscape-resizer {
> +  -moz-appearance: none;
> +  cursor: w-resize;

content CSS

@@ +150,5 @@
> +.splitview-portrait-resizer {
> +  -moz-appearance: none;
> +  background: -moz-linear-gradient(top, black 1px, rgba(255,255,255,0.2) 1px),
> +              -moz-linear-gradient(top, hsl(210,11%,36%), hsl(210,11%,18%));
> +  cursor: n-resize;

content CSS
Comment 34 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-15 07:53:33 PST
It's also worth noting that we raised bug 711059 to tidy up the pre-existing CSS.
Comment 35 Paul Rouget [:paul] 2011-12-15 07:54:14 PST
Created attachment 581966 [details] [diff] [review]
patch B - styleeditor theme - v1.2

addressed Joe's comments
Comment 36 Paul Rouget [:paul] 2011-12-18 09:03:41 PST
This bug doesn't depend on the dock feature.
Comment 37 Dão Gottwald [:dao] 2011-12-18 09:21:40 PST
Comment on attachment 581932 [details] [diff] [review]
patch A - devtools.css - v1.1

>+<?xml-stylesheet href="chrome://browser/skin/devtools/devtools.css" type="text/css"?>

Rename devtools.css to common.css.

Please only move styles that actually need to be shared. devtools-resizer-bar appears to be used only once, for instance.

background-noise-toolbar.png is unreasonable large. Please trim it down significantly. Also, you probably should use this only on OS X (not Linux).

>+.devtools-searchinput {
>+  -moz-appearance: none;
>+  -moz-box-flex: 1;

flex usually belongs in content CSS or the markup.

>+  max-width: 25ex;

Presumably the search box is going to exist in quite different contexts. It seems questionable that you'd want the same max-width in all those contexts.

>+  background-image: url(magnifying-glass.png), -moz-linear-gradient(hsla(210,16%,76%,.15), hsla(210,16%,76%,.35));

magnifying-glass.png doesn't exist
Comment 38 Paul Rouget [:paul] 2011-12-18 10:24:37 PST
Thank you for the review. Working on this now.
Comment 39 Paul Rouget [:paul] 2011-12-18 11:05:31 PST
Created attachment 582687 [details] [diff] [review]
patch A - devtools.css - v2
Comment 40 Paul Rouget [:paul] 2011-12-18 11:07:22 PST
Created attachment 582688 [details] [diff] [review]
patch B - styleeditor theme - v2
Comment 41 Paul Rouget [:paul] 2011-12-18 11:09:19 PST
(In reply to Dão Gottwald [:dao] from comment #37)
> Comment on attachment 581932 [details] [diff] [review]
> patch A - devtools.css - v1.1
> 
> >+<?xml-stylesheet href="chrome://browser/skin/devtools/devtools.css" type="text/css"?>
> 
> Rename devtools.css to common.css.

Done.

> Please only move styles that actually need to be shared.
> devtools-resizer-bar appears to be used only once, for instance.

I moved devtools-resizer-bar and devtools-resizer-anchor because, in the future, we will use them in other tools (web console & debugger at least).

> background-noise-toolbar.png is unreasonable large. Please trim it down
> significantly. Also, you probably should use this only on OS X (not Linux).

I'll let shorlander do that. I removed background-noise. We can do that later: bug 711851

> >+.devtools-searchinput {
> >+  -moz-appearance: none;
> >+  -moz-box-flex: 1;
> 
> flex usually belongs in content CSS or the markup.

Done (flex="1" in patch B)

> >+  max-width: 25ex;
> 
> Presumably the search box is going to exist in quite different contexts. It
> seems questionable that you'd want the same max-width in all those contexts.

Done. Moved to styleeditor.css (patch B)

> >+  background-image: url(magnifying-glass.png), -moz-linear-gradient(hsla(210,16%,76%,.15), hsla(210,16%,76%,.35));
> 
> magnifying-glass.png doesn't exist

They were in Patch B. Moved back to Patch A.
Comment 42 Dão Gottwald [:dao] 2011-12-18 11:37:52 PST
Comment on attachment 582687 [details] [diff] [review]
patch A - devtools.css - v2

(In reply to Paul Rouget [:paul] from comment #41)
> > Please only move styles that actually need to be shared.
> > devtools-resizer-bar appears to be used only once, for instance.
> 
> I moved devtools-resizer-bar and devtools-resizer-anchor because, in the
> future, we will use them in other tools (web console & debugger at least).

The web console correctly uses a splitter for this, not a resizer.
Comment 43 Paul Rouget [:paul] 2011-12-18 12:11:30 PST
Created attachment 582697 [details] [diff] [review]
patch A - devtools.css - v2.1

don't move the resizer style from browser.css
Comment 44 Dão Gottwald [:dao] 2011-12-18 12:48:41 PST
Comment on attachment 582697 [details] [diff] [review]
patch A - devtools.css - v2.1

>+.devtools-searchinput {
>+  -moz-appearance: none;
>+  margin: 0 3px;
>+  background-color: transparent;
>+  border: 1px solid hsla(210,8%,5%,.6);
>+  border-radius: 20px;
>+  background-image: url(magnifying-glass.png), -moz-linear-gradient(hsla(210,16%,76%,.15), hsla(210,16%,76%,.35));
>+  background-repeat: no-repeat;
>+  background-position: 4px 3px, top left, top left;
>+  padding: 0 12px 0 18px;

  padding-top: 0;
  padding-bottom: 0;
  -moz-padding-start: 18px;
  -moz-padding-end: 12px;

>+  box-shadow: 0 1px 1px hsla(210,8%,5%,.3) inset,
>+              0 0 0 1px hsla(210,16%,76%,.1) inset,
>+              0 1px 0 hsla(210,16%,76%,.15);
>+  color: hsl(210,30%,85%);
>+}
>+
>+.devtools-searchinput:-moz-locale-dir(rtl) {
>+  background-position: -moz-calc(100% - 4px) 3px, top left, top left;
>+  padding: 0 18px 0 12px;

and remove this line
Comment 45 Paul Rouget [:paul] 2011-12-18 14:01:04 PST
Thank you for the r+ Dao!
Comment 46 Dão Gottwald [:dao] 2011-12-18 14:20:15 PST
Comment on attachment 582688 [details] [diff] [review]
patch B - styleeditor theme - v2

>-      <xul:box class="toolbar">
>-        <xul:button class="style-editor-newButton"
>+      <xul:toolbar class="toolbar devtools-toolbar">

The toolbar class isn't needed anymore, is it?

>+        <!-- FIXME: no RTL friendly -->

Please fix this.
Comment 47 Panos Astithas [:past] 2011-12-19 00:07:26 PST
(In reply to Dão Gottwald [:dao] from comment #42)
> Comment on attachment 582687 [details] [diff] [review]
> patch A - devtools.css - v2
> 
> (In reply to Paul Rouget [:paul] from comment #41)
> > > Please only move styles that actually need to be shared.
> > > devtools-resizer-bar appears to be used only once, for instance.
> > 
> > I moved devtools-resizer-bar and devtools-resizer-anchor because, in the
> > future, we will use them in other tools (web console & debugger at least).
> 
> The web console correctly uses a splitter for this, not a resizer.

So would it really be better to have the patch in bug 704110 or the one in bug 692409 touch both the console (or debugger) and the style editor, in order to extract these two files? This is work that is going to happen very soon.
Comment 48 Paul Rouget [:paul] 2011-12-19 03:27:05 PST
(In reply to Dão Gottwald [:dao] from comment #46)
> Comment on attachment 582688 [details] [diff] [review]
> >+        <!-- FIXME: no RTL friendly -->
> 
> Please fix this.

I was planning to fix this in a follow-up bug (bug 710816) - I didn't manage to get something that works in RTL and LTR. I will be working on this today.

Do you see anything else?
Comment 49 Paul Rouget [:paul] 2011-12-19 03:45:19 PST
Created attachment 582784 [details] [diff] [review]
patch B - styleeditor theme - v2.1

fixed resizer and removed the 'toolbar' class
Comment 50 Dão Gottwald [:dao] 2011-12-19 04:25:14 PST
(In reply to Panos Astithas [:past] from comment #47)
> (In reply to Dão Gottwald [:dao] from comment #42)
> > Comment on attachment 582687 [details] [diff] [review]
> > patch A - devtools.css - v2
> > 
> > (In reply to Paul Rouget [:paul] from comment #41)
> > > > Please only move styles that actually need to be shared.
> > > > devtools-resizer-bar appears to be used only once, for instance.
> > > 
> > > I moved devtools-resizer-bar and devtools-resizer-anchor because, in the
> > > future, we will use them in other tools (web console & debugger at least).
> > 
> > The web console correctly uses a splitter for this, not a resizer.
> 
> So would it really be better to have the patch in bug 704110 or the one in
> bug 692409 touch both the console (or debugger) and the style editor, in
> order to extract these two files? This is work that is going to happen very
> soon.

I'm not sure what you're asking, but replacing the splitters with resizers surely looks bogus.
Comment 51 Dão Gottwald [:dao] 2011-12-19 04:49:44 PST
Comment on attachment 582784 [details] [diff] [review]
patch B - styleeditor theme - v2.1

>--- a/browser/themes/gnomestripe/devtools/styleeditor.css
>+++ b/browser/themes/gnomestripe/devtools/styleeditor.css

>+.splitview-nav > li,
>+.stylesheet-info,
>+.stylesheet-more {
>+  display: -moz-box;
>+}
>+
>+.splitview-nav > li {
>+  -moz-box-orient: horizontal;
>+}
>+
>+.splitview-nav > li > hgroup {
>+  display: -moz-box;
>+  -moz-box-orient: vertical;
>+  -moz-box-flex: 1;
>+}

>+.stylesheet-info > h1 {
>+  -moz-box-flex: 1;
>+}
>+
>+.stylesheet-name {

>+  /* clip the text at the beginning */
>+  display: -moz-box;
>+  direction: rtl;
>+  text-align: left;
>+  overflow: hidden;
>+}

>+.splitview-nav > li > hgroup.stylesheet-info {
>+  -moz-box-pack: center;
> }

>+.splitview-nav:-moz-locale-dir(ltr) > li.unsaved > hgroup .stylesheet-name:before,
>+.splitview-nav:-moz-locale-dir(rtl) > li.unsaved > hgroup .stylesheet-name:after {

>   content: "* ";
> }

>+.stylesheet-enabled {

>+  display: -moz-box;
> }

>+.stylesheet-saveButton {
>+  display: none;
> }

>+.stylesheet-rule-count,
>+li:hover > hgroup > .stylesheet-more > h3 >  .stylesheet-saveButton {
>   display: -moz-box;

>+.stylesheet-more > spacer {
>+  -moz-box-flex: 1;
>+}

>+@media (max-aspect-ratio: 5/3) {
>+  li:hover > hgroup > .stylesheet-more > .stylesheet-rule-count {
>+    display: none;
>+  }

>+  .stylesheet-more {
>+    -moz-box-flex: 1;
>+    -moz-box-direction: reverse;
>+  }
>+
>+  .splitview-nav > li > hgroup.stylesheet-info {
>+    -moz-box-orient: horizontal;

>+    -moz-box-flex: 1;
>+  }
>+
>+  .stylesheet-more > spacer {
>+    -moz-box-flex: 0;
>+  }
>+}

content CSS?

By the way, the clipping of the stylesheet locations at the beginning really feels weird. It would probably be better to do this in the center. For xul, this would be crop="center".
Comment 52 Paul Rouget [:paul] 2011-12-19 05:08:51 PST
Created attachment 582797 [details] [diff] [review]
patch B - styleeditor theme - v2.2
Comment 53 Paul Rouget [:paul] 2011-12-19 05:17:52 PST
(In reply to Dão Gottwald [:dao] from comment #51)
> Comment on attachment 582784 [details] [diff] [review]
> patch B - styleeditor theme - v2.1
> content CSS?

Done.

> By the way, the clipping of the stylesheet locations at the beginning really
> feels weird. It would probably be better to do this in the center. For xul,
> this would be crop="center".

I agree. But I didn't manage to get something better yet. We don't use XUL but HTML here,
and the possibilities for cropping are quite limited.

I believe we should use a <label>. I have tried to do that first (see earlier patches), but I failed because that breaks a lot of things in the JS code. I will fix that later (bug 710811).
Comment 54 Paul Rouget [:paul] 2011-12-19 05:46:09 PST
Thank you Dao!
Comment 55 Paul Rouget [:paul] 2011-12-19 06:11:22 PST
Created attachment 582807 [details] [diff] [review]
patch A - devtools.css - final

addressed Dao's comments (comment 44)
Comment 57 Tim Taubert [:ttaubert] 2011-12-20 00:57:11 PST
https://hg.mozilla.org/mozilla-central/rev/8269e12d7adb
Comment 58 Scoobidiver (away) 2011-12-21 05:13:13 PST
It missed the Firefox 11 release for 10 minutes.
Comment 59 Rob Campbell [:rc] (:robcee) 2011-12-21 05:18:52 PST
for real?
Comment 60 Rob Campbell [:rc] (:robcee) 2011-12-21 05:22:07 PST
Comment on attachment 582797 [details] [diff] [review]
patch B - styleeditor theme - v2.2

Would very much like to have this in Aurora. It was intended for Firefox 11 but missed the cut by 10 minutes according to Scoobidiver. This is tragic.

Most of this bug is styling. There are some pngs which inflate the size of the patch. I'm not saying it is insignificant, but this is in mozilla-central now and working well there. I believe the risk is minimal and the benefit to having a properly-styled feature (Style Editor is new in 11) is worth the risk.
Comment 61 Scoobidiver (away) 2011-12-21 05:37:34 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #60)
> It was intended for Firefox 11 but missed the cut by 10 minutes according to 
> Scoobidiver. This is tragic.
May be I am wrong because this changeset is between:
* The latest changeset in 11.0a1/20111220: http://hg.mozilla.org/mozilla-central/rev/2afd7ae68e8b.
* The first changeset for 11.0a2/20111220: http://hg.mozilla.org/mozilla-central/rev/a8506ab2c654
Comment 62 Scoobidiver (away) 2011-12-21 09:31:50 PST
I was definitively wrong about the Firefox 11 target. It made it.
Comment 63 christian 2011-12-21 15:46:19 PST
Clearing requests due to comment 62

Note You need to log in before you can comment on or make changes to this bug.