Skin the Style Editor according to shorlander's mockups

RESOLVED FIXED in Firefox 11

Status

defect
P2
normal
RESOLVED FIXED
8 years ago
Last year

People

(Reporter: cedricv, Assigned: paul)

Tracking

(Blocks 1 bug)

unspecified
Firefox 11
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 14 obsolete attachments)

840.73 KB, image/jpeg
Details
396.69 KB, image/jpeg
Details
866.50 KB, image/jpeg
Details
390.96 KB, image/jpeg
Details
95.10 KB, patch
dao
: review+
Details | Diff | Splinter Review
38.58 KB, patch
Details | Diff | Splinter Review
Reporter

Description

8 years ago
No description provided.

Comment 1

8 years ago
Marking as a P1... polish is important (especially in light of how the rest of the tools are shaping up!)
Priority: -- → P1

Updated

8 years ago
Priority: P1 → P2
Reporter

Updated

8 years ago
Assignee: nobody → cedricv
Reporter

Comment 2

8 years ago
Layout is ready in add-on 0.4.0. Colors/images/borders to go.
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.
Reporter

Comment 8

8 years ago
What is our RTL story? Especially considering list items? (cf discussion on bug 583041)
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?
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.
Reporter

Updated

8 years ago
Depends on: 707906
Reporter

Comment 11

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

Updated

8 years ago
Assignee: cedricv → paul
Assignee

Comment 12

8 years ago
Posted patch WIP (obsolete) — Splinter Review
Assignee

Comment 13

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

Updated

8 years ago
Attachment #581119 - Flags: feedback?(cedricv)
Assignee

Comment 14

8 years ago
Todo:
- windows and Mac version
Reporter

Comment 15

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

Comment 16

8 years ago
Posted patch devtools style - WIP (obsolete) — Splinter Review
Assignee

Comment 17

8 years ago
Posted patch patch v0.1 (obsolete) — Splinter Review
Assignee

Updated

8 years ago
Attachment #581203 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #581119 - Attachment is obsolete: true
Attachment #581119 - Flags: feedback?(cedricv)
Assignee

Comment 18

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

Comment 19

8 years ago
Posted patch patch v0.2 (obsolete) — Splinter Review
Assignee

Updated

8 years ago
Attachment #581236 - Attachment is obsolete: true
Assignee

Comment 20

8 years ago
Todo:
- test browser_styleeditor_readonly.js doesn't pass
- text doesn't crop
Assignee

Comment 21

8 years ago
Posted patch patch A - devtools.css - v1 (obsolete) — Splinter Review
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.
Assignee

Comment 22

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

Updated

8 years ago
Attachment #581702 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #581723 - Attachment description: patch B - styleeditor them - v1 → patch B - styleeditor theme - v1
Attachment #581723 - Flags: feedback?(cedricv)
Assignee

Updated

8 years ago
Attachment #581722 - Flags: feedback?(jwalker)
Assignee

Comment 23

8 years ago
follow up:
- ellipsis not present: bug 710811
- resizer bug on RTL: bug 710816
Reporter

Comment 25

8 years ago
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.
Attachment #581723 - Flags: feedback?(cedricv) → feedback+
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
Attachment #581722 - Flags: feedback?(jwalker) → feedback+
Assignee

Comment 27

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

Comment 28

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

Comment 29

8 years ago
addressed Cedric's comments
Assignee

Updated

8 years ago
Attachment #581723 - Attachment is obsolete: true
Assignee

Comment 30

8 years ago
Posted patch patch A - devtools.css - v1.1 (obsolete) — Splinter Review
Addressed Joe's comments
Assignee

Updated

8 years ago
Attachment #581722 - Attachment is obsolete: true
Assignee

Comment 31

8 years ago
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!
Attachment #581932 - Flags: review?(dao)
Assignee

Updated

8 years ago
Attachment #581931 - Flags: feedback?(jwalker)
Assignee

Comment 32

8 years ago
(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 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
Attachment #581931 - Flags: feedback?(jwalker) → feedback+
It's also worth noting that we raised bug 711059 to tidy up the pre-existing CSS.
Assignee

Comment 35

8 years ago
addressed Joe's comments
Assignee

Updated

8 years ago
Attachment #581931 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #581966 - Flags: review?(dao)
Assignee

Updated

8 years ago
Blocks: 709006
Blocks: 708257
No longer depends on: 708257
Assignee

Comment 36

8 years ago
This bug doesn't depend on the dock feature.
No longer depends on: 707906
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
Attachment #581932 - Flags: review?(dao) → review-
Assignee

Comment 38

8 years ago
Thank you for the review. Working on this now.
Assignee

Comment 39

8 years ago
Posted patch patch A - devtools.css - v2 (obsolete) — Splinter Review
Assignee

Updated

8 years ago
Attachment #581932 - Attachment is obsolete: true
Assignee

Comment 40

8 years ago
Assignee

Updated

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

Comment 41

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

Updated

8 years ago
Attachment #582687 - Flags: review?(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.
Attachment #582687 - Flags: review?(dao) → review-
Assignee

Comment 43

8 years ago
Posted patch patch A - devtools.css - v2.1 (obsolete) — Splinter Review
don't move the resizer style from browser.css
Assignee

Updated

8 years ago
Attachment #582687 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #582697 - Flags: review?(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
Attachment #582697 - Flags: review?(dao) → review+
Assignee

Updated

8 years ago
Attachment #582688 - Flags: review?(dao)
Assignee

Comment 45

8 years ago
Thank you for the r+ 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.
Attachment #582688 - Flags: review?(dao) → review-
(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.
Assignee

Comment 48

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

Comment 49

8 years ago
fixed resizer and removed the 'toolbar' class
Assignee

Updated

8 years ago
Attachment #582688 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #582784 - Flags: review?(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 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".
Assignee

Updated

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

Updated

8 years ago
Attachment #582797 - Flags: review?(dao)
Assignee

Comment 53

8 years ago
(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).
Attachment #582797 - Flags: review?(dao) → review+
Assignee

Comment 54

8 years ago
Thank you Dao!
Assignee

Comment 55

8 years ago
addressed Dao's comments (comment 44)
Assignee

Updated

8 years ago
Attachment #582697 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8269e12d7adb
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 11
It missed the Firefox 11 release for 10 minutes.
Target Milestone: Firefox 11 → Firefox 12
for real?
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.
Attachment #582797 - Flags: approval-mozilla-aurora?
Attachment #582807 - Flags: approval-mozilla-aurora?
(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
I was definitively wrong about the Firefox 11 target. It made it.
Target Milestone: Firefox 12 → Firefox 11

Updated

8 years ago
Attachment #582797 - Flags: approval-mozilla-aurora?

Updated

8 years ago
Attachment #582807 - Flags: approval-mozilla-aurora?

Comment 63

8 years ago
Clearing requests due to comment 62
Depends on: 713835

Updated

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