Style Editor: Extra white space appearing above the editor for a sourcemapped scss file

RESOLVED FIXED in Firefox 37

Status

()

Firefox
Developer Tools: Style Editor
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: canuckistani, Assigned: bgrins)

Tracking

(Blocks: 1 bug, {regression, testcase})

36 Branch
Firefox 38
x86
Mac OS X
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox35 unaffected, firefox36+ wontfix, firefox37+ fixed, firefox38+ fixed)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 8558197 [details]
gulp-sourcemaps-bug.png

WIll update with repro url in a sec
repro URL http://canuckistani.github.io/bug-1128747/
(Assignee)

Comment 2

3 years ago
Confirmed - I'm seeing it on 38, but not 37
(Assignee)

Comment 3

3 years ago
Actually, I'm seeing it in 37 too
(Assignee)

Comment 4

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

Updated

3 years ago
(Assignee)

Comment 5

3 years ago
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.
Flags: needinfo?(bgrinstead)
[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: ? → -
(Assignee)

Comment 9

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

Comment 10

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

Comment 12

3 years ago
I will take it
Assignee: nobody → bgrinstead
Flags: needinfo?(jgriffiths)
Brian, when are you planning to land a fix? Today is gtb for beta 8. Thanks
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 14

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

Comment 15

3 years ago
I have confirmed locally that backing out revision fd4bd4b3fbf8 from bug 1094208 causes the problem to go away.  Given that change [0] 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.

[0]: https://hg.mozilla.org/mozilla-central/rev/fd4bd4b3fbf8
Blocks: 1094208
Flags: needinfo?(bgrinstead)
Keywords: regressionwindow-wanted
Brian, What about doing a backout of fd4bd4b3fbf8 ?
status-firefox36: affected → wontfix
Flags: needinfo?(bgrinstead)
status-firefox36: wontfix → affected

Comment 17

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

Comment 18

3 years ago
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.
Flags: needinfo?(bgrinstead)
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!
(Assignee)

Comment 20

3 years ago
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?

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.
Attachment #8562389 - Flags: review?(jwalker)
Attachment #8562389 - Flags: feedback?(jgriffiths)
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 [1]. "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.

[1]: https://hg.mozilla.org/integration/fx-team/diff/2513f713a23e/browser/devtools/styleeditor/SplitView.jsm
Attachment #8562389 - Flags: review?(jwalker) → review+
(Assignee)

Updated

3 years ago
Blocks: 1132100
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+
(Assignee)

Comment 23

3 years ago
Created attachment 8562932 [details] [diff] [review]
styleeditor-load-error-testcase.patch

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
(Assignee)

Comment 24

3 years ago
Created attachment 8563233 [details] [diff] [review]
splitview-sync-oncreate.patch

Tests show up just as expected, so folded the test changes into this patch.
Attachment #8562389 - Attachment is obsolete: true
Attachment #8562932 - Attachment is obsolete: true
Attachment #8563233 - Flags: review+
(Assignee)

Comment 25

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3877af16026
Keywords: checkin-needed
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!
Flags: needinfo?(bgrinstead)
Keywords: checkin-needed
(Assignee)

Comment 27

3 years ago
Rebased and landed on fx-team:

https://hg.mozilla.org/integration/fx-team/rev/d74068ffe2ed
Status: NEW → ASSIGNED
Flags: needinfo?(bgrinstead)
Keywords: regression
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

3 years ago
Blocks: 1054765
https://hg.mozilla.org/mozilla-central/rev/d74068ffe2ed
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Brian, what are your plans wrt to this bug?
Flags: needinfo?(bgrinstead)
(for aurora & beta)
(Assignee)

Comment 31

3 years ago
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.
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 32

3 years ago
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
https://hg.mozilla.org/releases/mozilla-aurora/rev/8c0692b67d2d
status-firefox37: affected → fixed
You need to log in before you can comment on or make changes to this bug.