1710284 - Copying CSS Rules manually from the Devtools Inspector inserts extra indentation

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, enhancement, P3)

Description

[tero gusto]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:88.0) Gecko/20100101 Firefox/88.0

Steps to reproduce:

If you copy a CSS rule manually (select and copy) as opposed to right-clicking and selecting "Copy Rule", extra indentation is added.

Reading the issue Copying CSS Rules from the Devtools Inspector inserts empty lines between rules, I realized this way of marking a rule and copying it can be done with correct indentation by simply right-clicking the style, and selecting "Copy Rule". Nevertheless it would be great to get this way of copying a rule with the right indentation to work also.

To reproduce:

• Inspect
• Highlight a rule
• Copy
• Paste rule
• See that indentation is doubled (4 spaces):

#nav-menu-buffer {
    padding-top: 61px;
    -webkit-transition: all .3s ease-in;
    -moz-transition: all .3s ease-in;
}

Using "Copy Rule", with correct indentation:

• Inspect
• Right-click on a rule
• Select "Copy Rule"
• Paste
• See that indentation is correct (2 spaces):

#nav-menu-buffer {
  padding-top: 61px;
  -webkit-transition: all .3s ease-in;
  -moz-transition: all .3s ease-in;
}

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'DevTools::Inspector: Rules' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Julian Descottes [:jdescottes]]

We rely on Selection::toString here:
https://searchfox.org/mozilla-central/rev/d59bdea4956040e16113b05296c56867f761735b/devtools/client/inspector/rules/rules.js#635

        text = this.styleWindow.getSelection().toString();

        // Remove any double newlines.
        text = text.replace(/(\r?\n)\r?\n/g, "$1");

Selection::toString doesn't seem to be configurable: https://developer.mozilla.org/en-US/docs/Web/API/Selection/toString
We already remove double newlines, so we could also change the indentation with something like text.replace(/\ {4}/g, " ");.

Let's first check if there's anything else we could use to customize the indentation.

Comment 3

[Julian Descottes [:jdescottes]]

masayuki: I have a question about Selection toString()

When selecting a markup such as

  <div>
    <div>start {</div>
    <ul>
      <li>item1</li>
      <li>item2</li>
    </ul>
    <div>} end</div>
  </div>

and using window.getSelection().toString(), we get 4 spaces before item1 and item2.

start {

    item1
    item2

} end

Is there a way to have 2-space indentation instead?
Or should we modify the string retrieved in DevTools code to replace 4 spaces by 2 spaces?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, it's hard coded.
https://searchfox.org/mozilla-central/rev/5977b6fdebe32451ada35fa2cbd7c0752cfea982/dom/serializers/nsPlainTextSerializer.cpp#46,56

But it seems that there can be a chrome-only API to customize the indent level at computing the serialized text because the replacing is expensive. Mirko, any ideas?

Comment 5

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #4)

Yeah, it's hard coded.
https://searchfox.org/mozilla-central/rev/5977b6fdebe32451ada35fa2cbd7c0752cfea982/dom/serializers/nsPlainTextSerializer.cpp#46,56

But it seems that there can be a chrome-only API to customize the indent level at computing the serialized text because the replacing is expensive. Mirko, any ideas?

If it's only for the use-case I guess the inefficient proposal of replacing is acceptable. I guess in practice, it's a rare use-case and I don't expect the user to notice this.
:jdescottes: any idea if that presumption is correct?

I wouldn't add any chrome-only API, because it would complicate the code around nsPlainTextSerializer more. It is already complicated.

I also wouldn't change the default number of blanks from 4 to 2, because others might rely on the current state.

Comment 6

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

It would be helpful to track such post-processing workarounds, so that we can take them into account once nsPlainTextSerializer and nsIDocumentEncoder are refactored. But I don't see this happening any time soon.

Comment 7

[Julian Descottes [:jdescottes]]

