Confirmed - I'm seeing it on 38, but not 37
Actually, I'm seeing it in 37 too
There is an error right away when loading Style Editor panel (when it would usually be opening up an editor with the first source): TypeError: env.contentWindow is undefined editor.js:256:10 Then when you click on the sheet on the left it creates a new editor, so there are actually two editor iframes here. Relevant line: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/editor.js#256
The onLoad event seems to be firing at the wrong time (and there is not even a contentWindow defined on the iframe). It's not clear why this is different using original sources here, I'll have to look into it further.
[Tracking Requested - why for this release]: We can reproduce this issue on beta, dev edition and nightly. Brian: when you figure out a fix, we'll need to uplift as far as Beta, I can repro there as well but it seems a bit more intermittent.
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
tracking-firefox36: --- → ?
tracking-firefox37: --- → ?
tracking-firefox38: --- → ?
Also, to be clear, release channel is not effected.
Sounds like this is specific to Dev Edition in that case, and doesn't carry on to Beta/Release which means it's not a tracking issue (for release issues) but an uplift needed to mozilla-aurora branch can be considered when ready.
tracking-firefox36: ? → -
tracking-firefox37: ? → -
tracking-firefox38: ? → -
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #8) > Sounds like this is specific to Dev Edition in that case, and doesn't carry > on to Beta/Release which means it's not a tracking issue (for release > issues) but an uplift needed to mozilla-aurora branch can be considered when > ready. I don't think it is specific to Dev Edition - we are seeing it in both Nightly and Beta. I can confirm Jeff's testing in Comment 6 - here is what I am seeing locally: Nightly (38) - affected Dev Edition (37) - affected Beta (36) - affected Release (35) - unaffected
Flags: needinfo?(bgrinstead) → needinfo?(lsblakk)
To reproduce: Open http://canuckistani.github.io/bug-1128747/ Open devtools Style Editor panel Make sure "Show original sources" option is checked in the gear within the panel Click on the main.scss file on the left There is a big white space above the editor, as seen in attachment 8558197 [details]. There shouldn't be any extra space here.
Keywords: regressionwindow-wanted, testcase
Summary: Odd white space at the top of scss file → Style Editor: Extra white space appearing above the editor for a sourcemapped scss file
My mistake, reverting the flags to track in that case so we don't ship this regression. We need an assignee here to make sure we get a backout or low risk forward fix into Beta asap. Jeff - is there someone you can get on this?
status-firefox35: --- → unaffected
tracking-firefox36: - → +
tracking-firefox37: - → +
tracking-firefox38: - → +
Flags: needinfo?(lsblakk) → needinfo?(jgriffiths)
I will take it
Assignee: nobody → bgrinstead
Brian, when are you planning to land a fix? Today is gtb for beta 8. Thanks
Tracked down regression window for nightlies Good: 2014-11-13 Bad: 2014-11-14 https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ab137ddd3746&tochange=7f0d92595432 Trying to narrow it down further seems to give different results each time. I think the test case is only intermittently failing. I've ran mozregression a few of times and gotten the same nightly each time, so I'm fairly confident in that range.
I have confirmed locally that backing out revision fd4bd4b3fbf8 from bug 1094208 causes the problem to go away. Given that change  this looks like some kind of race condition that isn't covered by style editor tests. I noticed while debugging that it seems like two separate sources are added to the style editor list (main.scss and main.css). > when are you planning to land a fix? Today is gtb for beta 8. I'm not going to have a fix ready today, I'm still trying to understand exactly why this is failing and come up with a testcase locally. : https://hg.mozilla.org/mozilla-central/rev/fd4bd4b3fbf8
Brian, What about doing a backout of fd4bd4b3fbf8 ?
status-firefox36: affected → wontfix
(In reply to Sylvestre Ledru [:sylvestre] from comment #16) > Brian, What about doing a backout of fd4bd4b3fbf8 ? We shouldn't back out this change except as a last resort for Beta, because this would make Promise.jsm behave differently from DOM Promises, and other parts of the code may now depend on the correct scheduling in Promise.jsm, so we'd have the risk of new regressions and race conditions elsewhere. Going forward, we're going to remove Promise.jsm completely in favor of DOM promises, when the dependencies of bug 939636 are solved, so we shouldn't definitely back this change out in other channels.
I agree with Comment 17. This is a pretty specific problem contained only within the Style Editor and only certain sourcemapped files. I have a fix in place locally that resolves the problem, I just need to make sure that all other tests are passing.
OK. FYI, the build of beta 9 will start Thursday. Please make sure it lands tomorrow. Otherwise, we will either ship with this bug or disable this feature. Thanks!
Jeff, can you confirm that this fixes your issue using the following build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28eeed110b41? Joe, I'm still trying to figure out how to test this (it's been harder to test than I was hoping). But maybe you have some context on why this was done? (I found bug 583041 by looking through blame, but don't see it addressed specifically there). The change in the promise implementation seems to have upset the timing here when the style editor tries to set a sheet as selected during the onCreate callback.
Comment on attachment 8562389 [details] [diff] [review] splitview-sync-oncreate.patch Review of attachment 8562389 [details] [diff] [review]: ----------------------------------------------------------------- It looks from the original commit that this could have had something to do with animation . "Only call onCreate when the animation has started". I have vague recollections of the animations being cool, but a constant source of problems. So this could be some hackery around that. The animation code is no longer in appendItem, so this could be a safe thing to remove. : https://hg.mozilla.org/integration/fx-team/diff/2513f713a23e/browser/devtools/styleeditor/SplitView.jsm
Attachment #8562389 - Flags: review?(jwalker) → review+
Comment on attachment 8562389 [details] [diff] [review] splitview-sync-oncreate.patch Review of attachment 8562389 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Brian Grinstead [:bgrins] from comment #20) > Created attachment 8562389 [details] [diff] [review] > splitview-sync-oncreate.patch > > Jeff, can you confirm that this fixes your issue using the following build: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=28eeed110b41? I can't reproduce with this build - yay! I noticed some other minor weirdness and logged bug 1132100 - I noticed that the main.scss file is not always selected / visible in the editor.
Attachment #8562389 - Flags: feedback?(jgriffiths) → feedback+
Test case. Locally that this passes with the fix applied and fails without. Waiting for a couple of try pushes to confirm. New test with fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9035bd0ac31c New test without fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67fd0c29290b
Tests show up just as expected, so folded the test changes into this patch.
Hi Brian, this failed to apply: renamed 1128747 -> splitview-sync-oncreate.patch applying splitview-sync-oncreate.patch patching file browser/devtools/styleeditor/test/browser.ini Hunk #1 FAILED at 26 1 out of 2 hunks FAILED -- saving rejects to file browser/devtools/styleeditor/test/browser.ini.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh splitview-sync-oncreate.patch could you take a look, thanks!
Rebased and landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/d74068ffe2ed
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Brian, what are your plans wrt to this bug?
(for aurora & beta)
Going to request uplift for Aurora. I'd like to give this some time to bake rather than request it land in Beta and get merged to release in the next week.
Comment on attachment 8563233 [details] [diff] [review] splitview-sync-oncreate.patch Approval Request Comment [Feature/regressing bug #]: Bug 1094208 [User impact if declined]: The style editor code shows up in the wrong place (see attachment 8558197 [details]) [Describe test coverage new/current, TreeHerder]: There is a new test plus existing Style Editor tests are all passing [Risks and why]: The problem was a timing issue with the style editor templating and promise resolutions so there is a risk that the fix caused some other problems within the Style Editor. But nothing has surfaced so far, and all the tests are passing. The risk is limited since it only affects the Style Editor tab inside of Dev Tools. [String/UUID change made/needed]:
Attachment #8563233 - Flags: approval-mozilla-aurora?
Comment on attachment 8563233 [details] [diff] [review] splitview-sync-oncreate.patch This is a large patch but the actual fix is quite small. (The rest of the patch contains tests.) The risk here is limited in that this only affects the style editor, which is already in a bad state. Aurora+
Attachment #8563233 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Brian Grinstead [:bgrins] from comment #31) > Going to request uplift for Aurora. I'd like to give this some time to bake > rather than request it land in Beta and get merged to release in the next > week. Next week is going to be too late. Today, we build beta 10, next Thursday, 36 RC. So, it seems that we will ship with this bug.
status-firefox36: affected → wontfix
status-firefox37: affected → fixed
You need to log in before you can comment on or make changes to this bug.