583041 - Style Editor integration

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

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

The inspector needs a facility to edit CSS from the style panel. The editor should display the block of CSS from the appropriate style sheet containing the set of rules for the given selector. On completion, the updated rules should be applied to the page.

Comment 2

[Patrick Walton (:pcwalton)]

Attachment: Screenshot.

Quick annotated screenshot attached. UI feedback requested.

This panel appears when the user clicks on (a) a selector; (b) a style rule; (c) the name of the source file, in the style panel.

There is a mistake in that the name of the CSS file being edited is not yet shown in the title bar of the panel.

Comment 3

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

I think this looks pretty decent.

Are changes made to the CSS on completion of typing or does the user have to press "save" or similar?

Comment 4

[Patrick Walton (:pcwalton)]

(In reply to comment #3)
> I think this looks pretty decent.
> 
> Are changes made to the CSS on completion of typing or does the user have to
> press "save" or similar?

Preferably as you type. I'd like to avoid "save" buttons if we can.

Comment 5

[Kevin Dangoor]

I'm worried about the performance of doing it as you type... while I'm sure the browser can keep up with frequent stylesheet updates, we'll need to see how the style panel behaves.

It shouldn't be a lot of code to try it out and see how it works. But, definitely try it on some reason sites that have significant amounts of CSS.

Comment 6

[Kevin Dangoor]

We would like this to be a beta 5 blocker because there are strings in this feature.

Patrick has made good progress on this bug already and hopes to have the basic UI in a patch for this bug soon.

Comment 7

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1.

First version of the first part of the patch uploaded. This is the most important part since it has all the strings.

What's implemented:
* Showing and hiding the panel appropriately.
* Basic CSS editing with the Preview button.
* Choosing a style sheet to view.

What's not:
* Unit tests.
* Integration with the CSS panel (changes don't currently show up there).
* Export functionality.
* Automatically jumping to the line containing the rule of interest.
* Panel resizing.
* UI polish.

Comment 8

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

Comment on attachment 468562 [details] [diff] [review]
Proposed patch, part 1.

A few notes/nits.

you're using <statusbar> and <statusbarpanels> instead of <toolbars> and <hbox>es?

You're using a separate .js file for csseditor.js. Could this be a .jsm? (same probably goes for csslogic.js, for that matter)

nit: not a huge fan of the _method format, in csseditor.js, but it's your file so you do what you like!

and a mega-nit:
#inspector-css-editor-input's font-family: rule in browser.css has extra indentation on it.

I have no problems with any of this. I love it. I want to use it.

Comment 9

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

ps, I am sad there is no frankenxul widget in the final patch.

Comment 10

[Johnathan Nightingale [:johnath]]

Blocking beta5 - this is an essential function of the inspector feature.

Comment 11

[Patrick Walton (:pcwalton)]

(In reply to comment #8)
> Comment on attachment 468562 [details] [diff] [review]
> Proposed patch, part 1.
> 
> A few notes/nits.
> 
> you're using <statusbar> and <statusbarpanels> instead of <toolbars> and
> <hbox>es?

I made a change to put the toolbar on the bottom of the window instead of the top: this helps the balance of the UI with the added "Preview" button IMHO. Because it's on the bottom I opted for a statusbar. Is it okay to put toolbars on the bottom?

> You're using a separate .js file for csseditor.js. Could this be a .jsm? (same
> probably goes for csslogic.js, for that matter)

Will do.

> nit: not a huge fan of the _method format, in csseditor.js, but it's your file
> so you do what you like!

It's just a way of indicating which methods are private. I noticed precedent for it in csshtmltree.js.

> and a mega-nit:
> #inspector-css-editor-input's font-family: rule in browser.css has extra
> indentation on it.

Indeed it does. Fixed in the upcoming patch.

Comment 12

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.

New patch adds some simple unit tests, converts the module to JSM, and replaces the statusbar with a toolbar. Feedback requested.

I will be filing followup bugs on all of the missing features.

Comment 13

[Patrick Walton (:pcwalton)]

This patch is intended to be enough for feature and string freeze. Please let me know if there's anything that needs to be added. All missing features should be considered bugs at this point.

Comment 14

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 3.

Version 3 removes a debugging dump() that was mistakenly left in.

Comment 15

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 4.

Version 4 of the patch adds the export functionality (Copy and Save As), and unit tests for each. It adds one new l10n string: the title of the save dialog. This should really be feature complete now.

Feedback requested.

Comment 16

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 5.

Version 5 rebases the patch to the current dependencies as of this writing.

Comment 17

[Patrick Walton (:pcwalton)]

Followup bugs added for known issues: 590307 590795 590796 590797 590799.

Comment 18

[Kevin Dangoor]

reducing the "depends" list as this bug does not depend on the followups.

Comment 19

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

Comment on attachment 469289 [details] [diff] [review]
Proposed patch, version 5.

+              <menuitem label="foo.css"/>
+              <menuitem label="&#x2022; bar.css"/>
+              <menuitem label="baz.css"/>

not sure we need these.

+  // Enables or disables the "Preview" and "Export" buttons.
+  _enableUI: function CEUI_enableUI(aEnabled)

should probably use javadoc style comments even if these are private. Not sure if this is a requirement for review.

I really like the cssEditor.jsm module. Looks like this'll do nicely. :)

in global-scripts.inc:
+<script type="application/javascript" src="chrome://browser/content/csseditor.js"/>

is that a leftover from before?

Comment 21

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 5.1.

Patch rebased and revised per feedback. Review requested.

Comment 22

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 5.2.

Patch rebased to trunk and latest dependent patches.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

We're not going to be able to get this for beta 5, but should be able to for beta 6.

Comment 24

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 5.3.

Patch rebased to trunk and the latest dependent patches.

Comment 25

[Kevin Dangoor]

Inspector feature postponed. Removing blocking flags from Inspector bugs.

Comment 26

[Kevin Dangoor]

Removing items from kd4b6.

Comment 27

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 28

[Justin Dolske [:Dolske]]

Comment on attachment 470580 [details] [diff] [review]
Proposed patch, version 5.3.

Clearing review request, since the CSS inspector was cut from Fx4. Not sure if the plan is to resume work on the current code after Fx4, if it is feel to tag for review again, but I won't be getting around to it for some time.

Comment 29

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

Attachment: cssEditor

exported from neonux' github.

currently an issue with browser.xul (error at line 1520, <menuitem id="menu_styleEditor"...)

looks ok to me, but the parser didn't like something.

Comment 30

[Cedric Vivier [:cedricv]]

Attachment: Working and simpler patch with some 'side' features removed

Will open separate bugs for Import, Save, Save All and Indentation patches.

Comment 31

[Cedric Vivier [:cedricv]]

Attachment: Patch

Split tests in a separate patch.

Comment 32

[Cedric Vivier [:cedricv]]

Attachment: Tests

Comment 33

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

Comment on attachment 543983 [details] [diff] [review]
Patch

(note, this review was a little more detailed but the "splinter" review system lost my comments. What follows is an abridged version.)

Overall, this is really nice code. I don't have a lot of bad things to say about it. Most of my concerns are stylistic nits which are entirely yours to make. Since they're minor and I don't really feel like retyping them, I'll let them go.

StyleEditor.jsm

(license block)
+ *
+ * The Original Code is mozilla.org code.

Here and elsewhere, this should probably be:
* The Original Code is Style Editor.

...

+   * Flag changes can be tracked via onFlagChange (@see addActionListener).
+   */
+
+  DISABLED_FLAG:      "disabled",
+  ERROR_FLAG:         "error",
+  IMPORTED_FLAG:      "imported",
+  INLINE_FLAG:        "inline",
+  MODIFIED_FLAG:      "modified",
+  NEW_FLAG:           "new",
+  OPEN_FLAG:          "open",
+  UNSAVED_FLAG:       "unsaved",

I'm not sure I like these as properties of the StyleEditor's prototype. I think we probably want these to be in a separate StyleEditorFlags object.

+  setFlag: function SE_setFlag(aName)
+  {
+    if (this.hasFlag(aName)) {
+      return false;

No sanity checking on the flag. You could assert(StyleEditorFlags.hasOwnProperty(aName), ...) here.

+  clearFlag: function SE_clearFlag(aName)
+  {
+    let index = this._flags.indexOf(aName);
+    if (index == -1) {

probably don't care so much about flag validity in the clear method as we can just return as you're already doing.

in _loadSource: function SE__loadSource() ...
+    switch (scheme) {
+    case "file":

not crazy about this (lack of) indentation style, but I understand why you chose it.

+    case "chrome":
+    case "resource":
+      NetUtil.asyncFetch(this.styleSheet.href, function onAsyncFetch(stream, status) {
+        if (!Components.isSuccessCode(status)) {
+          return this._signalError(LOAD_ERROR);

You'll need a try/catch wrapper around this. It can throw.

+        let source = NetUtil.readInputStreamToString(stream, stream.available());

same here (or you can just wrap the whole thing).

+    default:
+      let cacheService = Cc["@mozilla.org/network/cache-service;1"].getService(Ci.nsICacheService);
+      let session = cacheService.createSession("HTTP", Ci.nsICache.STORE_ANYWHERE, true);

And you'll likely need another try block around this whole section.

+++ b/browser/base/content/StyleEditor/StyleEditorUtil.jsm

There seem to be a number of unused functions in this module.

Overall, very nice.

We'll need a browser peer review on the browser and style pieces.

A note about the UI, we need to get shorlander in here to do some tidying. I'm not super-keen with how this looks. There's a big chunky window border around the editor, for example.

I'm going to give this an r- for now just because of the unused methods in ...Utils and for the potentially uncaught exceptions. otherwise, looks nice!

Comment 34

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

Comment on attachment 543984 [details] [diff] [review]
Tests

in browser_styleeditor_enabled.js

+
+function run()
+{
+  gChromeWindow.removeEventListener("load", run);

we usually add the third option to removeEventListener. e.g., false.

+  list.selectedIndex = 0;
+  setTimeout(function () {

setTimeouts are verboten! If you need a slight pause, try executeSoon() instead. (it's equivalent to setTimeout(func, 0)).

+function cleanup()
+{
+  gChromeWindow.close();
+  gChromeWindow = null;

repeated functions like this one can be consolidated into a "head.js" file. The test harness will automatically include it in every test you run.

r- because of the setTimeouts.

Tests looks reasonably comprehensive though. Should be good with the above fixed.

Comment 35

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

Comment on attachment 543983 [details] [diff] [review]
Patch

canceling UI review request as Cedric's doing some refreshment over in bug 671350.

Comment 36

[Dave Camp (:dcamp)]

Cedric, can we get a new version of this patch landed before we go too far down the responsive UI road?

Comment 37

[Cedric Vivier [:cedricv]]

Makes sense. I'll rebase this patch tomorrow first thing in the morning.

Comment 38

[Cedric Vivier [:cedricv]]

Attachment: Bug 583041 - Style Editor integration [1/2] v2

New patch with improvements according to review.

Comment 39

[Cedric Vivier [:cedricv]]

Attachment: Bug 583041 - Style Editor integration (tests) [2/2]

Tests v2.

Comment 40

[Cedric Vivier [:cedricv]]

Attachment: Diff against v1

Interdiff if needed.

Comment 41

[Cedric Vivier [:cedricv]]

Attachment: Diff v2 against v1

Much better interdiff (stat, ignore space, ...)

Comment 42

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

this looks good.

I think we should land this in the browser/devtools directory though. Probably don't want to land new code into browser/base if we can avoid it.

If this is too much of a pain to rewrite to land there, we can try to get it into browser/base, though that could come with some additional review lag.

What do you think?

Comment 43

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

also, I'll continue reviewing this, with the ultimate destination in mind.

Comment 44

[Cedric Vivier [:cedricv]]

Makes sense.
There is no pain at all, just have to change the url constant in the few JSMs and update (actually simplify) the integration script from add-on :)

I've had already opened a bug for this move, but I can WONTFIX DUPLICATE it if we are actually gonna land directly in devtools/
https://bugzilla.mozilla.org/show_bug.cgi?id=675148

Comment 45

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

> Makes sense.
> There is no pain at all, just have to change the url constant in the few
> JSMs and update (actually simplify) the integration script from add-on :)


Well that's good! :)

> I've had already opened a bug for this move, but I can WONTFIX DUPLICATE it
> if we are actually gonna land directly in devtools/
> https://bugzilla.mozilla.org/show_bug.cgi?id=675148

yeah, I think we should.(In reply to comment #44)

Comment 47

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

still waiting on updated patches in these bugs.

Comment 48

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration [1/2] - b=583041 r=robcee,msucan

Comment 49

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration (tests) [2/2] - b=583041 r=robcee,msucan

Comment 50

[Cedric Vivier [:cedricv]]

Attachments 557511 and 557512 are browser integration patches based off tag 0.2.7.1 on http://github/neonux/StyleEditor.

Add-on builds are available at http://neonux.com/StyleEditor/builds/

The patches above have 92% test coverage according to reports you can find at http://neonux.com/StyleEditor/coverage/
(the uncovered 8% are mostly things not meaninful to cover)

Comment 51

[Dão Gottwald [:dao]]

Comment on attachment 557511 [details] [diff] [review]
Style Editor integration [1/2] - b=583041 r=robcee,msucan

>--- a/browser/themes/winstripe/browser/jar.mn
>+++ b/browser/themes/winstripe/browser/jar.mn
>@@ -236,8 +236,15 @@ browser.jar:
>         skin/classic/aero/browser/sync-desktopIcon.png
>         skin/classic/aero/browser/sync-mobileIcon.png
>         skin/classic/aero/browser/sync-notification-24.png
>         skin/classic/aero/browser/syncSetup.css
>         skin/classic/aero/browser/syncCommon.css
>         skin/classic/aero/browser/syncQuota.css
> #endif
> #endif
>+  skin/classic/browser/devtools/AdaptiveSplitView.css (devtools/AdaptiveSplitView.css)
>+  skin/classic/browser/devtools/StyleEditor.css (devtools/StyleEditor.css)
>+  skin/classic/browser/devtools/gimp-eye.png (devtools/gimp-eye.png)
>+  skin/classic/browser/devtools/right-arrow-hover.png (devtools/right-arrow-hover.png)
>+  skin/classic/browser/devtools/right-arrow-small-hover.png (devtools/right-arrow-small-hover.png)
>+  skin/classic/browser/devtools/right-arrow-small.png (devtools/right-arrow-small.png)
>+  skin/classic/browser/devtools/right-arrow.png (devtools/right-arrow.png)

Is this tested? It doesn't look like it's going to work.

Comment 52

[Dão Gottwald [:dao]]

Comment on attachment 557511 [details] [diff] [review]
Style Editor integration [1/2] - b=583041 r=robcee,msucan

>--- a/browser/base/content/browser-appmenu.inc
>+++ b/browser/base/content/browser-appmenu.inc
>@@ -187,16 +187,22 @@
>                     type="checkbox"
>                     command="Tools:Inspect"
>                     key="key_inspect"/>
>           <menuitem id="appmenu_scratchpad"
>                     hidden="true"
>                     label="&scratchpad.label;"
>                     key="key_scratchpad"
>                     command="Tools:Scratchpad"/>
>+          <menuitem id="appmenu_styleEditor"
>+                    hidden="true"
>+                    label="&styleeditor.label;"
>+                    accesskey="&styleeditor.accesskey;"
>+                    key="key_styleeditor"
>+                    command="Tools:StyleEditor"/>

This is going to fail browser_bug616836.js.

Comment 53

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #51)
> Comment on attachment 557511 [details] [diff] [review]
> Style Editor integration [1/2] - b=583041 r=robcee,msucan
> >+  skin/classic/browser/devtools/AdaptiveSplitView.css (devtools/AdaptiveSplitView.css)
> >+  skin/classic/browser/devtools/StyleEditor.css (devtools/StyleEditor.css)
> >+  ...
> Is this tested? It doesn't look like it's going to work.

Yes it works. Why shouldn't it?

Comment 54

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #52)
> Comment on attachment 557511 [details] [diff] [review]
> Style Editor integration [1/2] - b=583041 r=robcee,msucan
> >+                    accesskey="&styleeditor.accesskey;"
> This is going to fail browser_bug616836.js.

Oh nice catch! :)
Didn't notice accesskey are out. I'll run the full browser testsuite with it asap.

Comment 55

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

Comment on attachment 557511 [details] [diff] [review]
Style Editor integration [1/2] - b=583041 r=robcee,msucan

changes in browser.xul, browser-menubar.inc and browser-appmenu.inc look good.

browser.js:

+    // focus currently open Style Editor window for this document, if any
+    let contentWindow = Services.wm.getMostRecentWindow("navigator:browser")
+                          .gBrowser.selectedBrowser.contentWindow;

don't need to do getMostRecentWindow. You can access gBrowser directly in browser.js.

+    while(enumerator.hasMoreElements()) {
+      var win = enumerator.getNext();

use let instead of var here.

in adaptiveSplitView:

+
+  this._mql = aRoot.ownerDocument.defaultView.matchMedia(LANDSCAPE_MEDIA_QUERY);
+  this._mql.addListener(this._onOrientationChange.bind(this));

this is cute. :)

nice code in this file. No comments.

in styleeditor/Makefile.in

+#
+# The Initial Developer of the Original Code is Mozilla Corporation.

should be Mozilla Foundation.

+# 
+# Portions created by the Initial Developer are Copyright (C) 2010
+# the Initial Developer. All Rights Reserved.

2011.

+ifdef ENABLE_TESTS
+  ifneq (mobile,$(MOZ_BUILD_APP))

I don't think we need the "ifneq (mobile..." test.

in StyleEditor.jsm:

+const STYLE_EDITOR_CONTENT = "resource:///modules/devtools/";

don't think you really need this.

+//    placeholderText: aElement.getAttribute("data-placeholder"),

is that supposed to be here?

in +  _loadSourceFromCache: function SE__loadSourceFromCache(aHref)

nit:
you have some long lines (exceeding 90 columns!). They could stand to be split around the 80 column mark.

Nice method otherwise (if I can only complain about lines being too long, it's a pretty quiet review). :)

all good.

is that prettification algorithm based on anything or just your own?

in StyleEditorChrome:

populateChrome:
+      this._window.setTimeout(function queuedStyleSheetLoad() {
+        editor.load();
+      }, 0);
+    }

Would this work in a ChromeWorker instead?

StyleEditorChrome.xul

funky.

+<xul:script type="application/javascript"><![CDATA[
+Components.utils.import("resource:///modules/devtools/StyleEditorChrome.jsm");
+let chromeRoot = document.getElementById("style-editor-chrome");
+let chrome = new StyleEditorChrome(chromeRoot);
+window.styleEditorChrome = chrome;
+]]></xul:script>

I'm trying to think of a reason why we wouldn't want to do this, but I can't.

in Utils

+function _(aName)
+{
+

extra space after your opening bracket.

not sure I like the über convenient function name on this. It makes code that uses it harder to figure out. This is mostly a nit.

StyleEditor.dtd

+<!ENTITY styleeditor.label             "Style Editor">
+<!ENTITY styleeditor.accesskey         "y">
+<!ENTITY styleeditor.keycode           "VK_F9">
+<!ENTITY styleeditor.keytext           "F9">

F9 may collide with some users on OS X. It's commonly used for exposé features. I wish I had an alternative suggestion for you but there are basically no keys left.

Is this even needed in this file? It's included as well in browser.dtd.

gnomestripe/adaptiveSplitView.css:

+ol.splitview-nav > li.splitview-flash {
+  background-color: peachpuff;
+}

really? is that a common color on your operating system? :)

+.splitview-nav.splitview-all-filtered ~ .splitview-nav.placeholder.all-filtered {
+  background-color: peachpuff;
+}

what? again?

I don't really have any good comments on the CSS parts of this patch. Better to let Dão have his way with it.

Hopefully shorlander can have a look too.

+  skin/classic/browser/devtools/AdaptiveSplitView.css (devtools/AdaptiveSplitView.css)


Not sure these should live in a further subdirectory under themes/browser/*stripe/browser... devtools?

Should ask Dão where he'd like those to live, though I don't think we're quite ready to have a themes subdirectory under the browser/devtools section.

r+ for the JS bits. Lovely stuff!

Comment 56

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

Comment on attachment 557512 [details] [diff] [review]
Style Editor integration (tests) [2/2] - b=583041 r=robcee,msucan

test code looks solid.

Now that you're all geared up for Try server, you should push these over and link in the test results.

Great work!

Comment 57

[Cedric Vivier [:cedricv]]

Thanks for the r+!

I'll send an updated patch according to review comments and few style fixes asap (Dao please wait for the updated patch wrt CSS)

(In reply to Rob Campbell [:rc] (robcee) from comment #55)
> Comment on attachment 557511 [details] [diff] [review]
> Style Editor integration [1/2] - b=583041 r=robcee,msucan
> +const STYLE_EDITOR_CONTENT = "resource:///modules/devtools/";
> 
> don't think you really need this.

Yes, it was initially a helper for the add-on>integration scripts. I can remove that now.


> +//    placeholderText: aElement.getAttribute("data-placeholder"),
> 
> is that supposed to be here?

I can remove it. This depends on Bug .

> is that prettification algorithm based on anything or just your own?

I rolled over my own since we wanted to prettify only when strictly necessary and not touch the user's preferred indentation style otherwise.
It might need some tweaks according to user feedback later.


> in StyleEditorChrome:
> 
> populateChrome:
> +      this._window.setTimeout(function queuedStyleSheetLoad() {
> +        editor.load();
> +      }, 0);
> +    }
> 
> Would this work in a ChromeWorker instead?

I can try.


> +<!ENTITY styleeditor.label             "Style Editor">
> +<!ENTITY styleeditor.accesskey         "y">
> +<!ENTITY styleeditor.keycode           "VK_F9">
> +<!ENTITY styleeditor.keytext           "F9">
> 
> F9 may collide with some users on OS X. It's commonly used for exposé
> features. I wish I had an alternative suggestion for you but there are
> basically no keys left.

Yup, this shows my great familiarity with OSX ;)
I had a discussion with Alex Lakatos last week about this, we found they keys Shift-F5 and Shift-F7 to be available.
I'll use Shift-F5 unless someone has a batter idea.


> Is this even needed in this file? It's included as well in browser.dtd.

Oops indeed not :) That's a leftover from the add-on.


> +  skin/classic/browser/devtools/AdaptiveSplitView.css
> (devtools/AdaptiveSplitView.css)
> 
> Not sure these should live in a further subdirectory under
> themes/browser/*stripe/browser... devtools? (...) I don't think we're
> quite ready to have a themes subdirectory under the browser/devtools section.

The directory is already there containing Style Inspector files (?)

With our pretty DevTools skinning coming along with the Highlighter isn't this a good idea? We'll quickly have a significant number of CSS and resources afaik.

Comment 58

[Cedric Vivier [:cedricv]]

(In reply to Cedric Vivier [cedricv] from comment #57)
> > +//    placeholderText: aElement.getAttribute("data-placeholder"),
> > is that supposed to be here?
> 
> I can remove it. This depends on Bug .

Er. Bug 680371.

Comment 59

[Cedric Vivier [:cedricv]]

Comment on attachment 557511 [details] [diff] [review]
Style Editor integration [1/2] - b=583041 r=robcee,msucan

Will ask again for for Dao's review and Shorlander's ui-review on updated patch CSS-wise.

Comment 60

[Mihai Sucan [:msucan]]

I get:

Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch2.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://browser/content/browser.js :: delayedStartup :: line 7568"  data: no]

When I start Firefox with these two patches applied.

Cedric: you need to include the new preference (devtools.styleeditor.enabled) in browser/app/profile/firefox.js.

Comment 61

[Mihai Sucan [:msucan]]

Comment on attachment 557511 [details] [diff] [review]
Style Editor integration [1/2] - b=583041 r=robcee,msucan

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

Really great work Cedric! Awesome!

Here are my general review comments:

- When I open the Style Editor the Error Console shows:

Warning: XUL box for div element contained an inline small child, forcing all its children to be wrapped in a block.
Source File: chrome://browser/content/StyleEditorChrome.xul
Line: 0

It also shows when I play with the window size. Please fix this.

- For a follow up bug report: please add an option to revert the stylesheet changes to the initial state.

- Another follow up: please add an option to delete stylesheets.

- I suggest asking for a l10n review from Axel Hecht :Pike.

- I did only a bit of l10n and CSS reviewing, I'm leaving the rest to the experts.

More comments below. Please note I tried to not repeat what Rob said. :)

::: browser/devtools/styleeditor/AdaptiveSplitView.jsm
@@ +39,5 @@
> +
> +const EXPORTED_SYMBOLS = ["AdaptiveSplitView"];
> +
> +/* this must be kept in sync with CSS */
> +const LANDSCAPE_MEDIA_QUERY = "(min-aspect-ratio: 5/3)";

Please mention which CSS file we should keep in sync.

@@ +522,5 @@
> +function assert(aExpression, aMessage)
> +{
> +  if (!aExpression) {
> +    throw new Error("Assertion failed!\n" + aMessage);
> +  }

Why is this assert() implementation different from that in StyleEditorUtil.jsm? Please sync them, or even better, make sure that assert() is defined only in one place.

::: browser/devtools/styleeditor/StyleEditor.jsm
@@ +103,5 @@
> +  this._actionListeners = [];
> +
> +  // this is to perform pending updates before editor closing
> +  this._onWindowUnloadBinding = this._onWindowUnload.bind(this);
> +  // this is to proxies the focus event to underlying SourceEditor

this is to proxy the focus event ...

@@ +157,5 @@
> +  get inputElement() this._inputElement,
> +
> +  /**
> +   * Set the input element that handles display and input for this editor.
> +   * This detaches the previous input element if any were set.

if any *was* set.

@@ +238,5 @@
> +  /**
> +   * Setter for the read-only state of the editor.
> +   *
> +   * @param boolean aValue
> +   *        Tells if you want the editor to read-only or not.

to *be* read-only or not.

@@ +289,5 @@
> +   *        If not set a file picker will be shown.
> +   * @param nsIWindow aParentWindow
> +   *        Optional parent window for the file picker.
> +   */
> +  importFromFile: function SE_import(aFile, aParentWindow)

importFromFile: function SE_importFromFile(aFile, aParentWindow)

@@ +317,5 @@
> +    */
> +  get errorMessage() this._errorMessage,
> +
> +  /**
> +   * Retrieve whether the stylesheet has been loaded and ready for modifications.

Tells if the style sheet has been loaded and that it is ready for changes.

@@ +381,5 @@
> +    return this._friendlyName;
> +  },
> +
> +  /**
> +   * Add a listener for significant/semantic StyleEditor actions.

significant/semantic ?

@@ +407,5 @@
> +   *                           Arguments: (StyleEditor editor)
> +   * }
> +   *
> +   * A listener does not have to implement all of the interface above, actions
> +   * whose handler is not a function are ignored.

All listener methods are optional.

@@ +626,5 @@
> +    let source = this._state.text;
> +    let oldNode = this.styleSheet.ownerNode;
> +    let oldIndex = this.styleSheetIndex;
> +
> +    let newNode = this.contentDocument.createElement("style");

I would suggest here: createElementNS("http://www.w3.org/1999/xhtml", "style").

@@ +802,5 @@
> +  _appendNewStyleSheet: function SE__appendNewStyleSheet(aText)
> +  {
> +    let document = this.contentDocument;
> +    let parent = document.body;
> +    let style = document.createElement("style");

Again, I would like this code to use createElementNS() here.

@@ +807,5 @@
> +    style.setAttribute("type", "text/css");
> +    if (aText) {
> +      style.appendChild(document.createTextNode(aText));
> +    }
> +    parent.appendChild(style);

Why do you append to body?

@@ +893,5 @@
> +
> +  /**
> +    * Persist StyleEditor-specific to the attached DOM stylesheet expando.
> +    * The expando on the DOM stylesheet is used to restore  user-facing state
> +    * when the StyleEditor is closed and then reopened again.

Description is somewhat confusing. StyleEditor-specific (specific info?) to the ...

I am not sure how to rephrase this.

@@ +934,5 @@
> +    * SourceEditor action names are not displayed to the user.
> +    *
> +    * @return Array
> +    */
> +  _getKeyBindings: function () {

_getKeyBindings: function SE__getKeyBindings() {

These key bindings should be global to the StyleEditor chrome window, using xul:key elements, not editor-specific key bindings. Please change this or file a follow up bug about changing this post-landing.

@@ +984,5 @@
> +
> +/**
> + * Prettify minified CSS text.
> + * This prettify CSS code where there is no indentation in usual places while
> + * keeping original indentation as-is elsewhere.

This *prettifies* CSS code ...

@@ +991,5 @@
> + *        The CSS source to prettify.
> + * @return string
> + *         Prettified CSS source
> + */
> +function prettifyCSS(aText)

Shouldn't this function be in StyleEditorUtil.jsm?

@@ +1046,5 @@
> +  * @param string aText
> +  * @param number aCount
> +  * @return string
> +  */
> +function repeat(aText, aCount)

StyleEditorUtil.jsm?

::: browser/devtools/styleeditor/StyleEditorChrome.jsm
@@ +134,5 @@
> +    let onContentUnload = function () {
> +      aContentWindow.removeEventListener("unload", onContentUnload, false);
> +      this.contentWindow = null; // detach
> +    }.bind(this);
> +    aContentWindow.addEventListener("unload", onContentUnload, false);

this.contentWindow = null; detaches the StyleEditorChrome from the contentWindow when it is unloaded. What if the StyleEditorChrome is no longer attached to the same contentWindow? Won't this wrongly detach the StyleEditorChrome from a different contentWindow?

Can't unload fire *after* StyleEditorChrome.contentWindow is set to a new (and different) window object? You might want to do:

if (this.contentWindow == aContentWindow) {
  this.contentWindow = null; // detach
}

@@ +322,5 @@
> +
> +      // Queue editors loading so that ContentAttach is consistently triggered
> +      // right after all editor instances are available (this.editors) but are
> +      // NOT loaded/ready yet. This also helps responsivity during loading when
> +      // there is many heavy stylesheets.

when there *are* many heavy stylesheets.

::: browser/devtools/styleeditor/StyleEditorChrome.xul
@@ +73,5 @@
> +        <h5>&noStyleSheet.label;</h5>
> +        <small>&noStyleSheet-tip-start.label;
> +          <a href="#"
> +             class="style-editor-newButton">&noStyleSheet-tip-action.label;</a>
> +          &noStyleSheet-tip-end.label;</small>

From what I know about l10n, this is not appropriate. Languages are wildly different and this construct makes some language-related assumptions.

I would suggest changing this to use only one string (no start-middle-end strings). Make the entire line a link that allows the user to append a new stylesheet.

::: browser/devtools/styleeditor/StyleEditorUtil.jsm
@@ +172,5 @@
> + * @param object aDescriptor
> + *        An object describing how to wire matching selector, supported properties
> + *        are "events", "attributes" and "userData" taking objects themselves.
> + *        Each key of properties above represents the name of the event, attribute
> + *        or userData, with the value an function to use as the event handler,

with the value being a function use as the event handler, ...

::: browser/locales/en-US/chrome/browser/StyleEditor.dtd
@@ +11,5 @@
> +<!-- LOCALIZATION NOTE (window.title): This is the main title of Style Editor
> +     main window. The content document title follows between brackets, eg. if
> +     you open Style Editor on a web page titled "My homepage", the window title
> +     will be "Style Editor [My homepage]" -->
> +<!ENTITY window.title               "Style Editor">

The localization note is confusing. This string is the default window title for the Style Editor. From reading this note, one might expect that the JS appends on its own the " [web page title]" string.

You should mention that there's chromeWindowTitle in StyleEditor.properties and that string is used to dynamically update the Style Editor window title when a web page is loaded.

@@ +13,5 @@
> +     you open Style Editor on a web page titled "My homepage", the window title
> +     will be "Style Editor [My homepage]" -->
> +<!ENTITY window.title               "Style Editor">
> +
> +<!ENTITY newButton.label            "New stylesheet">

Please use "style sheet". The English spell checker tells me "stylesheet" is an unknown word. I also checked the CSS 2.1 spec:

http://www.w3.org/TR/CSS2/

the entire document does not include a single use of the two words "style sheet" glued together.

(Please update all references to "stylesheet" accordingly, in the .dtd and .properties files. Thanks!)

::: browser/locales/en-US/chrome/browser/StyleEditor.properties
@@ +1,3 @@
> +# LOCALIZATION NOTE  (chromeWindowTitle): This is the title of the Style Editor
> +# 'chrome' window. That is, the main window with the stylesheets list.
> +# The argument is either the content document's title or its href if no title

or its address

(href is related to an anchor)

@@ +4,5 @@
> +# is available.
> +chromeWindowTitle=Style Editor [%S]
> +
> +# LOCALIZATION NOTE  (inlineStyleSheet): This is the name used for an stylesheet
> +# that is declared inline in the <style> element. Shown in the stylesheets list.

This is the name used for *a* style sheet that is declared inline *using a* <style> element.

@@ +27,5 @@
> +# when you import a style sheet into the Style Editor.
> +importStyleSheet.title=Import stylesheet
> +
> +# LOCALIZATION NOTE  (importStyleSheet.title): This is the *.css filter title
> +importStyleSheet.filter=CSS stylesheets

"CSS files" or "Cascading Style Sheets"

@@ +34,5 @@
> +# when you save a style sheet from the Style Editor.
> +saveStyleSheet.title=Save stylesheet
> +
> +# LOCALIZATION NOTE  (saveStyleSheet.title): This is the *.css filter title
> +saveStyleSheet.filter=CSS stylesheets

Same as above.

::: browser/themes/gnomestripe/browser/devtools/AdaptiveSplitView.css
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + *
> + * All rules are prefixed with #style-editor-* in order to make integration
> + * with other XUL/CSS code (eg. browser) easier, less prone to conflicts.

None of the rules in this file are prefixed with #style-editor-. Please update/remove the comment as you see fit.

@@ +90,5 @@
> +ol.splitview-nav > li.splitview-flash {
> +  background-color: peachpuff;
> +}
> +ol.splitview-nav > li.splitview-slide {
> +  max-height: 0px;

max-height: 0;

::: browser/themes/gnomestripe/browser/devtools/StyleEditor.css
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + *
> + * All rules are prefixed with #style-editor-* in order to make integration
> + * with other XUL/CSS code (eg. browser) easier, less prone to conflicts.

None of the rules in this file are prefixed with #style-editor-. Please update/remove the comment as you see fit.

@@ +94,5 @@
> +  .stylesheet-title, .stylesheet-stats {
> +    display: none;
> +  }
> +  .stylesheet-editor-input {
> +    margin: 0px;

margin: 0;

@@ +104,5 @@
> +@media (max-width: 400px) {
> +  .splitview-filter {
> +    position: fixed;
> +    width: 100%;
> +    left: 0px;

left: 0;

@@ +110,5 @@
> +    -moz-transition-property: bottom;
> +    -moz-transition-duration: 0.5s;
> +  }
> +  .splitview-filter:focus {
> +    bottom: 0em;

bottom: 0;

@@ +113,5 @@
> +  .splitview-filter:focus {
> +    bottom: 0em;
> +  }
> +  .stylesheet-editor-input {
> +    margin: 0px;

margin: 0;

Comment 62

[Cedric Vivier [:cedricv]]

Thanks for the review Mihai. Will update the patches with your comments (no reply means ACK ;D), some comments/answers below:

(In reply to Mihai Sucan [:msucan] from comment #61)
> @@ +626,5 @@
> > +    let newNode = this.contentDocument.createElement("style");
> 
> I would suggest here: createElementNS("http://www.w3.org/1999/xhtml",
> "style").
> > +    let style = document.createElement("style");
> 
> Again, I would like this code to use createElementNS() here.

I'm not sure it provides any win.
You'd expect the editor to work on the default namespace of the document. Currently it works as-is with SVG or any document that accepts <style> without change (since it uses the document's default namespace).


> _getKeyBindings: function SE__getKeyBindings() {
> 
> These key bindings should be global to the StyleEditor chrome window, using
> xul:key elements, not editor-specific key bindings. Please change this or
> file a follow up bug about changing this post-landing.

I think it should stay editor-specific.
First, the key bindings like Save and al should work with StyleEditor alone (without the chrome), for instance when embedding it in other tools (eg. currently Tilt already embeds StyleEditor :) - but probably later with Style Inspector or others).
Also, the chrome could possibly display multiple editors simultaneously (eg. split mode).


> > +        <small>&noStyleSheet-tip-start.label;
> > +          <a href="#"
> > +             class="style-editor-newButton">&noStyleSheet-tip-action.label;</a>
> > +          &noStyleSheet-tip-end.label;</small>
> 
> From what I know about l10n, this is not appropriate. Languages are wildly
> different and this construct makes some language-related assumptions.
> 
> I would suggest changing this to use only one string (no start-middle-end
> strings). Make the entire line a link that allows the user to append a new
> stylesheet.

I've considered that and thought that "start/middle/end" (with any potentially empty) works with any language (it's not "left/right").
Will ask Alex during l10n review.

Comment 63

[Mihai Sucan [:msucan]]

(In reply to Cedric Vivier [cedricv] from comment #62)
> Thanks for the review Mihai. Will update the patches with your comments (no
> reply means ACK ;D), some comments/answers below:
> 
> (In reply to Mihai Sucan [:msucan] from comment #61)
> > @@ +626,5 @@
> > > +    let newNode = this.contentDocument.createElement("style");
> > 
> > I would suggest here: createElementNS("http://www.w3.org/1999/xhtml",
> > "style").
> > > +    let style = document.createElement("style");
> > 
> > Again, I would like this code to use createElementNS() here.
> 
> I'm not sure it provides any win.
> You'd expect the editor to work on the default namespace of the document.
> Currently it works as-is with SVG or any document that accepts <style>
> without change (since it uses the document's default namespace).

I believe it does provide a win, but you also have a good point about SVG documents. Therefore, we leave this as is for now. :)

My point was that you don't know what <style> means in other types of documents. Sure, for SVG it works. You can't assume <style> will work, therefore just using the namespace you know it works is best (IMO).

> > _getKeyBindings: function SE__getKeyBindings() {
> > 
> > These key bindings should be global to the StyleEditor chrome window, using
> > xul:key elements, not editor-specific key bindings. Please change this or
> > file a follow up bug about changing this post-landing.
> 
> I think it should stay editor-specific.
> First, the key bindings like Save and al should work with StyleEditor alone
> (without the chrome), for instance when embedding it in other tools (eg.
> currently Tilt already embeds StyleEditor :) - but probably later with Style
> Inspector or others).
> Also, the chrome could possibly display multiple editors simultaneously (eg.
> split mode).

Saving should be a matter for the "parent" user. The SourceEditor is a "widget", it's a fancy "textarea". It is up to the form-builder to save the form. So, even if there are multiple style editors, or even in the case of Tilt, saving should be handled by the code that uses the editor.

I would imagine that in the case of Tilt or the Style Editor chrome, when Ctrl-S is pressed there can be a check for "which is the active editor?" and call the StyleEditor.updateStyleSheet() method. In this way ... saving would also be customizable. For example, one could implement save-to-github or anything else, on top of the updateStyleSheet() method.

I agree that we can leave this as-is. This is something of details. :)

(unless Rob has strong opinions towards a direction)


> > > +        <small>&noStyleSheet-tip-start.label;
> > > +          <a href="#"
> > > +             class="style-editor-newButton">&noStyleSheet-tip-action.label;</a>
> > > +          &noStyleSheet-tip-end.label;</small>
> > 
> > From what I know about l10n, this is not appropriate. Languages are wildly
> > different and this construct makes some language-related assumptions.
> > 
> > I would suggest changing this to use only one string (no start-middle-end
> > strings). Make the entire line a link that allows the user to append a new
> > stylesheet.
> 
> I've considered that and thought that "start/middle/end" (with any
> potentially empty) works with any language (it's not "left/right").
> Will ask Alex during l10n review.

Sure. Please ask Alex for a review and he'll definitely have an opinion on this.

Thank you! Looking forward for the updated patch.

Comment 64

[Mihai Sucan [:msucan]]

Comment on attachment 557512 [details] [diff] [review]
Style Editor integration (tests) [2/2] - b=583041 r=robcee,msucan

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

Tests seem fine, good code! Awesome work!

Here are the comments:

- I get tests failing:

TEST-START | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser/browser_styleeditor_enabled.js
TEST-INFO | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser/browser_styleeditor_enabled.js | Console message: [JavaScript Error: "redeclaration of var Cc" {file: "chrome://browser/content/content.js" line: 3}]
TEST-INFO | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser/browser_styleeditor_enabled.js | Console message: [JavaScript Warning: "XUL box for div element contained an inline small child, forcing all its children to be wrapped in a block." {file: "chrome://browser/content/StyleEditorChrome.xul" line: 0}]

It tries to open:
https://example.com/browser/browser/base/content/test/StyleEditor/simple.html

... and it says 404 Not found, that's why the test breaks.

More errors:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser/browser_styleeditor_enabled.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser/browser_styleeditor_import.js | there is 2 stylesheets initially - Got 0, expected 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser/browser_styleeditor_loading.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser/browser_styleeditor_reopen.js | Test timed out
make: *** [mochitest-browser-chrome] Erreur 1


- After fixing the TEST_BASE path in some of the files, I still get failures:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser/browser_styleeditor_import.js | there is 2 stylesheets initially - Got 0, expected 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser/browser_styleeditor_indent.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser/browser_styleeditor_reopen.js | first stylesheet still has UNSAVED flag at reopening
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser/browser_styleeditor_reopen.js | Test timed out
make: *** [mochitest-browser-chrome] Erreur 1

Please fix these tests and send the updated patch to the try server. Thank you!

::: browser/devtools/styleeditor/test/Makefile.in
@@ +18,5 @@
> +# Portions created by the Initial Developer are Copyright (C) 2010
> +# the Initial Developer. All Rights Reserved.
> +#
> +# Contributor(s):
> +#     Cedric Vivier <[e-mail]>  (Original author)

Please align Cedric with "n":

# Contributor(s):
#   Cedric Vivier <[e-mail]> (Original author)

@@ +41,5 @@
> +VPATH		= @srcdir@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +DIRS = browser

I believe you don't need to nest your tests in test/browser/. Just go for test/ directly.

::: browser/devtools/styleeditor/test/browser/browser_styleeditor_asv_filter.js
@@ +7,5 @@
> +
> +
> +function test()
> +{
> +  registerCleanupFunction(cleanup);

I believe you shouldn't have this call here. Please move it to head.js.

::: browser/devtools/styleeditor/test/browser/browser_styleeditor_asv_keynav.js
@@ +43,5 @@
> +  EventUtils.synthesizeMouse(summary, 10, 1, {}, gChromeWindow);
> +
> +  waitForFocus(function () {
> +    let item = getStylesheetNameLinkFor(gChrome.editors[0]);
> +    ok(gChromeWindow.document.activeElement == item,

is(gChromeWindow.document.activeElement, item,

(same for the rest of ok() calls in this file, where appropriate)

@@ +76,5 @@
> +{
> +  ok(aEditor.sourceEditor.hasFocus(),
> +     "editor 2 has focus");
> +
> +  finish();

You do not clear the gChrome variable. This can leak memory.

::: browser/devtools/styleeditor/test/browser/browser_styleeditor_asv_resize.js
@@ +4,5 @@
> +
> +const TEST_BASE = "chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser/";
> +const TESTCASE_URI = TEST_BASE + "simple.html";
> +
> +let gOriginalWidth; // those are set by run() when gChromeWindow is ready

*these* are set by ...

@@ +14,5 @@
> +    if (gOriginalWidth && gOriginalHeight) {
> +      window.resizeTo(gOriginalWidth, gOriginalHeight);
> +    }
> +    cleanup();
> +  });

This doesn't make sense. Problems:

1. gChromeWindow is the StyleEditor chrome window. You store its original width and height sizes, when it first opens, in the run() function. then when the test ends, you have the resizeToOriginalAndCleanup() function registered as a cleanup function for this mochitest. In this function you reset to the original size the global window object, which is not the same as gChromeWindow. |window| here refers to the main browser chrome window, not the Style Editor chrome window.

2. Why do you reset Style Editor chrome window back to the original size? You'll close it, anyway.

3. You should not call the cleanup() function here. That should be called on its own. See my comments for head.js.

Please fix this. Thank you!

::: browser/devtools/styleeditor/test/browser/browser_styleeditor_enabled.js
@@ +2,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// https rather than chrome to improve coverage
> +const TEST_BASE = "https://example.com/browser/browser/base/content/test/StyleEditor/";

Wrong path. Test breaks. Perhaps this should be:

const TEST_BASE = "https://example.com/browser/browser/devtools/styleeditor/test/browser/";

::: browser/devtools/styleeditor/test/browser/browser_styleeditor_loading.js
@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const TEST_BASE = "https://example.com/browser/browser/base/content/test/StyleEditor/";

Wrong path. Test breaks. This should probably be:

const TEST_BASE = "https://example.com/browser/browser/devtools/styleeditor/test/browser/";

@@ +28,5 @@
> +
> +function run(aChrome)
> +{
> +  is(aChrome.contentWindow.document.readyState, "complete",
> +     "content document is complete");

What is the importance/need for this mochitest file? Beyond testing implementation details of how loading works.

::: browser/devtools/styleeditor/test/browser/browser_styleeditor_new.js
@@ +105,5 @@
> +      executeSoon(function () {
> +        is(gCommitCount, 1, "received only one Commit event (throttle)");
> +
> +        aEditor.removeActionListener(listener);
> +        finish();

You do not clear the gNewEditor variable. Potential memory leak.

::: browser/devtools/styleeditor/test/browser/browser_styleeditor_readonly.js
@@ +8,5 @@
> +
> +let gEditorAddedCount = 0;
> +let gEditorReadOnlyCount = 0;
> +let gChromeListener = {
> +  onEditorAdded: function continueWhenAllAreLoaded(aChrome, aEditor) {

Function name is too different from the method name. Please update.

@@ +24,5 @@
> +        gBrowser.removeCurrentTab();
> +      });
> +    }
> +  },
> +  onContentDetach: function (aChrome) {

Inconsistency: the previous method has a name, this one doesn't. Please either give names to both functions, or don't.

::: browser/devtools/styleeditor/test/browser/browser_styleeditor_reopen.js
@@ +2,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// http rather than chrome to improve coverage
> +const TEST_BASE = "http://example.com/browser/browser/base/content/test/StyleEditor/";

Wrong path. Test breaks. This should probably be:

const TEST_BASE = "https://example.com/browser/browser/devtools/styleeditor/test/browser/";

@@ +83,5 @@
> +  waitForFocus(function () {
> +    // insert char so that this stylesheet has the UNSAVED flag
> +    EventUtils.synthesizeKey("x", {}, gChromeWindow);
> +
> +    gOldChromeWindow = gChromeWindow;

gOldChromeWindow is not defined this mochitest file. You are assigning here to an undefined variable.

::: browser/devtools/styleeditor/test/browser/four.html
@@ +1,3 @@
> +<!doctype html>
> +<html>
> +<head>

Please add the PD license boilerplate to this file and the other test files that do not have it.

::: browser/devtools/styleeditor/test/browser/head.js
@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +let gStyleEditor = StyleEditor;  //StyleEditor object in browser window

Is gStyleEditor used anywhere? Why the alias?

@@ +5,5 @@
> +let gStyleEditor = StyleEditor;  //StyleEditor object in browser window
> +let gChromeWindow;               //StyleEditorChrome window
> +
> +
> +function cleanup()

You need to do registerCleanupFunction(cleanup) at the end of head.js, to make sure that cleanup() is always called.

@@ +8,5 @@
> +
> +function cleanup()
> +{
> +  gStyleEditor = null;
> +  gChromeWindow.close();

You should probably check if (gChromeWindow) { ... } because this file only sets gChromeWindow when some specific functions are called - those may not always be called.

Comment 65

[Mihai Sucan [:msucan]]

Comment on attachment 557511 [details] [diff] [review]
Style Editor integration [1/2] - b=583041 r=robcee,msucan

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

Something I forgot in my previous review. See below.

::: browser/devtools/styleeditor/StyleEditor.jsm
@@ +217,5 @@
> +      if (this._focusOnSourceEditorReady) {
> +        sourceEditor.focus();
> +      }
> +
> +      sourceEditor.addEventListener("TextChanged", function onTextChanged() {

Instead of "TextChanged" please use SourceEditor.EVENTS.TEXT_CHANGED. Thanks!

Comment 66

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration [1/2]

This addresses all review comments except the XUL box warning that I'm puzzled about (Dao might have an idea) but it can be removed after landing I believe.

Updated patches based off add-on v0.3.1 :
http://neonux.com/StyleEditor/builds/StyleEditor-0.3.1.xpi
http://neonux.com/StyleEditor/coverage/0.3.1/

Add-on's Automatic transition and bracket completion to be integrated in follow-up bugs/patches.

Comment 67

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration (tests) [2/2]

Comment 68

[Cedric Vivier [:cedricv]]

Try build underway: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=38bbc1b21dec

Comment 69

[Cedric Vivier [:cedricv]]

I meant https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=9fbde02a0946

Comment 70

[Cedric Vivier [:cedricv]]

New try with small fixes on tests (got a few timeouts due to missing waitForFocus probably) : https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=8b7fb7c4681b

Comment 71

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

still have some oranges in there.

Comment 72

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

cedric, could you post an interdiff between those versions please? I'd like to see what changed easily and bugzilla's interdiff is... not so reliable.

Comment 73

[Cedric Vivier [:cedricv]]

Attachment: Fix potential SourceEditor's ready callback invocation before Orion's iframe is ready. r=msucan,robcee

Comment 74

[Cedric Vivier [:cedricv]]

Attachment: Bug 583041 - Style Editor integration [1/2]. r=robcee,msucan

Comment 75

[Cedric Vivier [:cedricv]]

Attachment: Bug 583041 - Style Editor integration (tests) [2/2]. r=robcee,msucan

Comment 76

[Cedric Vivier [:cedricv]]

Above are updated/rebased patches that look all green on try (modulo intermittent oranges unrelated to Style Editor) :

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=17822a39218e

Based off add-on v0.3.2 with 91% coverage - http://neonux.com/StyleEditor/coverage/0.3.2/

Details on the changes since last browser patches based off 0.3.1 :
https://github.com/neonux/StyleEditor/compare/v0.3.1...v0.3.2

(the biggest change being https://github.com/neonux/StyleEditor/commit/121cb6fcae6b0170c95fed508ced972f5319028a which made it pass on all platforms [previous code was subtly wrong])

Comment 77

[Cedric Vivier [:cedricv]]

Attachment: Interdiff v0.3.1..v0.3.2

https://github.com/neonux/StyleEditor/compare/v0.3.1...v0.3.2.diff

Comment 78

[Mihai Sucan [:msucan]]

Comment on attachment 564518 [details] [diff] [review]
Fix potential SourceEditor's ready callback invocation before Orion's iframe is ready. r=msucan,robcee

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

A quick drive-by comment.

The point of the this is to try to avoid problems when someone wants to update Orion in the codebase ... without double-checking source-editor-orion.jsm and the changes that have happened inside orion.js. We can rely on public properties and methods, but we cannot make any kind of assumptions about the private ones. This patch should be considered as a workaround for a potential Orion initialization issue, that applies only to the current Orion code version (and it may no longer apply to subsequent updates).

Thank you!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +221,5 @@
>      }, this);
>  
>      if (aCallback) {
> +      let iframe = this._view._frame;
> +      let document = iframe.contentDocument;

If the _frame property does not exist then this line will throw. Please check the _frame existence.

@@ +222,5 @@
>  
>      if (aCallback) {
> +      let iframe = this._view._frame;
> +      let document = iframe.contentDocument;
> +      if (!document || document.readyState != "complete") {

if (!document) can happen if the _frame property no longer points to an iframe element (in a future version of Orion). Please check if _frame is instanceof Ci.nsIDOMHTMLIFrameElement.

Comment 79

[Cedric Vivier [:cedricv]]

(In reply to Mihai Sucan [:msucan] from comment #78)
> ::: browser/devtools/sourceeditor/source-editor-orion.jsm
> >      if (aCallback) {
> > +      let iframe = this._view._frame;
> > +      let document = iframe.contentDocument;
> 
> If the _frame property does not exist then this line will throw. Please
> check the _frame existence.

Yes, and that's a good thing imho :)
As you said earlier _frame is guaranteed here (which tests shows it indeed is).
If it's null then something really gone wrong, therefore it should fail fast and throw here, rather than invoking the callback as if all is good and then subtly fail later/harder.


> 
> @@ +222,5 @@
> >  
> >      if (aCallback) {
> > +      let iframe = this._view._frame;
> > +      let document = iframe.contentDocument;
> > +      if (!document || document.readyState != "complete") {
> 
> if (!document) can happen if the _frame property no longer points to an
> iframe element (in a future version of Orion). Please check if _frame is
> instanceof Ci.nsIDOMHTMLIFrameElement.

If this is a problem with a future version of Orion, let's fix it in the future? :p
I mean by that, with current code the test would be purely superfluous. If/whenever Orion does not use an iframe it would obviously fail 2 lines below. Which is good since we then simply remove this whole piece of code altogether (vs keeping dead code).

Comment 80

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

Comment on attachment 564519 [details] [diff] [review]
Bug 583041 - Style Editor integration [1/2]. r=robcee,msucan

in +++ b/browser/devtools/styleeditor/Makefile.in

+# The Initial Developer of the Original Code is Mozilla Corporation.

it's only build code, but we have to be careful with our licenses. s/Corporation/Foundation/

in StyleEditor.jsm

+    let config = {
+//    placeholderText: aElement.getAttribute("data-placeholder"),

this is still here. Yank it!

I think we're going to need a fallback in case we don't make orion enabled by default. We can do that in a followup bug for now.

Comment 81

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

Comment on attachment 564521 [details] [diff] [review]
Bug 583041 - Style Editor integration (tests) [2/2]. r=robcee,msucan

all tests passed locally. Lookin' good.

Comment 82

[Dão Gottwald [:dao]]

Comment on attachment 564519 [details] [diff] [review]
Bug 583041 - Style Editor integration [1/2]. r=robcee,msucan

>--- a/browser/themes/winstripe/browser/jar.mn
>+++ b/browser/themes/winstripe/browser/jar.mn
>@@ -240,8 +240,11 @@ browser.jar:
>         skin/classic/aero/browser/sync-desktopIcon.png
>         skin/classic/aero/browser/sync-mobileIcon.png
>         skin/classic/aero/browser/sync-notification-24.png
>         skin/classic/aero/browser/syncSetup.css
>         skin/classic/aero/browser/syncCommon.css
>         skin/classic/aero/browser/syncQuota.css
> #endif
> #endif
>+  skin/classic/browser/devtools/AdaptiveSplitView.css (devtools/AdaptiveSplitView.css)
>+  skin/classic/browser/devtools/StyleEditor.css (devtools/StyleEditor.css)
>+  skin/classic/browser/devtools/gimp-eye.png (devtools/gimp-eye.png)

This is broken (not just as far as the indentation is concerned).

Comment 83

[Cedric Vivier [:cedricv]]

Attachment: Address robcee,dao comments on [1/2]

Comment 84

[Cedric Vivier [:cedricv]]

Attachment: Address robcee comment on [2/2]

Comment 85

[Mihai Sucan [:msucan]]

Comment on attachment 565187 [details] [diff] [review]
Address robcee comment on [2/2]

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

Good work. Glad to see the tests fixed!

I just did a quick interdiff between my older Style Editor patch I had laying in mq, and against the older SE patches you posted in this bug - just to see what changed. Congratulations for the fixes!

I only have one nit below. :)

::: browser/devtools/styleeditor/test/browser_styleeditor_enabled.js
@@ +77,5 @@
> +        // now toggle it back to enabled
> +        waitForFocus(function () {
> +          //! does nothing on Win7 specifically...
> +          //EventUtils.synthesizeMouseAtCenter(enabledToggle, {}, gChromeWindow);
> +          enabledToggle.click();

Nit: the "middle ground" is to use EventUtils.sendMouseEvent() instead of synthesizeMouse(). sendMouseEvent() constructs the DOM events for click and so on. I would favor that, instead of directly calling .click().

@@ +99,5 @@
> +
> +  waitForFocus(function () {
> +    //! does nothing on Win7 specifically...
> +    //EventUtils.synthesizeMouseAtCenter(enabledToggle, {}, gChromeWindow);
> +    enabledToggle.click();

Same as above.

Comment 86

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

Comment on attachment 564518 [details] [diff] [review]
Fix potential SourceEditor's ready callback invocation before Orion's iframe is ready. r=msucan,robcee

is this patch still relevant?

Comment 87

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

Comment on attachment 565186 [details] [diff] [review]
Address robcee,dao comments on [1/2]

R+ on these with Mihai's comments addressed.

Thanks!

Comment 88

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

Comment on attachment 565186 [details] [diff] [review]
Address robcee,dao comments on [1/2]

Dao, could you take a look at the CSS in here? Thanks!

Comment 89

[Dão Gottwald [:dao]]

Comment on attachment 565186 [details] [diff] [review]
Address robcee,dao comments on [1/2]

>+++ b/browser/base/content/browser.js

>+    let contentWindow = Services.wm.getMostRecentWindow("navigator:browser")
>+                          .gBrowser.selectedBrowser.contentWindow;
>+    let enumerator = Services.wm.getEnumerator(CHROME_WINDOW_TYPE);
>+    while(enumerator.hasMoreElements()) {
>+      var win = enumerator.getNext();
>+      if (win.styleEditorChrome.contentWindow == contentWindow) {
>+        win.focus();
>+        return win;
>+      }
>+    }

nit: "while (" (space added)

More importantly, this isn't e10s-ready. I don't think this is acceptable for new code.

>+++ b/browser/themes/gnomestripe/browser/devtools/AdaptiveSplitView.css

>+:root {
>+  background-color: -moz-dialog;
>+  margin: 0;
>+  padding: 0;

Aren't these three properties redundant for xul windows?

>+  width: 100%;
>+  height: 100%;
>+  overflow: hidden;

Are these actually needed?

>+  font-size: 14px;

Where does this number come from? In general, we should respect system settings for font sizes and use relative values when we need to make text smaller or larger.

>+html, body,

StyleEditorChrome.xul doesn't have such nodes, does it?

>+.splitview-root,
>+.splitview-controller,
>+.splitview-main,
>+.splitview-nav-container,
>+.splitview-nav,
>+.splitview-inline-details,
>+.splitview-side-details,
>+.splitview-details {
>+  display: -moz-box;
>+  -moz-box-flex: 1;
>+  -moz-box-orient: vertical;
>+}

Should these just be xul vbox elements?

>+ol.splitview-nav > li.splitview-filtered {
>+  display: none;
>+}

This should be in content CSS.

>+/* inline details are not shown unless parent active */
>+.splitview-inline-details, .splitview-side-details {
>+  display: none;
...
>+}
>+li.splitview-active .splitview-inline-details {
>+  display: -moz-box;
>+}

ditto

>+.splitview-nav:empty,
>+.splitview-nav.splitview-all-filtered,
>+.splitview-nav + .splitview-nav.placeholder,
>+.splitview-filter-bar {
>+  display: none;
>+}
>+.splitview-nav.splitview-all-filtered ~ .splitview-nav.placeholder.all-filtered,
>+.splitview-nav:empty ~ .splitview-nav.placeholder.empty {
>+  display: -moz-box;

ditto

>+++ b/browser/themes/gnomestripe/browser/devtools/StyleEditor.css

>+.unsaved .stylesheet-name {
>+  font-style: italic;
>+}
>+.unsaved .stylesheet-name:before {
>+  content: "* ";
>+}

Can these selectors be made more efficient, using child combinators?

>+li hgroup {

ditto

>+  display: block;
>+  float: left;

Is this correct for RTL?

>+h1, h2, h3 {

nit: line break after each comma

Comment 90

[Cedric Vivier [:cedricv]]

Some replies/questions... I ack and/or investigate removed parts.

(In reply to Dão Gottwald [:dao] from comment #89)
> >+.splitview-root,
> >+.splitview-controller,
> >+.splitview-main,
> >+.splitview-nav-container,
> >+.splitview-nav,
> >+.splitview-inline-details,
> >+.splitview-side-details,
> >+.splitview-details {
> >+  display: -moz-box;
> >+  -moz-box-flex: 1;
> >+  -moz-box-orient: vertical;
> >+}
> 
> Should these just be xul vbox elements?

I'd rather not mix HTML with XUL here (the UI is HTML).
Also orient and flex need to change according to aspect-ratio, so using cascading/media queries really makes sense here imho.
Otoh it can be simplified, a few of those classes can be removed.


> 
> >+ol.splitview-nav > li.splitview-filtered {
> >+  display: none;
> >+}
> 
> This should be in content CSS.

I'm not sure to understand what do you mean here?

Comment 91

[Dão Gottwald [:dao]]

> > >+ol.splitview-nav > li.splitview-filtered {
> > >+  display: none;
> > >+}
> > 
> > This should be in content CSS.
> 
> I'm not sure to understand what do you mean here?

This is application logic, not styling. There should be a css file in browser/devtools/styleeditor/ that contains this rule (and other theme-independent ones).

Comment 92

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

(In reply to Dão Gottwald [:dao] from comment #89)
> Comment on attachment 565186 [details] [diff] [review] [diff] [details] [review]

> More importantly, this isn't e10s-ready. I don't think this is acceptable
> for new code.

We're planning on using linked iframes or browsers for this when they're ready. That shouldn't block this.

Comment 93

[:Gavin Sharp [email: [e-mail]]]

How far away are they from being ready? Doing a bunch of work here that will need to be redone shortly doesn't seem ideal.

The [1/2] patch uses getMostRecentWindow, which should generally never be used. Use explicit references to the current tab/window instead.

Comment 94

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

(In reply to Gavin Sharp (use [e-mail] for email) from comment #93)
> How far away are they from being ready? Doing a bunch of work here that will
> need to be redone shortly doesn't seem ideal.

work is progressing in bug 669698. Olli's close, has some test-failures, but I expect it'll be ready within a month or two.

> The [1/2] patch uses getMostRecentWindow, which should generally never be
> used. Use explicit references to the current tab/window instead.

Yup, we're hashing that out. Good suggestion.

Comment 95

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration [1/2]

Updated patch based off add-on v0.5 (stripped of new features added since 0.2).
http://neonux.com/StyleEditor/coverage/0.5.0/

Notes on changes:
- latest review comments addressed
- renamed AdaptiveSplitView => SplitView
- modified UI layout, which paves the way to 687702 - which enabled simplifying a bit CSS, SplitView (removed code), and tests.


Try is underway: https://tbpl.mozilla.org/?tree=Try&rev=a6d96506ec5d

Comment 96

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration (tests) [2/2]

Comment 97

[Cedric Vivier [:cedricv]]

Attachment: Interdiff with previous patches based off 0.3.2

Comment 98

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

Comment on attachment 569643 [details] [diff] [review]
Style Editor integration [1/2]

requesting additional reviews from Dao for splitview.css and styleeditor.css, and l10n review for StyleEditor.properties.

Comment 99

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

Comment on attachment 569643 [details] [diff] [review]
Style Editor integration [1/2]

moving review to the proper "l10n".

Comment 100

[Axel Hecht [:Pike]]

Comment on attachment 569643 [details] [diff] [review]
Style Editor integration [1/2]

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

Should the l10n files be in devtools? Also, see the comments below.

::: browser/locales/en-US/chrome/browser/StyleEditor.properties
@@ +14,5 @@
> +# user-created style sheet.
> +newStyleSheet=New style sheet #%S
> +
> +# LOCALIZATION NOTE  (ruleCount.label): This is shown in the stylesheets list.
> +ruleCount.label=%S rules.

I guess this should use a pluralform.

@@ +38,5 @@
> +saveStyleSheet.filter=CSS files
> +
> +# LOCALIZATION NOTE  (saveStyleSheet.accessKey): This the key to use in
> +# conjunction with accel (Command on Mac or Ctrl on other platforms) to Save
> +saveStyleSheet.accessKey=S

Reading this comment, this is a control and not an accesskey? Seems we have one in .dtd, too?

Comment 101

[Cedric Vivier [:cedricv]]

Attachment: l10n updates according to comment #100

Comment 102

[Cedric Vivier [:cedricv]]

(In reply to Axel Hecht [:Pike] from comment #100)
> Should the l10n files be in devtools? Also, see the comments below.

I guess this makes sense, I did not we have devtools/ now before bug 687851 thanks to gcli.
Done.


> ::: browser/locales/en-US/chrome/browser/StyleEditor.properties
> > +ruleCount.label=%S rules.
> 
> I guess this should use a pluralform.

Done.


> @@ +38,5 @@
> > +saveStyleSheet.accessKey=S
> Reading this comment, this is a control and not an accesskey?

Done.

> Seems we have one in .dtd, too?

One is for the style sheets list. One for the editors.

Comment 103

[Dave Camp (:dcamp)]

Comment on attachment 569678 [details] [diff] [review]
l10n updates according to comment #100

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

::: browser/themes/pinstripe/browser/jar.mn
@@ +127,5 @@
>    skin/classic/browser/devtools/csshtmltree.css             (devtools/csshtmltree.css)
>    skin/classic/browser/devtools/gcli.css                    (devtools/gcli.css)
> +  skin/classic/browser/devtools/SplitView.css               (devtools/SplitView.css)
> +  skin/classic/browser/devtools/StyleEditor.css             (devtools/StyleEditor.css)
> +  skin/classic/browser/devtools/gimp-eye.png                (devtools/gimp-eye.png)

Err, is this gimp-eye.png actually from Gimp?  I imagine there are licensing incompatibilities (or at least attribution issues) to worry about here.

Comment 104

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration [1/2]

(In reply to Dave Camp (:dcamp) from comment #103):

Good catch. Replaced it with Shorlander's eye (but inverted so that it 'works' with the current light-background design).

Comment 105

[Axel Hecht [:Pike]]

Comment on attachment 569729 [details] [diff] [review]
Style Editor integration [1/2]

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

r=me with the nit below.

::: browser/locales/en-US/chrome/browser/devtools/StyleEditor.properties
@@ +14,5 @@
> +# user-created style sheet.
> +newStyleSheet=New style sheet #%S
> +
> +# LOCALIZATION NOTE  (ruleCount.label): This is shown in the stylesheets list.
> +ruleCount.label=#1 rule.;#1 rules.

Make that localization note something like

# LOCALIZATION NOTE (downloadsTitleFiles): Semi-colon list of plural forms.
# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
# #1 number of files
# example: 111 files - Downloads

(copied straight from https://developer.mozilla.org/en/Localization_and_Plurals)

The dev docs link is the only hint I have to trigger the plural-specific checks in compare-locales, sadly.

Comment 106

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration [1/2]

(In reply to Axel Hecht [:Pike] from comment #105)

Thanks! Updated.

Comment 109

[Dão Gottwald [:dao]]

Comment on attachment 569753 [details] [diff] [review]
Style Editor integration [1/2]

> browser/devtools/styleeditor/SplitView.css

rename to splitView.css

> browser/devtools/styleeditor/StyleEditorChrome.xul

rename to styleEditor.xul

> browser/locales/en-US/chrome/browser/devtools/StyleEditor.dtd
> browser/locales/en-US/chrome/browser/devtools/StyleEditor.properties

rename to styleEditor.dtd/properties

> browser/themes/gnomestripe/browser/devtools/StyleEditor.css

rename to styleEditor.css

> browser/themes/gnomestripe/browser/devtools/SplitView.css

rename to splitView.css (or styleEditorSplitView.css? Why is this a separate stylesheet in the first place?)

>+++ b/browser/base/content/browser-appmenu.inc
>+          <menuitem id="appmenu_styleEditor"
>+                    hidden="true"
>+                    label="&styleeditor.label;"
>+                    key="key_styleeditor"

decide for *_styleEditor or *_styleeditor for the ids and stick with it

>+++ b/browser/base/content/browser.js
>+    while(enumerator.hasMoreElements()) {

while (

>+++ b/browser/devtools/styleeditor/StyleEditorChrome.xul
>+        <input class="splitview-filter"
>+               type="search"
>+               title="&searchInput.tooltip;"
>+               placeholder="&searchInput.placeholder;"/>

I think you want a XUL textbox here for proper search field styling.

>+++ b/browser/themes/gnomestripe/browser/devtools/SplitView.css
>@@ -0,0 +1,139 @@
>+/*
>+ * Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */

Why is this public domain rather than MPL?

>+:root {
>+  font-family: sans-serif;
>+}

I don't think this makes sense... The default chrome font should work just fine.

>+.splitview-filter {
>+  margin-left: 1ex;
>+}

This is wrong for RTL.

>+.splitview-filter.splitview-all-filtered:focus {
>+  background-color: #eaa;
>+}

This disables native theming, unfortunately...

>+.splitview-nav > li > .stylesheet-more {
>+  position: relative;
>+  right: 0;
>+}

Shouldn't this be on the left side for RTL?

>+.splitview-nav > li hgroup.stylesheet-stats,
>+.splitview-nav > li hgroup.stylesheet-actions {
>+  right: -100px;

This looks like it isn't going to work for locales with longer strings.

>+ol.splitview-nav:focus {
>+  outline: 0; /* focusable only for focus stealing */

I'm not sure what this comment means, but removing the outline makes it unclear what's focused when tabbing through the UI.

>+ol.splitview-nav > li {
>+  display: -moz-box;
>+  -moz-box-orient: vertical;
>+  outline: 0;
>+  border-radius: 4px;
>+  border-bottom: 1px solid -moz-cellhighlight;

-moz-cellhighlight is supposed to be used as a background color. See lines 65-86: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/listbox.css#65

>+ol.splitview-nav > li.splitview-active {
>+  background-color: rgb(225,235,251); /* same as SourceEditor gutter */
>+}
>+ol.splitview-nav > li:hover, ol.splitview-nav > li:focus {
>+  background-color: rgb(240,245,253);
>+}
>+ol.splitview-nav > li.splitview-flash {
>+  background-color: #faf0e1; /* complement of active color #e1ebfb */
>+}

If you set a background color, you need to specify a fitting foreground color (e.g. black). However, these colors should probably be the same as those in listbox.css.

>+/* chrome fixes */

What does this mean?

Comment 110

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #109)
> Comment on attachment 569753 [details] [diff] [review] [diff] [details] [review]
> > browser/themes/gnomestripe/browser/devtools/SplitView.css
> 
> rename to splitView.css (or styleEditorSplitView.css? Why is this a separate
> stylesheet in the first place?)

Separation of concerns. The SplitView UI has nothing StyleEditor-specific in it and we plan using it in other tools.


> >+++ b/browser/devtools/styleeditor/StyleEditorChrome.xul
> >+        <input class="splitview-filter"
> >+               type="search"
> >+               title="&searchInput.tooltip;"
> >+               placeholder="&searchInput.placeholder;"/>
> 
> I think you want a XUL textbox here for proper search field styling.



> 
> >+++ b/browser/themes/gnomestripe/browser/devtools/SplitView.css
> >@@ -0,0 +1,139 @@
> >+/*
> >+ * Any copyright is dedicated to the Public Domain.
> >+ * http://creativecommons.org/publicdomain/zero/1.0/
> >+ */
> 
> Why is this public domain rather than MPL?



> 
> >+:root {
> >+  font-family: sans-serif;
> >+}
> 
> I don't think this makes sense... The default chrome font should work just
> fine.

It doesn't work because the document is html so the default font is serif.


> >+.splitview-filter {
> >+  margin-left: 1ex;
> >+}
> 
> This is wrong for RTL.
> 
> >+.splitview-filter.splitview-all-filtered:focus {
> >+  background-color: #eaa;
> >+}
> 
> This disables native theming, unfortunately...

We're going away from native theming for devtools anyways.


> 
> >+.splitview-nav > li > .stylesheet-more {
> >+  position: relative;
> >+  right: 0;
> >+}
> 
> Shouldn't this be on the left side for RTL?

I'm not sure either. I'd think not.


> 
> >+.splitview-nav > li hgroup.stylesheet-stats,
> >+.splitview-nav > li hgroup.stylesheet-actions {
> >+  right: -100px;
> 
> This looks like it isn't going to work for locales with longer strings.



> 
> >+ol.splitview-nav:focus {
> >+  outline: 0; /* focusable only for focus stealing */
> 
> I'm not sure what this comment means, but removing the outline makes it
> unclear what's focused when tabbing through the UI.

I'll clarify the comment. When the nav (ie. list) is focused, the selected item in the list is focused directly, to give more sensible (native-like) behavior.


> >+/* chrome fixes */
> 
> What does this mean?

I'll remove it. It was there earlier because of few rules to override some chrome  styles that didn't make much sense here.

Comment 111

[Dão Gottwald [:dao]]

(In reply to Cedric Vivier [cedricv] from comment #110)
> > >+:root {
> > >+  font-family: sans-serif;
> > >+}
> > 
> > I don't think this makes sense... The default chrome font should work just
> > fine.
> 
> It doesn't work because the document is html so the default font is serif.

No, it's a XUL document.

> > >+.splitview-filter.splitview-all-filtered:focus {
> > >+  background-color: #eaa;
> > >+}
> > 
> > This disables native theming, unfortunately...
> 
> We're going away from native theming for devtools anyways.

The rule I cited only disables it conditionally (for splitview-all-filtered, whatever this means exactly, and :focus).

> > >+.splitview-nav > li > .stylesheet-more {
> > >+  position: relative;
> > >+  right: 0;
> > >+}
> > 
> > Shouldn't this be on the left side for RTL?
> 
> I'm not sure either. I'd think not.

Why not?

Comment 112

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #111)
> (In reply to Cedric Vivier [cedricv] from comment #110)
> > > >+:root {
> > > >+  font-family: sans-serif;
> > > >+}
> > > 
> > > I don't think this makes sense... The default chrome font should work just
> > > fine.
> > It doesn't work because the document is html so the default font is serif.
>
> No, it's a XUL document.

No, the UI is defined in HTML... cf. xmlns="http://www.w3.org/1999/xhtml"

> > We're going away from native theming for devtools anyways.
> 
> The rule I cited only disables it conditionally (for splitview-all-filtered,
> whatever this means exactly, and :focus).

Ok. I'll make it look pretty with a light background in advance of bug 687702 then.


> > > Shouldn't this be on the left side for RTL?
> > I'm not sure either. I'd think not.
> 
> Why not?

Because the most important thing imho is to visually link the editor with the selected style sheet. So they should probably be adjacent regardless of RTL.
I'll ask Shorlander his opinion about RTL layout in bug 687702.

Comment 113

[Dão Gottwald [:dao]]

(In reply to Cedric Vivier [cedricv] from comment #112)
> > > > >+:root {
> > > > >+  font-family: sans-serif;
> > > > >+}
> > > > 
> > > > I don't think this makes sense... The default chrome font should work just
> > > > fine.
> > > It doesn't work because the document is html so the default font is serif.
> >
> > No, it's a XUL document.
> 
> No, the UI is defined in HTML... cf. xmlns="http://www.w3.org/1999/xhtml"

The root node is a XUL window and you're correctly loading global.css. Removing the cited rule does not make the font serif.

> > > > Shouldn't this be on the left side for RTL?
> > > I'm not sure either. I'd think not.
> > 
> > Why not?
> 
> Because the most important thing imho is to visually link the editor with
> the selected style sheet.

I'm not really sure what this means...

Comment 114

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #113)
> (In reply to Cedric Vivier [cedricv] from comment #112)
> > No, the UI is defined in HTML... cf. xmlns="http://www.w3.org/1999/xhtml"
> 
> The root node is a XUL window and you're correctly loading global.css.
> Removing the cited rule does not make the font serif.

It does. There must be something we overlook :
http://neonux.com/StyleEditor/before.jpg
http://neonux.com/StyleEditor/after.jpg


> > Because the most important thing imho is to visually link the editor with
> > the selected style sheet.
> 
> I'm not really sure what this means...

Did you have a look at the UI?
The SplitView has a list of the document's style sheets on one side, and the editor for the currently selected style sheet on the other side.
Having the style sheet name next to the editor helps making the 'connection' rather than having informational data like the rule count next to it.

Comment 115

[Dão Gottwald [:dao]]

(In reply to Cedric Vivier [cedricv] from comment #114)
> (In reply to Dão Gottwald [:dao] from comment #113)
> > (In reply to Cedric Vivier [cedricv] from comment #112)
> > > No, the UI is defined in HTML... cf. xmlns="http://www.w3.org/1999/xhtml"
> > 
> > The root node is a XUL window and you're correctly loading global.css.
> > Removing the cited rule does not make the font serif.
> 
> It does. There must be something we overlook :
> http://neonux.com/StyleEditor/before.jpg
> http://neonux.com/StyleEditor/after.jpg

after.jpg doesn't use a serif font, it's just a different serifless font. Change your desktop settings if you don't like that particular font? :P

> > > Because the most important thing imho is to visually link the editor with
> > > the selected style sheet.
> > 
> > I'm not really sure what this means...
> 
> Did you have a look at the UI?

Yes.

> The SplitView has a list of the document's style sheets on one side, and the
> editor for the currently selected style sheet on the other side.
> Having the style sheet name next to the editor helps making the 'connection'
> rather than having informational data like the rule count next to it.

For RTL, isn't the stylesheet list is on the left and the editor on the right side?

Comment 116

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #115)
> after.jpg doesn't use a serif font, it's just a different serifless font.
> Change your desktop settings if you don't like that particular font? :P

Yes and no ;)
This isn't the font that matches any other UI element in Firefox whereas forcing 'font-family: sans-serif' does make it look consistent with the rest of the browser UI.
Any idea how I could do this in another way?


> > The SplitView has a list of the document's style sheets on one side, and the
> > editor for the currently selected style sheet on the other side.
> > Having the style sheet name next to the editor helps making the 'connection'
> > rather than having informational data like the rule count next to it.
> 
> For RTL, isn't the stylesheet list is on the left and the editor on the
> right side?

I don't think it will. Actually one could argue the style sheet list might better be on the left-side in non-RTL ;)
It is on the right to reduce scrollbar annoyances and to make the switch from bottom-docked bar to sidebar on the right less disruptive visually (see bug 687702).

Comment 117

[Dão Gottwald [:dao]]

(In reply to Cedric Vivier [cedricv] from comment #116)
> (In reply to Dão Gottwald [:dao] from comment #115)
> > after.jpg doesn't use a serif font, it's just a different serifless font.
> > Change your desktop settings if you don't like that particular font? :P
> 
> Yes and no ;)
> This isn't the font that matches any other UI element in Firefox whereas
> forcing 'font-family: sans-serif' does make it look consistent with the rest
> of the browser UI.
> Any idea how I could do this in another way?

The font looks different because you reduced the size.

> > > The SplitView has a list of the document's style sheets on one side, and the
> > > editor for the currently selected style sheet on the other side.
> > > Having the style sheet name next to the editor helps making the 'connection'
> > > rather than having informational data like the rule count next to it.
> > 
> > For RTL, isn't the stylesheet list is on the left and the editor on the
> > right side?
> 
> I don't think it will.

Well, it should. Did you try it?

> Actually one could argue the style sheet list might
> better be on the left-side in non-RTL ;)

Sure, maybe; but only then it should be on the right side for RTL.

Comment 118

[Cedric Vivier [:cedricv]]

Attachment: Address Dao comments + RTL-friendly layout

Comment 119

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration [1/2]

Comment 120

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration (tests) [2/2]

Updated patches to address Dao's comments, including making the UI RTL-friendly, and (very) minor fixes/polish.

New try underway at https://tbpl.mozilla.org/?tree=Try&rev=df97f1c073e1

Comment 121

[Cedric Vivier [:cedricv]]

(In reply to Cedric Vivier [cedricv] from comment #120)
> New try underway at https://tbpl.mozilla.org/?tree=Try&rev=df97f1c073e1

Er. This try had an older [2/2] patch... new try with correct patch this time: 
https://tbpl.mozilla.org/?tree=Try&rev=8a60d86a594e

Comment 122

[Dão Gottwald [:dao]]

Comment on attachment 571299 [details] [diff] [review]
Style Editor integration [1/2]

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

Content stylesheets should precede theme stylesheets, so that the latter can override the former if needed. 

>+++ b/browser/themes/gnomestripe/browser/devtools/splitview.css

>+.splitview-root,
>+.splitview-controller,
>+.splitview-main,
>+.splitview-nav-container,
>+.splitview-nav,
>+.splitview-side-details,
>+.splitview-details {
>+  display: -moz-box;
>+  -moz-box-flex: 1;

This boilerplate code bugs me every time I see it. Can you make these xul:box nodes and set flex="1" on them?

>+  -moz-box-orient: vertical;

And move this to the content stylesheet?

>+.splitview-nav:-moz-locale-dir(ltr) > li,
>+.splitview-nav:-moz-locale-dir(ltr) > li hgroup {
>+  float: left;
>+}
>+.splitview-nav:-moz-locale-dir(rtl) > li,
>+.splitview-nav:-moz-locale-dir(rtl) > li hgroup {
>+  float: right;
>+}

:-moz-locale-dir should be at the end of the selector (here and elsewhere). This might speed up selector matching, I think.

Comment 123

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

Comment on attachment 571301 [details] [diff] [review]
Style Editor integration (tests) [2/2]

impressive stuff

Comment 124

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration [1/2]

Comment 125

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration (tests) [2/2]

Addressed latest review comments by Dao and other minor fixes, details of changes below :

0e40ecb ![2/2] Fix intermittent in _new test.
de7f33d ![1/2] - Listen to 'command' instead of 'input' on the search box. Fixes embedded Clear button.
ec74d4a ![2/2] - Force dispatch command event on input in sv_filter test since a search box timeout cannot be disabled.
90352bb ![1/2] - Prefer :-moz-locale-dir on the right-side of the selectors. r=dao
89819ed ![1/2] - Fix search-on-type with xul:textbox.
7dc9944 ![1/2] - Prefer xul:box when applicable, remove boilerplate CSS rule. r=dao
bf45f5c ![1/2] - Load content style sheets before skin stylesheets. r=dao
c5d9968 ![1/2] - Add DevTools-specific localization notes (see bug 689685)
aa89a4c Bug 583041 - Style Editor integration (tests) [2/2] - r=robcee,msucan,dao
1478b8b Bug 583041 - Style Editor integration [1/2]. r=robcee,msucan,dao

Comment 126

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

Comment on attachment 571902 [details] [diff] [review]
Style Editor integration (tests) [2/2]

I'm ok with these tests provided they pass through try successfully. No need to re-ask for review unless you change them radically.

Comment 127

[Dão Gottwald [:dao]]

You forgot to rename styleeditorchrome.xul (comment 109). Please fix before landing.

Comment 128

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

thanks, Dao, I'll double-check that before we land.

Cedric, does this need another try run before landing?

Comment 129

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #127)
> You forgot to rename styleeditorchrome.xul (comment 109). Please fix before
> landing.

Thanks Dao for the r+.
I did not. I renamed it already from PascalCase. I kept it named styleeditorchrome.xul and not styleeditor.xul, because it is named as it should be imho.
StyleEditor.jsm has no UI (can be embedded anywhere - eg. Tilt), StyleEditorChrome.jsm does; naming the XUL UI file with the name of the module that does not implement UI would be confusing.

Comment 130

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (robcee) from comment #128)
> Cedric, does this need another try run before landing?

Latest is: https://tbpl.mozilla.org/?tree=Try&rev=d9ed14e743ad
Of all the repeated tests two thirds of the oranges are unrelated to Style Editor. Unfortunately I have one regression _intermittent_ on SE's _new test.
I'm looking at it. Can we still land it and fix that intermittent in a follow up?

Comment 131

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

(In reply to Cedric Vivier [cedricv] from comment #130)
> (In reply to Rob Campbell [:rc] (robcee) from comment #128)
> > Cedric, does this need another try run before landing?
> 
> Latest is: https://tbpl.mozilla.org/?tree=Try&rev=d9ed14e743ad
> Of all the repeated tests two thirds of the oranges are unrelated to Style
> Editor. Unfortunately I have one regression _intermittent_ on SE's _new test.
> I'm looking at it. Can we still land it and fix that intermittent in a
> follow up?

we can't land with a failing test, we'll have to disable it and fix after the fact.

Please file the followup bug and post a patch with teh offending test disabled, referring to that bug.

Thanks! Let's land this!

Comment 132

[Dão Gottwald [:dao]]

Comment on attachment 571901 [details] [diff] [review]
Style Editor integration [1/2]

Every XUL file is a UI file. "chrome" in the file name doesn't make any sense. Please get rid of it.

Comment 133

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (robcee) from comment #131)
> Please file the followup bug and post a patch with teh offending test
> disabled, referring to that bug.

Ok. I'm running just one more try before that, this time with the SourceEditor patch attached earlier on this bug applied. The intermittent is due to a SourceEditor bug that Mihai and I likely fixed with this patch.

Comment 134

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #132)
> Every XUL file is a UI file. "chrome" in the file name doesn't make any
> sense. Please get rid of it.

I think you misunderstood me.
The point is not about the XUL file being UI... yes, every XUL file is somehow about UI ;)

The reason is that StyleEditorChrome module implements the "chrome" around the StyleEditor module (which is *just ONE textbox/editor* - embeddable in other tools).
This is a term similar to the chrome in the browser that implements what's around one/multiple web content.

Does it really make any more sense to name a xul file with the name of a module that is unrelated?
Or do you have a better suggestion to make about a name for StyleEditorChrome module?

Comment 135

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Cedric Vivier [cedricv] from comment #134)
> Or do you have a better suggestion to make about a name for
> StyleEditorChrome module?

I hate bikesheds, but if it helps get this landed: how about StyleEditorContainer?

Comment 136

[Dão Gottwald [:dao]]

(In reply to Cedric Vivier [cedricv] from comment #134)
> Does it really make any more sense to name a xul file with the name of a
> module that is unrelated?

I don't see this as a problem. (I'm assuming that StyleEditor.jsm isn't *entirely* unrelated -- otherwise that's the module that should be renamed.)

Comment 137

[Cedric Vivier [:cedricv]]

(In reply to Cedric Vivier [cedricv] from comment #133)
> (In reply to Rob Campbell [:rc] (robcee) from comment #131)
> > Please file the followup bug and post a patch with teh offending test
> > disabled, referring to that bug.
> 
> Ok. I'm running just one more try before that, this time with the
> SourceEditor patch attached earlier on this bug applied. The intermittent is
> due to a SourceEditor bug that Mihai and I likely fixed with this patch.

Good news!
It's all green with that patch: https://tbpl.mozilla.org/?tree=Try&rev=b4586a0f8f1e

Ready to land! Modulo finding a new name, renaming a xul file, rechecking everything is registered correctly...

Comment 138

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #136)
> (In reply to Cedric Vivier [cedricv] from comment #134)
> > Does it really make any more sense to name a xul file with the name of a
> > module that is unrelated?
> 
> I don't see this as a problem. (I'm assuming that StyleEditor.jsm isn't
> *entirely* unrelated -- 

The assumption is wrong ;)
It is *entirely* unrelated. StyleEditor.jsm only knows of one/any DOM element where it instantiates an Orion instance.


> otherwise that's the module that should be renamed.)

Ok. That would make more sense. StyleEditor.jsm can be more explicitly considered as being the standalone "textbox" it actually is (and plans to be embedded in parts of Style Inspector - which is another reason styleeditor.xul for the chrome would not really work).

StyleEditorTextbox? StyleEditorInput? StyleEditorEditor?

Comment 139

[Mihai Sucan [:msucan]]

(In reply to Cedric Vivier [cedricv] from comment #138)
> StyleEditorTextbox? StyleEditorInput? StyleEditorEditor?

StyleEditorWidget?

Comment 140

[Dão Gottwald [:dao]]

(In reply to Cedric Vivier [cedricv] from comment #138)
> (In reply to Dão Gottwald [:dao] from comment #136)
> > (In reply to Cedric Vivier [cedricv] from comment #134)
> > > Does it really make any more sense to name a xul file with the name of a
> > > module that is unrelated?
> > 
> > I don't see this as a problem. (I'm assuming that StyleEditor.jsm isn't
> > *entirely* unrelated -- 
> 
> The assumption is wrong ;)
> It is *entirely* unrelated. StyleEditor.jsm only knows of one/any DOM
> element where it instantiates an Orion instance.

It's still part of the style editor feature, which is all I really care about.

Anyway, StyleEditorTextbox and StyleEditorWidget sound alright to me.

Comment 141

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #140)
> (In reply to Cedric Vivier [cedricv] from comment #138)
> > (In reply to Dão Gottwald [:dao] from comment #136)
> > > (In reply to Cedric Vivier [cedricv] from comment #134)
> > > > Does it really make any more sense to name a xul file with the name of a
> > > > module that is unrelated?
> It's still part of the style editor feature, which is all I really care
> about.

Alright. Updated patch with minimum change - please open a new bug for further renaming (which actually involves quite a bunch more than just file names) if necessary :

!1/2 Rename styleeditorchrome.xul to styleeditor.xul. r=dao

Uploaded additional patch that fixes a regression on one scratchpad test when the SourceEditor fix is applied.

The regression fix patch should be attributed to Mihai iirc.

Comment 142

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration [1/2]

Comment 143

[Cedric Vivier [:cedricv]]

Attachment: Fix regression on one scratchpad test after latest SourceEditor patch

Comment 144

[Dão Gottwald [:dao]]

Comment on attachment 572388 [details] [diff] [review]
Fix regression on one scratchpad test after latest SourceEditor patch

oops, wrong patch.

By the way, do you need the load event listener? waitForFocus should wait for focus and load.

Comment 145

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #144)
> Comment on attachment 572388 [details] [diff] [review] [diff] [details] [review]
> Fix regression on one scratchpad test after latest SourceEditor patch
> By the way, do you need the load event listener? waitForFocus should wait
> for focus and load.

Good catch, it's probably unnecessary. I'll push a try without it.

Comment 146

[Cedric Vivier [:cedricv]]

Attachment: Fix regression in one scratchpad test after SourceEditor update

The patch without "load" listener is good to go. https://tbpl.mozilla.org/?tree=Try&rev=020cb373258f

Comment 147

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

Comment on attachment 572415 [details] [diff] [review]
Fix regression in one scratchpad test after SourceEditor update

looks good.

I... think we can land???

Comment 148

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (robcee) from comment #147)
> Comment on attachment 572415 [details] [diff] [review] [diff] [details] [review]
> Fix regression in one scratchpad test after SourceEditor update
> 
> looks good.
> 
> I... think we can land???

I... hope you're right ;)

We should probably land in order 1) Scratchpad test regression fix and 2) SourceEditor fix, before Style Editor patches so that testsuite stays green at any given commit.

Comment 149

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

https://hg.mozilla.org/integration/fx-team/rev/2df2fe2bcc5d

https://hg.mozilla.org/integration/fx-team/rev/43eb4cd0c275

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

Comment 150

[Johnathan Nightingale [:johnath]]

(\o/)

Comment 151

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

and,

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

Comment 152

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

backout in: https://hg.mozilla.org/integration/fx-team/rev/a6adeac760bb

We had to backout for a few reasons:

1) TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_new.js | Test timed out
2) Paste in orion is completely broken
3) text selection
4) what we didn't see in 10 minutes of testing.

I think we should backout before we merge to aurora (done) and reland after the merge is complete so we can fix these issues in nightly.

:(

Comment 153

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (robcee) from comment #152)
> We had to backout for a few reasons:

:(


> 1) TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/styleeditor/test/
> browser_styleeditor_new.js | Test timed out

Where can I check the logs about this orange? Tinderbox fx-team seems down now.
The latest I could find at https://tbpl.mozilla.org/?rev=77622fd9c783 is green.

Would be interesting to check if that happened only because the patches were not landed in a safe order (as in comment #148) ?

Comment 154

[Cedric Vivier [:cedricv]]

(In reply to Cedric Vivier [cedricv] from comment #153)
> The latest I could find at https://tbpl.mozilla.org/?rev=77622fd9c783 is
> green.

Fwiw, this other try, that includes all patches, is green as well:
https://tbpl.mozilla.org/?rev=410deae0fe13

Please let me know where I can investigate about that orange.

Comment 155

[Dave Camp (:dcamp)]

(In reply to Cedric Vivier [cedricv] from comment #153)
> The latest I could find at https://tbpl.mozilla.org/?rev=77622fd9c783 is
> green.

This merge includes the style editor landing and its backout (so it doesn't really include the style editor).

The missing patch was pushed in https://tbpl.mozilla.org/?tree=Fx-Team&rev=fc249ee8d921 and had one style editor timeout (WinXP Debug - the starring is in error, this build included the fix)

The next push was green, then https://tbpl.mozilla.org/?tree=Fx-Team&rev=f57c833bb23e failed with a style editor timeout (Linux64 opt).  There was one more green before the backout.

Mihai also saw a timeout on his own local machine, but I don't know if he kept around logs.

Comment 156

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration [1/2]

Updated with fix(es) for the rare _new intermittent, 80+ successful try test runs ( https://tbpl.mozilla.org/?tree=Try&rev=9acea551a4cf and https://tbpl.mozilla.org/?tree=Try&rev=91fcba1df362 ).
Also fixes bug 699117.

Details:
ef78e85 ![1/2] Fix truncated loading of gzip-compressed style sheets. b=699117
d83d087 ![1/2] Move skin out of browser/ sub-folder consequently to Bug 701212.
cecc096 ![2/2] Disable all animations when testing.
352e23b ![1/2] Set activeSummary right before triggering EditorAdded.
e131285 ![2/2] Fix _new test.
ebdb896 ![1/2] Queue onCreate handler after the new element has been appended to the document.

Comment 157

[Cedric Vivier [:cedricv]]

Attachment: Style Editor integration (tests) [2/2]

Despite the confusing "Fix _new test" commit message above (fixing of a recent useless change in it), the real fix for the intermittent is "Queue onCreate handler after the new element has been appended to the document."

Comment 158

[Mihai Sucan [:msucan]]

Attachment: Orion-related fixes

This brings Orion in Style Editor up to par with Orion in Scratchpad. This makes the Orion editor usable - unfortunately it was almost unusable on my machine. :(

Changes:

1. Fix for: mouse text selection stops working after you switch between several CSS files in the Style Editor.

Problem: Style Editor keeps one instance of Orion per stylesheet. Each instance is made visible when needed by changing one of the parent element.style.display to block (or none when the respective editor is hidden). This causes Orion "convulsions" leading to no longer being able to prevent the browser default selection from happening (when mouse interactivity occurs). See point 2 for details on the cause of "convulsions".

Solution: added an event.preventDefault() call in their mousedown event handler. This bug is now fixed upstream in the same manner - they now always call preventDefault().

See: https://bugs.eclipse.org/bugs/show_bug.cgi?id=363508


2. Fix for: Scrolling is broken after TextView hide/unhide.

Problem: thanks to bug 94623 when the Orion editor is hidden and shown back by the Style Editor the scrolled DIVs inside Orion's iframe loose their scroll position and other layout information.

Solution would be a fix for bug 94623.

Work around: added state tracking to the Style Editor for the scroll position. So now when the Orion editors are hidden/shown, the state is properly restored.

This could probably be implemented at a "lower level" - inside the Source Editor. I could move onShow and onHide into the Source Editor component. Rob, do you have any thoughts on this?

Anyhow, this is not an Orion bug (yay).


3. Fix for: Undo/Redo keyboard shortcuts do not work.

Solution: added the keyboard shortcuts.

Ideal solution: fix bug 684546. I will take this bug ASAP.

Here I aimed to keep the changes minimal.


4. Fix for: cut/copy/paste keyboard shortcuts fail badly.

Problem: the container element of the Orion editor is a DIV with a tabindex, which means the element is focusable (see inside the styleeditor.xul file). The Style Editor manages focus as follows: whenever focus needs to be given to the editor widget the styleEd.inputElement.focus() is called (where styleEd is an instance of the StyleEditor object, and inputElement is a reference to the focusable DIV). This focus() call happens in the Style Editor UI code. The StyleEditor code handles the inputElement onfocus event and passes the event "downstream" to Orion (by calling the SourceEditor.focus() method which is just a proxy for Orion's focus() call).

When cut/copy/paste (and other operations) happen inside Orion there's a lot of voodoo work involved with focus() and blur() calls. Orion changes which elements are focused inside its own iframe quite many times without the user ever noticing. This happens for a varying number of reasons, including, but not limited to, avoiding browser bugs.

For paste, for example, focus is given to a hidden PRE which has contentEditable=true (this is not the same as main the main contentEditable DIV that shows the source on screen). Paste happens inside that PRE, then we take the result from there and put it back into our main view and so on.

(ugly stuff, fear not)

The fact that the Style Editor proxies inputElement onfocus events back to Orion cause a great deal of state confusion inside Orion, breaking many things, but what we first noticed are the cut/copy/paste actions. When blur() is called inside Orion focus is given to the StyleEditor inputElement DIV, which in turn proxies the focus() back to Orion when it's not expected (ouch). Orion handles its own focus() very diligently...

Solution: allow Orion to work its "white magic" without the Style Editor causing unneeded focus() calls. So when Orion calls blur() nothing bad happens.

In the Style Editor I removed tabindex from the Orion container. The user can still tab to the iframe without problems (iframes are focusable by default, so having the DIV focusable was not needed). I removed the inputElement onfocus handler and made code directly call the SourceEditor.focus() method - so we no longer proxy calls through a different DOM element.

--- end of fixes ---

I am going to open a new bug to update Orion once again from upstream. We have many of our fixes already landed there, and some more. I will bring in async init of Orion as well.

I believe the Orion update is not a requirement for the Style Editor to land. Cedric only needs to fix the Scratchpad _unsaved.js test failure. His changes to the Style Editor _new.js should fix the intermittent failures as well.

Looking forward for your review Rob! Thank you!


(note on patch order: make sure this patch is the last one you apply.)

Comment 159

[Cedric Vivier [:cedricv]]

(In reply to Mihai Sucan [:msucan] from comment #158)
> Created attachment 573847 [details] [diff] [review] [diff] [details] [review]

Awesome, Mihai!
Two questions:
- do we really need onShow/onHide at all? ie. is the root issue fixable? (why does Orion care about this?)
- could we save scrollTop (in px) rather than the "top line" ? this would not break smooth scrolling and would perhaps avoid the addition of get/setTopIndex altogether (whose name sounds really confusing to me. From seeing the name alone I have no idea what it does: could be setScrollTopLine?)

Comment 160

[Mihai Sucan [:msucan]]

(In reply to Cedric Vivier [cedricv] from comment #159)
> (In reply to Mihai Sucan [:msucan] from comment #158)
> > Created attachment 573847 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> 
> Awesome, Mihai!
> Two questions:
> - do we really need onShow/onHide at all? ie. is the root issue fixable?
> (why does Orion care about this?)

The root cause is not fixable here and now - that is bug 94623. 

A workaround inside Orion for bug 94623 would be far uglier. So, yes, we need that onShow/onHide. Unless you want the StyleEditorChrome to track the StyleEditor._sourceEditor scroll location state info separately. I considered that would be uglier.


> - could we save scrollTop (in px) rather than the "top line" ? this would
> not break smooth scrolling and would perhaps avoid the addition of
> get/setTopIndex altogether (whose name sounds really confusing to me. From
> seeing the name alone I have no idea what it does: could be
> setScrollTopLine?)

I elected to track the line number, but we can track the top pixel. getTopPixel() and setTopPixel(). I'll let Rob decide this. It's trivial to switch.

Direct access to the element scrollTop is forbidden. That would be a very ugly hack and highly unreliable - Orion changes how the internal scrolling works.

get/setTopPixel() is almost identical to scrollTop access.

Comment 161

[Cedric Vivier [:cedricv]]

Attachment: Scratchpad test fixes after SourceEditor fix.

Scratchpad+StyleEditor tests all green. https://tbpl.mozilla.org/?tree=Try&rev=00e1280351e9

Comment 162

[Cedric Vivier [:cedricv]]

Attachment: Fixed one line to match intent.

The correct order to land is http://neonux.com/StyleEditor/patchqueue/ + Mihai's fixes.

Comment 163

[Mihai Sucan [:msucan]]

Comment on attachment 574901 [details] [diff] [review]
Scratchpad test fixes after SourceEditor fix.

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

While I didn't like the idea of an onReady event for the Scratchpad I just remembered that this is going to be needed. Orion initialization is going to be async - once we land bug 702331. The way we listen for the Scratchpad window to load is probably going to be a problem.

Some comments below...

::: browser/devtools/scratchpad/test/browser_scratchpad_bug_699130_edit_ui_updates.js
@@ -2,5 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> -"use strict";
> -

Please do not remove this.

("use strict"; is per file or per function, so even if this is now in head.js it won't apply to the test files.)

@@ +20,5 @@
>  }
>  
>  function runTests()
>  {
> +  gScratchpadWindow.removeEventListener("load", arguments.callee, false);

s/arguments.callee/runTests/

arguments.callee is deprecated.

Comment 164

[Mihai Sucan [:msucan]]

The latest patch added by Cedric is now a requirement for bug 702331. As expected, Scratchpad tests need to change a bit to take into consideration the async Orion init. Tests are already async - they wait for the Scratchpad window load event, but that doesn't coincide with the Orion load event (the Source Editor ready event).

Comment 165

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

running the devtools suite, I get the following scratchpad errors:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_660560_tab.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_669612_unsaved.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_669612_unsaved.js | Found a devtools:scratchpad after previous test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_669612_unsaved.js | Found a devtools:scratchpad after previous test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_669612_unsaved.js | Found a devtools:scratchpad after previous test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_669612_unsaved.js | Found a devtools:scratchpad after previous test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_669612_unsaved.js | Found a devtools:scratchpad after previous test timed out
make: *** [mochitest-browser-chrome] Error 1

presumably these are the issues Mihai mentions in comment #164.

Comment 166

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

now that we're modifying the source editor, these scratchpad tests will need to be updated. We can do that as part of bug 702331 or do it here, but we can't land without them being fixed.

Comment 167

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

Comment on attachment 573847 [details] [diff] [review]
Orion-related fixes

Looks good.

Comment 168

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

Comment on attachment 574901 [details] [diff] [review]
Scratchpad test fixes after SourceEditor fix.

ok, but I want to cross-check those test-failures with the tests in this bug.

Comment 169

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

so it looks like it was just two test files. I'll see if I can figure out why these are failing.

Comment 170

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

ok, so after a little more digging, these two tests appear to be using the new onReady notification mechanism implemented in the Scratchpad test fixes patch. This doesn't appear to be working.

Comment 171

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

Comment on attachment 574901 [details] [diff] [review]
Scratchpad test fixes after SourceEditor fix.

+    // insert this Scratchpad instance ashey first argument

bad comment. "insert this Scratchpad instance as the first arg..."

this looks like it should work, but it doesn't seem to be firing in these tests.

in browser_scratchpad_bug_660560_tab.js:

     gScratchpadWindow = Scratchpad.openScratchpad();
-    gScratchpadWindow.addEventListener("load", runTests, false);
+    gScratchpadWindow.addEventListener("load", function () {
+      gScratchpadWindow.Scratchpad.addObserver({

use named functions for your event listeners, like function loadListener1() { ...

remove them with removeEventListener("load", loadListener1, false) instead of arguments.callee.

+      gScratchpadWindow.Scratchpad.addObserver({
+        onReady: runTests
+      });

this doesn't appear to be getting triggered.

 function runTests()
 {
   gScratchpadWindow.removeEventListener("load", arguments.callee, false);

need to remove the observer here.

 
 function runTests2()
 {
   gScratchpadWindow.removeEventListener("load", arguments.callee, false);

and here.

Same goes for browser_scratchpad_bug_669612_unsaved.js.

Comment 172

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (robcee) from comment #170)
> ok, so after a little more digging, these two tests appear to be using the
> new onReady notification mechanism implemented in the Scratchpad test fixes
> patch. This doesn't appear to be working.

After some investigation trying to repro, it appears you get those failures because you didn't apply all the (4) patches, in particular the SourceEditor fix aka patch 0002 here and at http://neonux.com/StyleEditor/patchqueue

Comment 173

[Cedric Vivier [:cedricv]]

(In reply to Mihai Sucan [:msucan] from comment #163)
> > -"use strict";
> Please do not remove this.

> > +  gScratchpadWindow.removeEventListener("load", arguments.callee, false);
> s/arguments.callee/runTests/

The reason I removed strict is that it is inconsistently the only test that is in strict mode and indeed used arguments.callee incorrectly since it is forbidden in strict mode.

Do we want to consistently use strict for all tests?

Comment 174

[Mihai Sucan [:msucan]]

(In reply to Cedric Vivier [cedricv] from comment #173)
> (In reply to Mihai Sucan [:msucan] from comment #163)
> > > -"use strict";
> > Please do not remove this.
> 
> > > +  gScratchpadWindow.removeEventListener("load", arguments.callee, false);
> > s/arguments.callee/runTests/
> 
> The reason I removed strict is that it is inconsistently the only test that
> is in strict mode and indeed used arguments.callee incorrectly since it is
> forbidden in strict mode.

The test did not use arguments.callee. You added it in your patch (please check fx-team). The test wouldn't run with "use strict" if it would have arguments.callee - you would get a fatal error.


> Do we want to consistently use strict for all tests?

The inconsistency of that test being the only test with "use strict" does not matter in this context. We should aim to have all our code run with "use strict", piece by piece, in time and with patience.

Please do not change the tests to "use strict" now, here. We don't need to add more patch churn - we want to land these patches ASAP. I only ask for "use strict" to not be removed from that test.

Thank you!

Comment 175

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (robcee) from comment #166)
> now that we're modifying the source editor, these scratchpad tests will need
> to be updated. We can do that as part of bug 702331 or do it here, but we
> can't land without them being fixed.

I believe the Scratchpad test failures caused by the Style Editor and Source Editor changes here should be fixed here. I would like us to land the Style Editor before we land bug 702331 - the Style Editor is a really good test ground for the updated Orion.

Comment 176

[Cedric Vivier [:cedricv]]

Attachment: 0001 - Make Scratchpad tests pass after SourceEditor changes

Addressed review comments. Fixed missing removeEventListeners elsewhere.

test [fx-team-SE7]$ git grep addEventListener | wc -l
40
test [fx-team-SE7]$ git grep removeEventListener | wc -l
40

Le compte est bon!

Comment 177

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

Comment on attachment 575124 [details] [diff] [review]
0001 - Make Scratchpad tests pass after SourceEditor changes

all tests pass. Landing!

Comment 178

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

https://hg.mozilla.org/integration/fx-team/rev/00e06ace7946
https://hg.mozilla.org/integration/fx-team/rev/69cd4124a49a
https://hg.mozilla.org/integration/fx-team/rev/2513f713a23e
https://hg.mozilla.org/integration/fx-team/rev/4119bee2221d
https://hg.mozilla.org/integration/fx-team/rev/1cac8411fdc6

Comment 179

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/mozilla-central/rev/00e06ace7946
https://hg.mozilla.org/mozilla-central/rev/69cd4124a49a
https://hg.mozilla.org/mozilla-central/rev/2513f713a23e
https://hg.mozilla.org/mozilla-central/rev/4119bee2221d
https://hg.mozilla.org/mozilla-central/rev/1cac8411fdc6

Comment 180

[Francesco Lodolo [:flod]]

A couple of questions related to localization:

1) Would it be a problem if noStyleSheet-tip-start.label remains empty? We usually remove all courtesy forms in Italian

2) Would it be possible to add a tooltip to scoped.label? Since it's a technical term I'll keep it in English, but a tooltip saying something like "This stylesheet uses the scoped attribute on the <style> element" would be useful. Not sure if this can be done on this element though.

Comment 181

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

(In reply to flod (Francesco Lodolo) from comment #180)
> A couple of questions related to localization:
> 
> 1) Would it be a problem if noStyleSheet-tip-start.label remains empty? We
> usually remove all courtesy forms in Italian

I don't think that would be a problem.

> 2) Would it be possible to add a tooltip to scoped.label? Since it's a
> technical term I'll keep it in English, but a tooltip saying something like
> "This stylesheet uses the scoped attribute on the <style> element" would be
> useful. Not sure if this can be done on this element though.

I think so. Should probably file a separate bug for that.

thanks!

Comment 182

[Ed Morley [:emorley]]

browser/devtools/styleeditor/Makefile.in and browser/devtools/styleeditor/test/Makefile.in were created in the tree by this bug, but were not listed in browser/makefiles.sh, so I've added them in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17e8e969fa09

Comment 183

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/Tools/Style_Editor

And mentioned on Firefox 11 for developers.