1247083 - Style Editor: inline stylesheets are numbered incorrectly

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect, P3)

Description

[Joe Whitfield-Seed]

Attachment: stylesheet-count-test.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.103 Safari/537.36

Steps to reproduce:

1. Open the attached testcase
2. Open Style Editor

The testcase contains 4 stylesheets. The first 2 are linked, the others inline.


Actual results:

Inline stylesheets are listed as <inline style sheet #3> and <inline style sheet #4>


Expected results:

Inline stylesheets are listed as #1 and #2

The count includes <link> stylesheets. I think counting online inline stylesheets is clearer.

That is particularly true in a case like this where linked stylesheets fail to load - the style editor shows a list of 2 sheets that starts at #3.

Comment 1

[Tom Tromey :tromey]

I could go either way on the numbering issue.
This test case also has two other weird bits:

1. It shows a.css and b.css in the list of sources, but clicking either one
   just shows the .html file.  This presumably happens because the resources aren't
   available, but surely the behavior could be nicer, for example saying "I couldn't
   fetch this".

2. If I click in a source and scroll around, eventually I get an exception from the
   CSS completer.  I assume this is related to the bad behavior in #1, since we aren't
   really trying to parse css text.

Comment 2

[Fuqiao Xue [:xfq]]

Attachment: style-sheets-numbering.png

User-created style sheets also have this problem.

Comment 3

[Fuqiao Xue [:xfq]]

Should we fix the numbering in devtools/client/styleeditor/StyleSheetEditor.jsm[1] (the "view") or in the styleSheetIndex[2][3] getter?

Personally, I prefer to change the code of the styleSheetIndex getter, because all the "downstream" code (such as getStyleSheetIdentifier, inlineStyleSheet and newStyleSheet) will benefit from the fix. Otherwise we have to fix all the "downstream" code.

[1] https://dxr.mozilla.org/mozilla-central/rev/a835589ae0c63a2d91be150d80b5fc600e44b447/devtools/client/styleeditor/StyleSheetEditor.jsm#192-195
[2] https://dxr.mozilla.org/mozilla-central/rev/a835589ae0c63a2d91be150d80b5fc600e44b447/devtools/server/actors/styleeditor.js#62-73
[3] https://dxr.mozilla.org/mozilla-central/rev/a835589ae0c63a2d91be150d80b5fc600e44b447/devtools/server/actors/stylesheets.js#240-251

Comment 4

[Tom Tromey :tromey]

