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

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Tim Nguyen :ntim]

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.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Does the CSS file use source maps?  Do you have an example page to testing?

Comment 2

[Tim Nguyen :ntim]

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.

Comment 3

[Tim Nguyen :ntim]

(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.

Comment 4

[Tim Nguyen :ntim]

Bug 1147765 must have fixed this.

Comment 5

[Tim Nguyen :ntim]

Filed bug 1151381 for the new issue.

Comment 6

[Tim Nguyen :ntim]

This is reproducible again with bug 1151381 fixed

Comment 7

[Tim Nguyen :ntim]

Sami, you've been doing a lot of Style Editor work recently.

Can you take a look ?

Comment 8

[Sami Jaktholm]

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

Comment 9

[Sami Jaktholm]

For the record, this seems to be busted on release too.

Comment 10

[Sami Jaktholm]

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

Comment 11

[Sami Jaktholm]

Attachment: bug-1148086-styleeditor-jump-on-select.patch

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

Comment 12

[Brian Grinstead [:bgrins]]

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

Comment 13

[Sami Jaktholm]

Attachment: bug-1148086-styleeditor-jump-on-select.patch

Thanks. Here's a patch with inlined test page.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65ee77d7f79b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f547e4381234

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f547e4381234