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

Categories

(DevTools :: Style Editor, defect)

defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: ntim, Assigned: sjakthol)

Details

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

Attachments

(1 file, 1 obsolete file)

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.
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
Whiteboard: [devedition-40]
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 [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] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditorUI.jsm#684 (via setCursor => alignLine)
[2] 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 [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].

[1] https://hg.mozilla.org/integration/fx-team/rev/4119bee2221d#l5.140
[2] https://hg.mozilla.org/integration/fx-team/rev/26caa9ab5c35
[3] 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)
Whiteboard: [devedition-40][difficulty=easy]
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
Attachment #8597222 - Attachment is obsolete: true
Attachment #8597577 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f547e4381234
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f547e4381234
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 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.