(In reply to Xue Fuqiao [:xfq] from comment #3)
> Should we fix the numbering in
> devtools/client/styleeditor/StyleSheetEditor.jsm[1] (the "view") or in the
> styleSheetIndex[2][3] getter?

Changing the index reported by the actor seems like it could have far-reaching
consequences.  Since the goal is just to change the title, I think changing the
view is the better approach.

Comment 5

[Bryan Kok [:transfusion]]

Attachment: Bug 1247083 - Numbering of inline stylesheets should count only inline stylesheets r=pbro

Comment 6

[Bryan Kok [:transfusion]]

Attachment: non_styling_css_text.png

While fixing this bug, I noticed that blocks of CSS in the middle of the HTML document (e.g. for demonstration purposes, like in a <code/> block) were being detected as inline styles too; as of right now the styleSheetIndex getter[1] uses the InspectorUtils.getAllStyleSheets(this.document, true); method to get a list of all the stylesheet actors, which do not include said blocks of CSS.

I decided to do it like how the getStyleSheets() method[2] does it (since that eventually makes its way to the UI), so the relative ordering should always be correct once the document fully loads with <inline style sheet #1> towards the top and <inline style sheet #n> towards the bottom.

I used .filter(styleSheet => !styleSheet.href); to get the inline stylesheets since that was how it's done in friendlyName() [3].

I'm more than happy to add a mochitest if this is indeed the correct approach.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/stylesheets.js#324
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/stylesheets.js#726
[3] https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleeditor/StyleSheetEditor.jsm#195

Comment 7

[Patrick Brosset <:pbro>]

Thank you Bryan for attempting to solve this bug.

I want to discuss about what Tom Tromey said in comment #4:

Changing the index reported by the actor seems like it could have
far-reaching consequences. Since the goal is just to change the title, I think changing
the view is the better approach.

In your patch you are changing the actor side of things. One consequence I can see here is related to backward compatibility: the toolbox can connect to various DevTools servers (when remote debugging devices mostly) which may be running various versions of the code. So while newest servers would have your changes on the actor side, connecting to an older one would result in an error as the new method you are adding would not exist.
This can be handled, and I'll make comments to it in the code. But can you give a thought about what it would mean to do it on the client-side only instead? Would there be a potentially simpler/safer way to assign an index to inline stylesheets in the UI only? We might be able to change the code in friendlyName to increment 2 sets of index instead of 1 (one for linked sheets, and one for inline sheets).

Comment 8

[Bryan Kok [:transfusion]]

Attachment: Bug 1247083 - Number inline and new stylesheets separately in Style Editor; r=pbro

Comment 9

[Bryan Kok [:transfusion]]

Attachment: nondeterministic_stylesheet_ordering.png

(In reply to Patrick Brosset <:pbro> from comment #7)

This can be handled, and I'll make comments to it in the code. But can you give a thought about what it would mean to do it on the client-side only instead? Would there be a potentially simpler/safer way to assign an index to inline stylesheets in the UI only? We might be able to change the code in friendlyName to increment 2 sets of index instead of 1 (one for linked sheets, and one for inline sheets).

Certainly! I've made a new diff of a client-only implementation above. I also handled user-added sheets added via the + button. I reasoned that editing friendlyName() in StyleSheetEditor.jsm alone isn't sufficient because that only renders a single row; we have to get a list of all the open editors, filter them by checking if their styleSheet property has href set, etc.

I found another issue - try keeping the style editor pane open, then navigating to redhat.com, or any other page with many inline stylesheets. You should notice something like the attached screenshot, all the inline style indexes are unique, but in the "wrong" order. Closing and reopening the DevTools results in everything being rendered in the correct order. The editors array in StyleEditorUI.jsm [1] which I used to derive the indexes is always in the correct order (hence why the mochitest I added should pass).

This can already be observed in central; the order of the stylesheets while the style editor pane is already open and navigating to a different page is different from opening the style editor pane only after the page has finished loading. You may also see multiple inline stylesheets with the same index; with redhat.com, I initially see a few instances of <inline style sheet #1>; since friendlyName() only depends on the styleSheetIndex of the styleSheet actor's (form) it must be the case that there are multiple sheets with the same styleSheetIndex (0) upon a hot load. The sources for each sheet are then fetched asynchronously here and some ordering imposed based on the styleSheetIndex (the ordinal variable, I think.) [2].

This issue is also present in the 1st server-side patch that I submitted due to several stylesheets occasionally having the same styleSheetIndex, thus causing the rendering order to be wrong.

Should I try to handle navigating while the style editor is open in this issue, too? My gut feeling would be to set ordinal based on the editors array instead of the styleSheetIndex, but I'm not sure if this will have bad side effects.

[1] https://searchfox.org/mozilla-central/source/devtools/client/styleeditor/StyleEditorUI.jsm#87
[2] https://searchfox.org/mozilla-central/source/devtools/client/styleeditor/StyleEditorUI.jsm#595

Comment 10

[Patrick Brosset <:pbro>]

The problem probably comes from the fact that, when you're navigating to/reloading a page, it takes time for the stylesheets to be downloaded and parsed by the engine and therefore appear in the style editor. The logic that displays stylesheets in the sidebar is asynchronous and it may very well be that it adds each stylesheet as it becomes available. So, stylesheet 23 could become available before, say, stylesheet 3. Hence the order not being respected.
If you close devtools and re-open again after the page has finished loading, then the order seems fine because all stylesheets are ready to be displayed in the sidebar right from the start.

That's just my feeling about the issue. I'm looking at the code now and will come back with comments inside the patch when I have something to share.

Comment 11

[Patrick Brosset <:pbro>]

I did more research on this. Here are my findings: the order of stylesheets in the sidebar is controlled by the MozBoxOrdinalGroup CSS style property. This property (which pertains to the XUL flexbox layout system) acts like order in traditional CSS flexbox.
This was introduced at least 7 years ago in the Style Editor, so it's really always been here.

It currently gets set to the StyleSheetActor.styleSheetIndex value. This value is set on the server side and is meant to indicate the position of a given stylesheet within the document.

So there seems to be a disconnect already in the code today between the way we assign friendly names to stylesheets and this document-order index.
We might want to address it too, but that goes beyond the scope of this bug.

I think if you limited yourself to reworking the indexes of stylesheets that appear in the friendly name as you've done so far, that would probably be fine.

In a follow-up bug, we could rework the order in the sidebar. I think it might be interesting to experiment with a different approach, maybe one where inline stylesheets are grouped together, linked stylesheets together, etc.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e74ecc34bd94
Number inline and new stylesheets separately in Style Editor; r=pbro

Comment 13

[Oana Pop-Rus]

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