687702 - Skin the Style Editor according to shorlander's mockups

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Comment 1

[Kevin Dangoor]

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]]

Layout is ready in add-on 0.4.0. Colors/images/borders to go.

Comment 3

[Stephen Horlander [:shorlander]]

Attachment: 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]]

Attachment: StyleEditor Design Specs (OSX) - Horizontal Layout - 02

Comment 5

[Stephen Horlander [:shorlander]]

Attachment: 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]]

Attachment: StyleEditor Design Specs (Windows) - Horizontal Layout - 01

Comment 7

[Stephen Horlander [:shorlander]]

Attachment: 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]]

What is our RTL story? Especially considering list items? (cf discussion on bug 583041)

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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)]

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]]

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]]

Attachment: WIP

Comment 13

[Paul Rouget [:paul]]

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]]

Todo:
- windows and Mac version

Comment 15

[Cedric Vivier [:cedricv]]

(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]]

Attachment: devtools style - WIP

Comment 17

[Paul Rouget [:paul]]

Attachment: patch v0.1

Comment 18

[Paul Rouget [:paul]]

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]]

Attachment: patch v0.2

Comment 20

[Paul Rouget [:paul]]

Todo:
- test browser_styleeditor_readonly.js doesn't pass
- text doesn't crop

Comment 21

[Paul Rouget [:paul]]

Attachment: 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]]

Attachment: 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]]

follow up:
- ellipsis not present: bug 710811
- resizer bug on RTL: bug 710816

Comment 24

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=14f614039ca0

Comment 25

[Cedric Vivier [:cedricv]]

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]]

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]]

(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]]

(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]]

Attachment: patch B - styleeditor theme - v1.1

addressed Cedric's comments

Comment 30

[Paul Rouget [:paul]]

Attachment: patch A - devtools.css - v1.1

Addressed Joe's comments

Comment 31

[Paul Rouget [:paul]]

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]]

(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]]

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]]

It's also worth noting that we raised bug 711059 to tidy up the pre-existing CSS.

Comment 35

[Paul Rouget [:paul]]

Attachment: patch B - styleeditor theme - v1.2

addressed Joe's comments

Comment 36

[Paul Rouget [:paul]]

This bug doesn't depend on the dock feature.

Comment 37

[Dão Gottwald [:dao]]

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]]

Thank you for the review. Working on this now.

Comment 39

[Paul Rouget [:paul]]

Attachment: patch A - devtools.css - v2

Comment 40

[Paul Rouget [:paul]]

Attachment: patch B - styleeditor theme - v2

Comment 41

[Paul Rouget [:paul]]

(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]]

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]]

Attachment: patch A - devtools.css - v2.1

don't move the resizer style from browser.css

Comment 44

[Dão Gottwald [:dao]]

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]]

Thank you for the r+ Dao!

Comment 46

[Dão Gottwald [:dao]]

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 (he/him) [:past] (please ni?)]

(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]]

(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]]

Attachment: patch B - styleeditor theme - v2.1

fixed resizer and removed the 'toolbar' class

Comment 50

[Dão Gottwald [:dao]]

(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]]

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]]

Attachment: patch B - styleeditor theme - v2.2

Comment 53

[Paul Rouget [:paul]]

(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]]

Thank you Dao!

Comment 55

[Paul Rouget [:paul]]

Attachment: patch A - devtools.css - final

addressed Dao's comments (comment 44)

Comment 56

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/8269e12d7adb

Comment 57

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8269e12d7adb

Comment 58

[Scoobidiver (away)]

It missed the Firefox 11 release for 10 minutes.

Comment 59

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

for real?

Comment 60

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

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)]

(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)]

I was definitively wrong about the Firefox 11 target. It made it.

Comment 63

[LegNeato]

Clearing requests due to comment 62