(In reply to Mirko Brodesser (:mbrodesser) from comment #5)

If it's only for the use-case I guess the inefficient proposal of replacing is acceptable. I guess in practice, it's a rare use-case and I don't expect the user to notice this.
:jdescottes: any idea if that presumption is correct?

In our case, it only happens on demand when users manually copy a rule in the inspector. So I think the performance should not be an issue.
I mostly wanted to check if there was an existing API to change the indentation, to avoid adding unnecessary complexity. It's fine to do it in DevTools' code.

Thanks for taking a look!

Comment 8

[Julian Descottes [:jdescottes]]

Should be a good first bug, see comment 2. Contributors docs are at https://firefox-source-docs.mozilla.org/

Comment 9

[Clinton]

I'd love to work on this, could this be assigned to me, please?

Comment 10

[Julian Descottes [:jdescottes]]

(In reply to Clinton from comment #9)

I'd love to work on this, could this be assigned to me, please?

Sorry I missed your comment! Assigning the bug to you, thanks.

Comment 11

[Clinton]

Attachment: Bug 1710284 - Copying CSS Rules manually from the Devtools Inspector inserts extra indentation r=jdescottes

Depends on D129672

Comment 12

[Clinton]

Attachment: Bug 1710284 - Copying CSS Rules manually from the Devtools Inspector inserts extra indentation r=jdescottes Differential Revision: https://phabricator.services.mozilla.com/D129679

Depends on D129672

Comment 13

[Clinton]

Hi, I see you're away now but hopefully, when you come back you can look through this, I finally worked on this and while it was for the most part smooth, I still have some questions, I originally wanted to chain the replace commands together, i.e

        text = text.replace(/(\r?\n)\r?\n/g, "$1").replace(/\ {4}/g, " ");

But I felt it would make the Code less-readable and not fit into the General Mozilla coding style so I decided to go with `

 // Remove any double newlines.
        text = text.replace(/(\r?\n)\r?\n/g, "$1");
// Remove 4 space indentation.
       text = text.replace(/\ {4}/g, " ");

What do you think? Secondly, I think I made a mistake when submitting my patch which explains the "Depends on D129672" subscript, any reviews or suggestions would be welcome.

Comment 14

[Julian Descottes [:jdescottes]]

(In reply to Clinton from comment #13)

Hi, I see you're away now but hopefully, when you come back you can look through this, I finally worked on this and while it was for the most part smooth, I still have some questions, I originally wanted to chain the replace commands together, i.e

        text = text.replace(/(\r?\n)\r?\n/g, "$1").replace(/\ {4}/g, " ");

But I felt it would make the Code less-readable and not fit into the General Mozilla coding style so I decided to go with `

 // Remove any double newlines.
        text = text.replace(/(\r?\n)\r?\n/g, "$1");
// Remove 4 space indentation.
       text = text.replace(/\ {4}/g, " ");

What do you think? Secondly, I think I made a mistake when submitting my patch which explains the "Depends on D129672" subscript, any reviews or suggestions would be welcome.

Hi!

Splitting the replace on two separate lines sounds like a good idea.

Regarding your phabricator submissions, I guess you submitted them using moz-phab and that locally your patch for this bug was applied on top of your other patch for Bug 1590432. Honestly it's fine to keep it as is, we should still be able to review and land the patches.

I would fix it by rebasing the changeset for this bug on top of the current central revision, and then re-submitting it to phabricator (some mercurial commands which should help you here: rebase, wip, strip). But I am not using moz-phab so for further information either take a look at the docs or ask in the chat, someone else might be able to help :)

• https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html
• https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

You can abandon the revisions which are not going to be updated anymore from their Phabricator page (in the Add action... select box).

Comment 15

[Clinton]

You are absolutely right about the Phabricator submission issue, I finally figured that before I begin working on a new bug I should run hg pull and hg update central so that I get a clean slate. I would do as you said so as to fix the subscript issue, thanks for the tips.

Comment 16

[Clinton]

Attachment: Bug 1710284 - Copying CSS Rules manually from the Devtools Inspector inserts extra indentation r=jdescottes

Comment 17

[Clinton]

Attachment: Bug 1710284 - Copying CSS Rules manually from the Devtools Inspector inserts extra indentation r=jdescottes Differential Revision: https://phabricator.services.mozilla.com/D129680

Comment 18

[Clinton]

Attachment: Bug 1710284 Test for test r=jdescottes

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9ff65fb4f48
Copying CSS Rules manually from the Devtools Inspector inserts extra indentation r=jdescottes

Comment 20

[Marian-Vasile Laza]

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