Bug 31248 – Tab width for javascript source is 8, should be 4

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

Currently the tab width for the javascript source display is 8 spaces. These days most people use 4 spaces in their editors, so perhaps it should be 4 in the source display.

[Timothy Hatcher]

I agree

[Patrick Mueller]

I briefly looked into this at one point.  

I didn't see that we supported any kind of tab expansion setting in WebKit's CSS.  

There were a couple of proposals to in CSS spec-land to set tab expansion values, but they all seemed kind of dead-ish when I remember looking at them.  

I suppose we could expand the tabs ourselves.  No fun, but not very hard either.

It would be nice to do something, as tabs == 8 spaces is so 1970's ...

I should be noted that FireBug uses 4 spaces for tabs.

Created attachment 48176 [details]
Patch to convert tabs to 4 spaces

It was an easy fix, patch is attached. Hope this makes it into the source.

[Timothy Hatcher]

You should make a ChangeLog entry so we can land this. Looks good though!

http://webkit.org/coding/contributing.html

[Pavel Feldman]

Comment on attachment 48176 [details]
Patch to convert tabs to 4 spaces

Text editor does not support tabs yet. I'd like to keep it (and its model) operating real characters with no tab substitutions for now.
There are good reasons for your change though, so if you are to make this replace, please do it in the SourceFrame.js (setContent method).

Please also follow the patch submit guidelines (your change is missing ChangeLog entry).

Created attachment 48187 [details]
Proposed patch

Created attachment 48188 [details]
Proposed patch

Okay, here is a complete patch with ChangeLog, and the mod was moved to SourceFrame#setContent

[WebKit Review Bot]

Attachment 48188 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick Mueller]

The patch is doing a blind replacement of \t with 4 space characters; but that's not how tabs actually work; they add a variable # of spaces to get the following characters to align on the tab-width boundaries.

For example, if the original text looks is this, where - is the tab character

----------------
-a-b-c
 -d -e -f
----------------

then the text will be displayed like this:

----------------
    a   b   c
    d   e   f
----------------

The following original text, again, with - as the tab character, yields the same display as above; note there is a space before each tab character:

----------------
-a-b-c
 -d -e -f
----------------

However, if we use the "\t == 4 spaces" replacement, the expanded text becomes this:

----------------
    a    b    c
     d     e     f
----------------

In practice, if people consistently use tabs just at the beginnings of lines, and never accidently inject spaces, then the expansion will work fine.  In my practice, when I used to use tabs in source files, I found that I frequently accidently mixed spaces with tabs; everything LOOKED ok in the editor.  But using the "\t == 4" spaces replacement would have ended up with the unaligned output as above.

[Pavel Feldman]

Created attachment 48208 [details]
[PATCH] Proposed change.

[Pavel Feldman]

(In reply to comment #10)
> Okay, here is a complete patch with ChangeLog, and the mod was moved to
> SourceFrame#setContent

I added a setting into the editor model and made a correct tab substitution based on the character offset instead.

@aparajita sorry for stealing your bug / patch. We've got lots of stuff to do though, so if you have spare cycles / are interested in doing something, jump into #webkit-inspector IRC channel and talk to us!

[WebKit Commit Bot]

Comment on attachment 48208 [details]
[PATCH] Proposed change.

Clearing flags on attachment: 48208

Committed r54415: <http://trac.webkit.org/changeset/54415>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

@pavel don't be sorry, I'm happy I pushed you guys to fix this. ;-)