Closed Bug 687702 Opened 9 years ago Closed 9 years ago

Skin the Style Editor according to shorlander's mockups

Categories

(DevTools :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 11

People

(Reporter: cedricv, Assigned: paul)

References

Details

Attachments

(6 files, 14 obsolete files)

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
No description provided.
Marking as a P1... polish is important (especially in light of how the rest of the tools are shaping up!)
Priority: -- → P1
Priority: P1 → P2
Assignee: nobody → cedricv
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.
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.
Depends on: 707906
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: cedricv → paul
Attached patch WIP (obsolete) — Splinter Review
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)
Attachment #581119 - Flags: feedback?(cedricv)
Todo:
- windows and Mac version
(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?
Attached patch devtools style - WIP (obsolete) — Splinter Review
Attached patch patch v0.1 (obsolete) — Splinter Review
Attachment #581203 - Attachment is obsolete: true
Attachment #581119 - Attachment is obsolete: true
Attachment #581119 - Flags: feedback?(cedricv)
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)
Attached patch patch v0.2 (obsolete) — Splinter Review
Attachment #581236 - Attachment is obsolete: true
Todo:
- test browser_styleeditor_readonly.js doesn't pass
- text doesn't crop
Attached 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.
Attached patch patch B - styleeditor theme - v1 (obsolete) — Splinter Review
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.
Attachment #581702 - Attachment is obsolete: true
Attachment #581723 - Attachment description: patch B - styleeditor them - v1 → patch B - styleeditor theme - v1
Attachment #581723 - Flags: feedback?(cedricv)
Attachment #581722 - Flags: feedback?(jwalker)
follow up:
- ellipsis not present: bug 710811
- resizer bug on RTL: bug 710816
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+
(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
(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).
addressed Cedric's comments
Attachment #581723 - Attachment is obsolete: true
Attached patch patch A - devtools.css - v1.1 (obsolete) — Splinter Review
Addressed Joe's comments
Attachment #581722 - Attachment is obsolete: true
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)
Attachment #581931 - Flags: feedback?(jwalker)
(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.
addressed Joe's comments
Attachment #581931 - Attachment is obsolete: true
Attachment #581966 - Flags: review?(dao)
Blocks: 709006
Blocks: 708257
No longer depends on: 708257
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-
Thank you for the review. Working on this now.
Attached patch patch A - devtools.css - v2 (obsolete) — Splinter Review
Attachment #581932 - Attachment is obsolete: true
Attached patch patch B - styleeditor theme - v2 (obsolete) — Splinter Review
Attachment #581966 - Attachment is obsolete: true
Attachment #581966 - Flags: review?(dao)
(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.
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-
Attached patch patch A - devtools.css - v2.1 (obsolete) — Splinter Review
don't move the resizer style from browser.css
Attachment #582687 - Attachment is obsolete: true
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+
Attachment #582688 - Flags: review?(dao)
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.
(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?
fixed resizer and removed the 'toolbar' class
Attachment #582688 - Attachment is obsolete: true
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".
Attachment #582784 - Attachment is obsolete: true
Attachment #582784 - Flags: review?(dao)
Attachment #582797 - Flags: review?(dao)
(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+
Thank you Dao!
addressed Dao's comments (comment 44)
Attachment #582697 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8269e12d7adb
Status: NEW → RESOLVED
Closed: 9 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
Attachment #582797 - Flags: approval-mozilla-aurora?
Attachment #582807 - Flags: approval-mozilla-aurora?
Clearing requests due to comment 62
Depends on: 713835
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.