Closed Bug 583041 Opened 14 years ago Closed 13 years ago

Style Editor integration

Categories

(DevTools :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 11

People

(Reporter: rcampbell, Assigned: cedricv)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [strings][minotaur])

Attachments

(6 files, 41 obsolete files)

1.52 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
62.39 KB, patch
Details | Diff | Splinter Review
150.21 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
16.81 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
46.32 KB, patch
Details | Diff | Splinter Review
30.61 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
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.
Blocks: 584371
Depends on: 575234
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Whiteboard: [kd4b4]
Whiteboard: [kd4b4] → [kd4b5]
Attached image Screenshot. (obsolete) —
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.
Assignee: rcampbell → pwalton
Depends on: 582596
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?
(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.
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.
blocking2.0: --- → ?
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.
Attached patch Proposed patch, part 1. (obsolete) — Splinter Review
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 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.
Attachment #468562 - Flags: feedback+
ps, I am sad there is no frankenxul widget in the final patch.
Blocking beta5 - this is an essential function of the inspector feature.
blocking2.0: ? → beta5+
(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.
Depends on: 590307
Attached patch Proposed patch, version 2. (obsolete) — Splinter Review
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.
Attachment #468562 - Attachment is obsolete: true
Attachment #468892 - Flags: feedback?(rcampbell)
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.
Attached patch Proposed patch, version 3. (obsolete) — Splinter Review
Version 3 removes a debugging dump() that was mistakenly left in.
Attachment #468892 - Attachment is obsolete: true
Attachment #468899 - Flags: feedback?(rcampbell)
Attachment #468892 - Flags: feedback?(rcampbell)
Attached patch Proposed patch, version 4. (obsolete) — Splinter Review
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.
Attachment #468899 - Attachment is obsolete: true
Attachment #469250 - Flags: feedback?(rcampbell)
Attachment #468899 - Flags: feedback?(rcampbell)
Attached patch Proposed patch, version 5. (obsolete) — Splinter Review
Version 5 rebases the patch to the current dependencies as of this writing.
Attachment #469250 - Attachment is obsolete: true
Attachment #469289 - Flags: feedback?(rcampbell)
Attachment #469250 - Flags: feedback?(rcampbell)
Depends on: 590795
Depends on: 590796
Depends on: 590797
Depends on: 590799
Followup bugs added for known issues: 590307 590795 590796 590797 590799.
reducing the "depends" list as this bug does not depend on the followups.
No longer depends on: 590307, 590795, 590796, 590797, 590799
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?
Attachment #469289 - Flags: feedback?(rcampbell) → feedback+
Whiteboard: [kd4b5] → [kd4b5] [patchbitrot]
Attached patch Proposed patch, version 5.1. (obsolete) — Splinter Review
Patch rebased and revised per feedback. Review requested.
Attachment #469564 - Flags: review?(gavin.sharp)
Whiteboard: [kd4b5] [patchbitrot] → [kd4b5] [patchclean:0826]
Whiteboard: [kd4b5] [patchclean:0826] → [kd4b5] [patchbitrot]
Attached patch Proposed patch, version 5.2. (obsolete) — Splinter Review
Patch rebased to trunk and latest dependent patches.
Attachment #469564 - Attachment is obsolete: true
Attachment #469969 - Flags: review?(gavin.sharp)
Attachment #469564 - Flags: review?(gavin.sharp)
Whiteboard: [kd4b5] [patchbitrot] → [kd4b5] [patchclean:0827]
We're not going to be able to get this for beta 5, but should be able to for beta 6.
blocking2.0: beta5+ → beta6+
Whiteboard: [kd4b5] [patchclean:0827] → [kd4b6] [patchclean:0827]
Attachment #469969 - Flags: review?(gavin.sharp) → review?(dolske)
Whiteboard: [kd4b6] [patchclean:0827] → [kd4b6] [strings] [patchclean:0827]
Whiteboard: [kd4b6] [strings] [patchclean:0827] → [kd4b6] [strings] [patchbitrot]
Attached patch Proposed patch, version 5.3. (obsolete) — Splinter Review
Patch rebased to trunk and the latest dependent patches.
Attachment #469969 - Attachment is obsolete: true
Attachment #470580 - Flags: review?(dolske)
Attachment #469969 - Flags: review?(dolske)
Whiteboard: [kd4b6] [strings] [patchbitrot] → [kd4b6] [strings] [patchclean:0830]
Blocks: 592320
No longer depends on: 575234
Severity: normal → blocker
Inspector feature postponed. Removing blocking flags from Inspector bugs.
blocking2.0: beta6+ → ---
Removing items from kd4b6.
Whiteboard: [kd4b6] [strings] [patchclean:0830] → [strings] [patchclean:0830]
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
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.
Attachment #470580 - Flags: review?(dolske)
Assignee: pwalton → cedricv
Attached patch cssEditor (obsolete) — Splinter Review
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.
Attachment #469289 - Attachment is obsolete: true
Attachment #470580 - Attachment is obsolete: true
Whiteboard: [strings] [patchclean:0830] → [strings]
Whiteboard: [strings] → [strings][patch-needs-updating]
Summary: create a css editor for the inspector (milestone 0.6.1) → Style Editor feature
Will open separate bugs for Import, Save, Save All and Indentation patches.
Attachment #467640 - Attachment is obsolete: true
Attachment #542483 - Attachment is obsolete: true
Summary: Style Editor feature → Style Editor integration
Attached patch Patch (obsolete) — Splinter Review
Split tests in a separate patch.
Attachment #543945 - Attachment is obsolete: true
Attached patch Tests (obsolete) — Splinter Review
Attachment #543984 - Flags: review?(rcampbell)
Attachment #543983 - Flags: review?(rcampbell)
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!
Attachment #543983 - Flags: ui-review?(shorlander)
Attachment #543983 - Flags: review?(rcampbell)
Attachment #543983 - Flags: review-
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.
Attachment #543984 - Flags: review?(rcampbell) → review-
Comment on attachment 543983 [details] [diff] [review] Patch canceling UI review request as Cedric's doing some refreshment over in bug 671350.
Attachment #543983 - Flags: ui-review?(shorlander)
Whiteboard: [strings][patch-needs-updating] → [strings][patch-needs-updating][minotaur]
Whiteboard: [strings][patch-needs-updating][minotaur] → [strings][patch-needs-updating][minotaur][p1]
Priority: -- → P1
Whiteboard: [strings][patch-needs-updating][minotaur][p1] → [strings][patch-needs-updating][minotaur]
Cedric, can we get a new version of this patch landed before we go too far down the responsive UI road?
Makes sense. I'll rebase this patch tomorrow first thing in the morning.
New patch with improvements according to review.
Attachment #543983 - Attachment is obsolete: true
Attachment #549341 - Flags: review?
Attachment #549341 - Flags: review? → review?(rcampbell)
Tests v2.
Attachment #543984 - Attachment is obsolete: true
Attachment #549342 - Flags: review?(rcampbell)
Attached patch Diff against v1 (obsolete) — Splinter Review
Interdiff if needed.
Attached patch Diff v2 against v1 (obsolete) — Splinter Review
Much better interdiff (stat, ignore space, ...)
Attachment #549343 - Attachment is obsolete: true
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?
also, I'll continue reviewing this, with the ultimate destination in mind.
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
> 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)
Depends on: 671350
still waiting on updated patches in these bugs.
Priority: P1 → --
Target Milestone: --- → Firefox 8
Version: unspecified → Trunk
Attachment #549342 - Flags: review?(rcampbell)
Attachment #549341 - Flags: review?(rcampbell)
Priority: -- → P1
Attachment #549342 - Attachment is obsolete: true
Attachment #549341 - Attachment is obsolete: true
Attachment #549347 - Attachment is obsolete: true
Whiteboard: [strings][patch-needs-updating][minotaur] → [strings][minotaur]
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)
Attachment #557511 - Flags: review?(rcampbell)
Attachment #557512 - Flags: review?(rcampbell)
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 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.
(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?
(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 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!
Attachment #557511 - Flags: ui-review?(shorlander)
Attachment #557511 - Flags: review?(rcampbell)
Attachment #557511 - Flags: review?(dao)
Attachment #557511 - Flags: review+
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!
Attachment #557512 - Flags: review?(rcampbell) → review+
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.
(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 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.
Attachment #557511 - Flags: ui-review?(shorlander)
Attachment #557511 - Flags: review?(dao)
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 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;
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.
(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 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 <cedricv@neonux.com> (Original author) Please align Cedric with "n": # Contributor(s): # Cedric Vivier <cedricv@neonux.com> (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 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!
Blocks: 687698
Blocks: 687700
Blocks: 687702
Blocks: 687705
Blocks: 687707
Blocks: 687708
Attachment #557511 - Attachment is obsolete: true
Attachment #557512 - Attachment is obsolete: true
Attached patch Style Editor integration [1/2] (obsolete) — Splinter Review
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.
Attachment #561352 - Flags: review?(rcampbell)
Attachment #561354 - Flags: review?(rcampbell)
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
still have some oranges in there.
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.
Version: Trunk → 10 Branch
Attachment #561352 - Attachment is obsolete: true
Attachment #561352 - Flags: review?(rcampbell)
Attachment #561354 - Attachment is obsolete: true
Attachment #561354 - Flags: review?(rcampbell)
Attachment #564519 - Flags: review?(rcampbell)
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 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.
(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 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.
Attachment #564519 - Flags: review?(rcampbell) → review+
Comment on attachment 564521 [details] [diff] [review] Bug 583041 - Style Editor integration (tests) [2/2]. r=robcee,msucan all tests passed locally. Lookin' good.
Attachment #564521 - Flags: review?(rcampbell) → review+
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).
Attachment #564519 - Flags: review-
Attachment #564519 - Attachment is obsolete: true
Attachment #565186 - Flags: review?
Attached patch Address robcee comment on [2/2] (obsolete) — Splinter Review
Attachment #564521 - Attachment is obsolete: true
Attachment #565187 - Flags: review?(rcampbell)
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 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?
Attachment #564518 - Flags: review?(rcampbell) → review+
Comment on attachment 565186 [details] [diff] [review] Address robcee,dao comments on [1/2] R+ on these with Mihai's comments addressed. Thanks!
Attachment #565186 - Flags: review? → review+
Attachment #565187 - Flags: review?(rcampbell) → review+
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!
Attachment #565186 - Flags: review?(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
Attachment #565186 - Flags: review?(dao) → review-
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?
> > >+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).
(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.
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.
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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.
Attachment #565187 - Attachment is obsolete: true
Attachment #565186 - Attachment is obsolete: true
Attachment #564523 - Attachment is obsolete: true
Attached patch Style Editor integration [1/2] (obsolete) — Splinter Review
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
Attachment #569643 - Flags: review?(rcampbell)
Attachment #569644 - Flags: review?(rcampbell)
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.
Attachment #569643 - Flags: review?(rcampbell)
Attachment #569643 - Flags: review?(dao)
Attachment #569643 - Flags: review?(community)
Attachment #569643 - Flags: review+
Attachment #569644 - Flags: review?(rcampbell) → review+
Comment on attachment 569643 [details] [diff] [review] Style Editor integration [1/2] moving review to the proper "l10n".
Attachment #569643 - Flags: review?(community) → review?(l10n)
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?
Attachment #569643 - Flags: review?(l10n) → review-
Attachment #569643 - Attachment is obsolete: true
Attachment #569678 - Flags: review?(l10n)
Attachment #569678 - Flags: review?(dao)
Attachment #569643 - Flags: review?(dao)
(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 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.
Attached patch Style Editor integration [1/2] (obsolete) — Splinter Review
(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).
Attachment #569678 - Attachment is obsolete: true
Attachment #569729 - Flags: review?(l10n)
Attachment #569729 - Flags: review?(dao)
Attachment #569678 - Flags: review?(l10n)
Attachment #569678 - Flags: review?(dao)
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.
Attachment #569729 - Flags: review?(l10n) → review+
Attached patch Style Editor integration [1/2] (obsolete) — Splinter Review
(In reply to Axel Hecht [:Pike] from comment #105) Thanks! Updated.
Attachment #569729 - Attachment is obsolete: true
Attachment #569753 - Flags: review?(dao)
Attachment #569729 - Flags: review?(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?
Attachment #569753 - Flags: review?(dao) → review-
Target Milestone: Firefox 8 → ---
Version: 10 Branch → Trunk
(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.
(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?
(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.
(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...
(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.
(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?
(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).
(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.
Attachment #569753 - Attachment is obsolete: true
Attachment #571297 - Flags: review?(dao)
Attachment #571297 - Attachment is obsolete: true
Attachment #571297 - Flags: review?(dao)
Attached patch Style Editor integration [1/2] (obsolete) — Splinter Review
Attachment #571299 - Flags: review?(dao)
Attachment #571299 - Attachment description: Address Dao comments + RTL-friendly layout + misc minor polish → Style Editor integration [1/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
Attachment #569644 - Attachment is obsolete: true
Attachment #571301 - Flags: review?(rcampbell)
(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 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 on attachment 571301 [details] [diff] [review] Style Editor integration (tests) [2/2] impressive stuff
Attachment #571301 - Flags: review?(rcampbell) → review+
Attached patch Style Editor integration [1/2] (obsolete) — Splinter Review
Attachment #571299 - Attachment is obsolete: true
Attachment #571901 - Flags: review?
Attachment #571299 - Flags: review?(dao)
Attachment #571901 - Flags: review? → review?(dao)
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
Attachment #571301 - Attachment is obsolete: true
Attachment #571902 - Flags: review?
Attachment #571902 - Flags: review? → review?(rcampbell)
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.
Attachment #571902 - Flags: review?(rcampbell) → review+
Attachment #571901 - Flags: review?(dao) → review+
You forgot to rename styleeditorchrome.xul (comment 109). Please fix before landing.
thanks, Dao, I'll double-check that before we land. Cedric, does this need another try run before landing?
(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.
(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?
(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 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.
Attachment #571901 - Flags: review+ → review-
(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.
(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?
(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?
(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.)
(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...
(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?
(In reply to Cedric Vivier [cedricv] from comment #138) > StyleEditorTextbox? StyleEditorInput? StyleEditorEditor? StyleEditorWidget?
(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.
(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.
Attached patch Style Editor integration [1/2] (obsolete) — Splinter Review
Attachment #571901 - Attachment is obsolete: true
Attachment #572387 - Flags: review?(dao)
Attachment #572388 - Flags: review?(rcampbell) → review+
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.
Attachment #572388 - Flags: review+ → review?(rcampbell)
Attachment #572387 - Flags: review?(dao) → review+
(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.
The patch without "load" listener is good to go. https://tbpl.mozilla.org/?tree=Try&rev=020cb373258f
Attachment #572388 - Attachment is obsolete: true
Attachment #572415 - Flags: review?(rcampbell)
Attachment #572388 - Flags: review?(rcampbell)
Comment on attachment 572415 [details] [diff] [review] Fix regression in one scratchpad test after SourceEditor update looks good. I... think we can land???
Attachment #572415 - Flags: review?(rcampbell) → review+
(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.
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. :(
Whiteboard: [strings][minotaur][fixed-in-fx-team]! → [strings][minotaur][backed-out]
(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) ?
(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.
(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.
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.
Attachment #572387 - Attachment is obsolete: true
Attachment #573765 - Flags: review?(rcampbell)
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."
Attachment #571902 - Attachment is obsolete: true
Attachment #573766 - Flags: review+
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.)
Attachment #573847 - Flags: review?(rcampbell)
(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?)
(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.
Scratchpad+StyleEditor tests all green. https://tbpl.mozilla.org/?tree=Try&rev=00e1280351e9
Attachment #572415 - Attachment is obsolete: true
Attachment #574901 - Flags: review?
Attachment #574901 - Flags: review? → review?(rcampbell)
The correct order to land is http://neonux.com/StyleEditor/patchqueue/ + Mihai's fixes.
Attachment #573766 - Attachment is obsolete: true
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.
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).
Blocks: 702331
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.
Target Milestone: --- → Firefox 8
Version: Trunk → 10 Branch
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.
Attachment #573765 - Flags: review?(rcampbell) → review+
Comment on attachment 573847 [details] [diff] [review] Orion-related fixes Looks good.
Attachment #573847 - Flags: review?(rcampbell) → review+
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.
Attachment #574901 - Flags: review?(rcampbell) → review+
so it looks like it was just two test files. I'll see if I can figure out why these are failing.
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 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.
Attachment #574901 - Flags: review+ → review-
(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
(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?
Target Milestone: Firefox 8 → ---
Version: 10 Branch → Trunk
(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!
(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.
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!
Attachment #574901 - Attachment is obsolete: true
Attachment #575124 - Flags: review?(rcampbell)
Comment on attachment 575124 [details] [diff] [review] 0001 - Make Scratchpad tests pass after SourceEditor changes all tests pass. Landing!
Attachment #575124 - Flags: review?(rcampbell) → review+
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.
(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!
Depends on: 703874
No longer depends on: 703874
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
Documented: https://developer.mozilla.org/en/Tools/Style_Editor And mentioned on Firefox 11 for developers.
Depends on: 738974
No longer depends on: 738974
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: