arni2033 Reporter
	Description • 8 years ago

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open url   data:text/html,<style>body{background:white;margin:0px}
2. Open devtools -> style editor

AR:  Style editor uses tab indentation
ER:  Style editor should use whitespaces as indentation

	Julian Descottes [:jdescottes]
	Comment 1 • 7 years ago

I don't see any option to set an indentation preference in the style editor?

	Tom Tromey :tromey
	Comment 2 • 7 years ago

(In reply to Julian Descottes [:jdescottes] from comment #1)
> I don't see any option to set an indentation preference in the style editor?

I think the complaint is that the options under "Editor Preferences" in the
"Toolbox Options" page should affect the style editor's editor component as well.
This seems reasonable to me.

The bug is that prettifyCSS unconditionally uses tabs:

https://dxr.mozilla.org/mozilla-central/rev/88030580b14bb253a55bc174c987a9fa43c3fb55/devtools/shared/inspector/css-logic.js#291,304

	Tom Tromey :tromey
	Comment 3 • 7 years ago

Maybe this file is relevant:
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/indentation.js#1

	Patrick Brosset <:pbro>
	Comment 4 • 7 years ago

Inspector bug triage (filter on CLIMBING SHOES).

	Ajay Assignee
	Comment 5 • 7 years ago

I was trying to fix the bug and I've got a doubt.
Near the checkbox 'Indent using spaces' there is an
option for setting the tab size. Effects of changing 
the tab size are successfully displayed in the stylesheet
editor even though |prettifyCSS| doesn't make use of
tab size. This would mean that other functions also change
the way text is displayed, but I wasn't able to find them.
What i did find was that, when a preference is changed then
|reloadPreferences| 
(https://dxr.mozilla.org/mozilla-central/source/devtools/client/sourceeditor/editor.js#528)
is called. But this only seems to change the values of keys such as 'tabSize'
but not actually edit text. I thought that by looking at how 
changes in tab sizes affect text, I might be able to fix this bug.
Am I going wrong somewhere?

Despite having the doubt as previously mentioned, I tried fixing
the issue by modifying |prettifyCSS| 
(https://dxr.mozilla.org/mozilla-central/source/devtools/shared/inspector/css-logic.js#141)
Partial output of 'hg diff' reflecting the changes I made are given below:
 

 const TAB_CHARS = "\t";
+const SPACE_CHARS = " ";
+const EXPAND_TAB = "devtools.editor.expandtab";
 

+    let useTabs = !Services.prefs.getBoolPref(EXPAND_TAB);
+
     if (isCloseBrace) {
       // Even if the stylesheet contains extra closing braces, the indent level should
       // remain > 0.
       indentLevel = Math.max(0, indentLevel - 1);
-
-      indent = TAB_CHARS.repeat(indentLevel);
+      if (useTabs) {
+        indent = TAB_CHARS.repeat(indentLevel);
+      } else {
+        indent = SPACE_CHARS.repeat(indentLevel);
+      }
       result = result + indent + "}";
     }



       result += "{";
-      indent = TAB_CHARS.repeat(++indentLevel);
+      if (useTabs) {
+        indent = TAB_CHARS.repeat(++indentLevel);
+      } else {
+        indent = SPACE_CHARS.repeat(++indentLevel);
+      }
     }


This partially solved the problem. For example
if  'Indent using spaces' is changed from 'unchecked' to
'checked' then indentation is performed using spaces instead of tabs,
but only after reloading the page. That is, effects of changing
status of 'Indent using spaces' are realised only after the page is
reloaded.

Other info:
1. Used the URL given by bug reporter for checking the effect of
   changes I made.
2. I built first using './mach build'. Then made the changes
   in the working copy. Then './mach build faster' followed
   by './mach run'

Thanks

	Ajay Assignee
	Comment 6 • 7 years ago

Could you give me suggestions on how I should proceed?

	Tom Tromey :tromey
	Comment 7 • 7 years ago

(In reply to Ajay from comment #6)
> Could you give me suggestions on how I should proceed?

Thanks for the needinfo.  I had seen this earlier but then forgot about it, a needinfo is really
the best way... :)

(In reply to Ajay from comment #5)
> I was trying to fix the bug and I've got a doubt.
> Near the checkbox 'Indent using spaces' there is an
> option for setting the tab size. Effects of changing 
> the tab size are successfully displayed in the stylesheet
> editor even though |prettifyCSS| doesn't make use of
> tab size. This would mean that other functions also change
> the way text is displayed, but I wasn't able to find them.

One thing that can edit the text is the rule rewriter.
It gets the indentation here:
https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/devtools/shared/css/parsing-utils.js#835
This is active when you use the rule view to edit a property value
coming from a style sheet.

> I thought that by looking at how 
> changes in tab sizes affect text, I might be able to fix this bug.
> Am I going wrong somewhere?

I think there are two things going on -- (1) whether indentation is done
with tabs or with spaces (currently hard-coded to tabs); and (2) how wide
a tab is when displayed.  I think (but I'm not totally sure) that the tab
width setting affects #2.  However, this bug is really about #1.

> This partially solved the problem. For example
> if  'Indent using spaces' is changed from 'unchecked' to
> 'checked' then indentation is performed using spaces instead of tabs,
> but only after reloading the page. That is, effects of changing
> status of 'Indent using spaces' are realised only after the page is
> reloaded.

That's what I would expect, because that's when the style sheet
is reformatted.  There is another bug (bug 724505) about how the style sheet
reformatting process is not done in the best way...

My only comment on the patch is whether it would be possible to use
devtools/shared/indentation.js to get the desired indentation string.

	Ajay Assignee
	Comment 8 • 7 years ago

(In reply to Tom Tromey :tromey from comment #7)
> That's what I would expect, because that's when the style sheet
> is reformatted.  There is another bug (bug 724505) about how the style sheet
> reformatting process is not done in the best way...
So, it is alright if the effects of changing the status of 'Indent using spaces' are realized only after the page is reloaded, correct?

> My only comment on the patch is whether it would be possible to use
> devtools/shared/indentation.js to get the desired indentation string.

By 'indentation string' you mean one of tab character, space character, right?
The rest of the comment is written assuming you meant that.

> whether it would be possible to use
> devtools/shared/indentation.js to get the desired indentation string.

It would be possible to use |getIndentationFromPrefs| 
(https://dxr.mozilla.org/mozilla-central/source/devtools/shared/indentation.js#28) 
and check the value of |indentWithTabs| 
(https://dxr.mozilla.org/mozilla-central/source/devtools/shared/indentation.js#36) 
if it is returned, to determine whether to use space or tab. However, |getIndentationFromPrefs| uses the same method 
(https://dxr.mozilla.org/mozilla-central/source/devtools/shared/indentation.js#34) 
to obtain the value of |indentWithTabs| that I used to determine whether to use tabs or spaces.
What do you suggest I do?

	Tom Tromey :tromey
	Comment 9 • 7 years ago

(In reply to Ajay from comment #8)

> So, it is alright if the effects of changing the status of 'Indent using
> spaces' are realized only after the page is reloaded, correct?

Yes, I think so.

> However,
> |getIndentationFromPrefs| uses the same method 
> (https://dxr.mozilla.org/mozilla-central/source/devtools/shared/indentation.
> js#34) 
> to obtain the value of |indentWithTabs| that I used to determine whether to
> use tabs or spaces.
> What do you suggest I do?

I looked at the code a bit more closely and I see it's a little funny, in that
getIndentationFromPrefs will return false if the "detect indentation" pref is set.
But, in this case, that's not the behavior we want, because when reformatting the
css we specifically don't want to try to detect the indentation.

I would suggest a new function in indentation.js that skips this first check and
just returns the value of the indentation prefs unconditionally.  getIndentationFromPrefs
could call this instead.  The reason for this approach is that it seems better to keep
all this checking in a single spot, so that any future changes are likely to affect
all tools equally.

	Ajay Assignee
	Comment 10 • 7 years ago

The attachment contains the output of 'hg diff'.

I've created a new function in 'devtools/shared/indentation.js' named |getTabPrefs| which is used by |prettifyCSS| in 'devtools/shared/inspector/css-logic.js' to determine whether to use spaces or tabs. |getTabPrefs| is also called by |getIndentationFromPrefs| (which is in 'devtools/shared/indentation.js').

I built firefox once and verified (using the URL given by the bug reporter) whether the issue is solved. Could you give your feedback. If everything looks good, i shall generate a patch.

	Tom Tromey :tromey
	Comment 11 • 7 years ago

This looks reasonable to me.
The only other important bit is having a test case.

	Ajay Assignee
	Comment 12 • 7 years ago

1) This is the attachment containing the proposed diffs for the file 'devtools/shared/tests/unit/test_prettifyCSS.js'. With these changes and changes as mentioned in the previous attachment 'diff_1' in place, I ran 'mach xpcshell-test devtools/shared/tests/unit' and there were no unexpected results.

2) Is a test for the new function |getTabPrefs| that I created in indentation.js required?

	Tom Tromey :tromey
	Comment 13 • 7 years ago

Thanks for the needinfo.

(In reply to Ajay from comment #12)
> Created attachment 8833329 [details]
> test_prettifyCSS.js - diff_1

The patch's description should be a commit message following the mozilla guidelines.
Normally they are of the form: "Bug NNN - brief description; r?reviewer".
You can put me for the reviewer.  Put both parts of the patch in a single attachment like this,
and be sure to mark the old attachments as obsolete.

The patch looks good to me.

> 1) This is the attachment containing the proposed diffs for the file
> 'devtools/shared/tests/unit/test_prettifyCSS.js'. With these changes and
> changes as mentioned in the previous attachment 'diff_1' in place, I ran
> 'mach xpcshell-test devtools/shared/tests/unit' and there were no unexpected
> results.

Super.

> 2) Is a test for the new function |getTabPrefs| that I created in
> indentation.js required?

Yes, but that's easily done by adding a line in test_indent_from_prefs, in
devtools/shared/tests/unit/test_indentation.js

	Ajay Assignee
	Comment 14 • 7 years ago

1) I noticed in 'test_indentation.js' that prefs are cleared (https://dxr.mozilla.org/mozilla-central/source/devtools/shared/tests/unit/test_indentation.js#130). I guess I should be doing that in 'test_prettifyCSS.js' too because I am setting some prefs for the tests?

2) Is it ok if I submit the patch through MozReview?

	Tom Tromey :tromey
	Comment 15 • 7 years ago

(In reply to Ajay from comment #14)
> 1) I noticed in 'test_indentation.js' that prefs are cleared
> (https://dxr.mozilla.org/mozilla-central/source/devtools/shared/tests/unit/
> test_indentation.js#130). I guess I should be doing that in
> 'test_prettifyCSS.js' too because I am setting some prefs for the tests?

Yeah, probably a good idea.

> 2) Is it ok if I submit the patch through MozReview?

Yes, definitely.

	Tom Tromey :tromey
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8834430 [details]
Bug 1327773 - Fix non-functioning devtools option 'Indent using spaces' by modifying prettifyCSS.js;

https://reviewboard.mozilla.org/r/110382/#review111966

Thank you for the patch.  This looks good to me.
Can you do a try run or should I do it?

	Ajay Assignee
	Comment 18 • 7 years ago

(In reply to Tom Tromey :tromey from comment #17)
> Comment on attachment 8834430 [details]
> Bug 1327773 - Fix non-functioning devtools option 'Indent using spaces' by
> modifying prettifyCSS.js;
> 
> https://reviewboard.mozilla.org/r/110382/#review111966
> 
> Thank you for the patch.  This looks good to me.
> Can you do a try run or should I do it?

I don't think I have try server access.

	Tom Tromey :tromey
	Comment 19 • 7 years ago

The try run has a failure that seems related to this patch.
See https://treeherder.mozilla.org/#/jobs?repo=try&revision=e09104efa946626e39c5c4e27b24ab013de57de4
In particular in the dt5 (dt9 in e10s) orange there is:

TEST-UNEXPECTED-FAIL | devtools/client/styleeditor/test/browser_styleeditor_pretty.js | minified source has been prettified automatically -

	Ajay Assignee
	Comment 21 • 7 years ago

The issue was that 'PRETTIFIED_SOURCE' 
(https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleeditor/test/browser_styleeditor_pretty.js#23)
used tabs for indentation, but the browser when opened, by default has 
'Indent using spaces' checked, hence indentation is performed using spaces.

After making the changes to the file 'browser_styleeditor_pretty.js', I ran
'mach mochitest devtools/client/styleeditor/test' and no tests failed.
(Before making the change there was 1 failed test).

	Tom Tromey :tromey
	Comment 22 • 7 years ago

I forgot to deal with this yesterday, sorry about that.
I sent the new one through try just now.
NI'ing myself so I remember to look at this tomorrow.

	Tom Tromey :tromey
	Comment 23 • 7 years ago

Looks good, I clicked "land".

	Pulsebot
	Comment 24 • 7 years ago

Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a6ab1af7f87
Fix non-functioning devtools option 'Indent using spaces' by modifying prettifyCSS.js; r=tromey

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 25 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9a6ab1af7f87

	Ajay Assignee
	Comment 26 • 7 years ago

Thank you for all the help!