Closed Bug 1148086 Opened 6 years ago Closed 6 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
STR: - 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.
Does the CSS file use source maps? Do you have an example page to testing?
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.
(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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
Filed bug 1151381 for the new issue.
This is reproducible again with bug 1151381 fixed
Status: RESOLVED → REOPENED
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  and StyleSheetEditor  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).  https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditorUI.jsm#684 (via setCursor => alignLine)  https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleSheetEditor.jsm#449
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 . Originally it also had a onHide handler which saved the current topIndex but that was removed by  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 .  https://hg.mozilla.org/integration/fx-team/rev/4119bee2221d#l5.140  https://hg.mozilla.org/integration/fx-team/rev/26caa9ab5c35  http://comments.gmane.org/gmane.comp.web.codemirror/2653
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. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3ec44cdebd0
Assignee: nobody → sjakthol
Status: REOPENED → ASSIGNED
Attachment #8597222 - Flags: review?(bgrinstead)
Comment on attachment 8597222 [details] [diff] [review] bug-1148086-styleeditor-jump-on-select.patch Review of attachment 8597222 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=177e967d16c1. 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 https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/test/browser_styleeditor_bug_870339.js#5, for instance
Attachment #8597222 - Flags: review?(bgrinstead) → review+
Thanks. Here's a patch with inlined test page. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65ee77d7f79b
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
You need to log in before you can comment on or make changes to this bug.