Closed Bug 1148086 Opened 8 years ago Closed 8 years ago

Clicking the styleeditor link from the inspector doesn't jump to the right line if the file isn't loaded in the styleeditor


(DevTools :: Style Editor, defect)

Not set


(firefox40 fixed)

Firefox 40
Tracking Status
firefox40 --- fixed


(Reporter: ntim, Assigned: sjakthol)


(Whiteboard: [polish-backlog][difficulty=easy])


(1 file, 1 obsolete file)

- Open the inspector (without loading the styleeditor)
- Click a style editor link in the rules sidebar

AR :
- The styleeditor opens but doesn't jump to the right line

ER :
- The styleeditor should jump to the right line

Also, if the file is already loaded in the styleeditor, everything works smoothly

This has been annoying me for a few releases now, but I thought that was already filed.
Whiteboard: [devedition-40]
Does the CSS file use source maps?  Do you have an example page to testing?
Flags: needinfo?(ntim.bugs)
This seems to have been fixed very recently. I'm not sure which bug fixed it though.

Now that this issue is gone, I see a new bug, the first file in the style editor appears empty.
Flags: needinfo?(ntim.bugs)
(In reply to Tim Nguyen [:ntim] from comment #2)
> Now that this issue is gone, I see a new bug, the first file in the style
> editor appears empty.
I can reproduce this bug on all websites btw.
Bug 1147765 must have fixed this.
Closed: 8 years ago
Resolution: --- → WORKSFORME
Whiteboard: [devedition-40]
Filed bug 1151381 for the new issue.
This is reproducible again with bug 1151381 fixed
Resolution: WORKSFORME → ---
Sami, you've been doing a lot of Style Editor work recently.

Can you take a look ?
There's a small race in the selection code. Basically both StyleEditorUI [1] and StyleSheetEditor [2] try to set the top line and if StyleSheetEditor gets the final say (which happens if the editor is not already loaded), it'll scroll the source back to to top (topIndex is always 0).

[1] (via setCursor => alignLine)
For the record, this seems to be busted on release too.
So, StyleSheetEditor.onShow always resets the first visible line to be zero (_state.topIndex is never changed from the initial value of 0).

Looking at the commit history this piece of code seems to have been around forever [1]. Originally it also had a onHide handler which saved the current topIndex but that was removed by [2] when StyleEditor was made remotable. The idea here was to keep the scroll position even if another editor was selected.

This all happened before StyleEditor began using CodeMirror and lucky us CodeMirror can keep the scroll position during hide/show if CodeMirror.refresh is called when the editor is shown [3].

Here's a patch that fixes this problem and also makes the editors to remember their scroll positions if they are hidden and then shown again.

Assignee: nobody → sjakthol
Attachment #8597222 - Flags: review?(bgrinstead)
Whiteboard: [devedition-40][difficulty=easy]
Comment on attachment 8597222 [details] [diff] [review]

Review of attachment 8597222 [details] [diff] [review]:

Nice.  Here's a try push:  I have a minor nit I'd like to see addressed with the test file, but r=me with that fix.

::: browser/devtools/styleeditor/test/browser_styleeditor_scroll.js
@@ +7,5 @@
> +//  * selectStyleSheet (specified line)
> +//  * click on the sidebar item (line before the editor was unselected)
> +// See bug 1148086.
> +
> +const TESTCASE_URI = TEST_BASE_HTTP + "doc_long-stylesheets.html";

Nit: I'd like to see the HTML content added inline here since it's such a small file and it's easier to follow when looking at the test file.  Something like, for instance
Attachment #8597222 - Flags: review?(bgrinstead) → review+
Thanks. Here's a patch with inlined test page.

Attachment #8597222 - Attachment is obsolete: true
Attachment #8597577 - Flags: review+
Keywords: checkin-needed
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.