Bug 33001 – Web Inspector: Migrate to canvas-based text viewer / editor that scales.

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

[Pavel Feldman]

I started experimenting with the canvas-based viewers / editors and ended up writing my own :) I took about two days to bring it to the following state:
http://repenaxa.com/editor/editor.html

- Simple (~800 lines of code so far)
- Feels snappy
- Scales
- It highlights in a fairly optimal way (by the regex modulus)
- Supports clipboard

Todo:
- Editor does not track view size (canvas is set its container's client width/height without being updated on resize). Hit refresh to fix :)
- Lacks lots of keyboard actions

I am now gathering your feedback. I think it is a way to go (at least for the viewers). But if you like an idea of having a simple and lightweight editor in inspector, I am all for pulling it all in. Source code available upon request.

[Timothy Hatcher]

Very impressive!!

Two passing nit-picks.

The insertion caret is 2px wide, should be 1px.

THe text selection should fill the whole line width when the whole line is selected and is in the middle of a selection spanning multiple lines. If that dosen't make sense just look at what WebKit does normally.

[Timothy Hatcher]

The caret should not be visible when there is a range selection.

[Joseph Pecoraro]

Yah, I thought this was awesome! I really like how snappy it is. Very cool stuff =)

I had a few whitespace battles:
  - tab key doesn't work
  - newline should auto-indent?

[Pavel Feldman]

(In reply to comment #1)
> Very impressive!!
> 
> Two passing nit-picks.
> 
> The insertion caret is 2px wide, should be 1px.
> 

Done.

> THe text selection should fill the whole line width when the whole line is
> selected and is in the middle of a selection spanning multiple lines. If that
> dosen't make sense just look at what WebKit does normally.

Done.

================
(In reply to comment #2)
> The caret should not be visible when there is a range selection.

I don't think so. In general, IDEs show it. Rationale: where does selection grow upon Shift + Right when there is a range selected?

================
(In reply to comment #3)
> Yah, I thought this was awesome! I really like how snappy it is. Very cool
> stuff =)
> 
> I had a few whitespace battles:
>   - tab key doesn't work
>   - newline should auto-indent?

Oh. I've just nailed down alt + cmd + shift + arrows combinations. Tabs and indent are more of language-specific...


Should I put up a change for review?

[Timothy Hatcher]

(In reply to comment #4)
> (In reply to comment #1)
> > The caret should not be visible when there is a range selection.
> 
> I don't think so. In general, IDEs show it. Rationale: where does selection
> grow upon Shift + Right when there is a range selected?

No editors/IDEs I have used show the caret.

Xcode - no.
TextMate - no.
SubEthaEdit - no.
TextEdit - no.
Eclipse - yes?

The selection extension problem has been lived with in many editors that don't show it.

We should strive to make it feel like WebKit's editing as much as possible. Or make it a setting?

> Should I put up a change for review?

Sure.

[Joseph Pecoraro]

Looking better and better. You seem to be blitzing through feature requests, so I'll be happy to add some more!

Feature Requests:

  -⌘↑ and ⌘↓ or Home / End to move to the top and bottom of a document.
  - Tab works now, but it should be more then 1 space!
  - Line Numbers? There is a little space on the edge.

Minor Bugs:

  - When selecting a line, with ⌘←, then pushing ← should leave the cursor where it is, not actually go left to the previous line. ⌥← does this correctly.
  - Select All ⌘A then push ← leaves a small "selection" on the bottom left.

Keep up the great work. I look forward to seeing a patch =).

[Pavel Feldman]

(In reply to comment #6)
> Looking better and better. You seem to be blitzing through feature requests, so
> I'll be happy to add some more!
> 
> Feature Requests:
> 
>   -⌘↑ and ⌘↓ or Home / End to move to the top and bottom of a document.

Done, as well as PageUp/Down, Ctrl Up/Down

>   - Tab works now, but it should be more then 1 space!

Present text model is very simple - one character in source line is one glyph on screen. Tab complicatens it, so leaving for latter.

>   - Line Numbers? There is a little space on the edge.
> 

Done.

> Minor Bugs:
> 
>   - When selecting a line, with ⌘←, then pushing ← should leave the cursor
> where it is, not actually go left to the previous line. ⌥← does this correctly.

Done.

>   - Select All ⌘A then push ← leaves a small "selection" on the bottom left.
> 

Could not repro this one. Could be obsolete.

> Keep up the great work. I look forward to seeing a patch =).

[Timothy Hatcher]

The longest line, line 80, is cut off by the scrollbar (hides some characters under the scrollbar).

[Pavel Feldman]

Created attachment 45708 [details]
[PATCH] Meet the editor

[Pavel Feldman]

Created attachment 45709 [details]
[File] Standalone editor runner for quick testing.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45708 [details] without any errors.

[Daniel Bates]

Very cool.

Two possible enhancements:

1) Allow double clicking on a word to select it (like in TextEdit).
2) Allow a closed selection to be extended. That is, suppose a selection has been made that starts at point (x_0, y_0). Then hold down the shift key and click on some other place (x_1, y_1) in the document to extend the existing selection up to the point (x_1, y_1). Notice, the closer the point (x_1, y_1) to the point (x_0, y_0), the smaller the existing selection becomes. Also, we could make this work for "dragged" selections. That is, make an existing selection then, holding down the shift key, click and drag to extend/reduce the existing selection.

[Timothy Hatcher]

Another nice feature that I enjoy in Xcode is the flashing of the matching brace/parenthesis/bracket when you arrow past the other one. Also double clicking them select the contents between the two.

[Pavel Feldman]

(In reply to comment #12)
> Very cool.
> 
> Two possible enhancements:
> 
> 1) Allow double clicking on a word to select it (like in TextEdit).

This was working, but regressed while I was renaming stuff for the patch. Fixed now.
 
> 2) Allow a closed selection to be extended. That is, suppose a selection has
> been made that starts at point (x_0, y_0). Then hold down the shift key and
> click on some other place (x_1, y_1) in the document to extend the existing
> selection up to the point (x_1, y_1). Notice, the closer the point (x_1, y_1)
> to the point (x_0, y_0), the smaller the existing selection becomes. Also, we
> could make this work for "dragged" selections. That is, make an existing
> selection then, holding down the shift key, click and drag to extend/reduce the
> existing selection.

Fixed. This was a one line change in fact:

was:
this._setCaretLocation(location.line, location.column);

now:
if (e.shiftKey)
        this._setSelectionEnd(location.line, location.column);
else
        this._setCaretLocation(location.line, location.column);

Check out http://repenaxa.com/editor/editor.html

[Pavel Feldman]

(In reply to comment #13)
> Another nice feature that I enjoy in Xcode is the flashing of the matching
> brace/parenthesis/bracket when you arrow past the other one. Also double
> clicking them select the contents between the two.

We'll get there. I was trying to improve highlighter speed via switching from regex to a lexer. That would also solve a whole class of problems such as matching brace, indent in code block, etc.

But it seems like all lexers are hand-written for java/ecmascript. I found couple of parser generators with JavaScript target, but no good grammars for them.  I'll spend some more time looking at ANTLRv3's ECMAScript one anyways.

Bespin is using simple hand-written lexer that makes it much faster than us. But it is really poor - no support for multiline tokens such as strings / regexes, etc.

[Timothy Hatcher]

I have been thinking about this more.

I love how the viewing is super fast and performs well compared to the DOM. Though there are things we miss when using canvas that I think are big.

When viewing:
* All the native text related context menu options. Some can be emulated. Others can't, like text-to-speach, spotlight, look up in dictionary and Snow Leopard's text services.

When editing:
* All the "for free" platform native text behaviors, substitutions and transformations in Snow Leopard. Some we could emulate, others we couldn't without excesive cost.
* Spelling and grammer checking also for free normally.

This makes me wonder if a hybrid editor would be feasable. One that uses a DOM for the portion that is being edited and uses the native WebKit editing support. While just viewing uses the canvas.

Also text selection while viewing could use the DOM to invoke native context menu items. Or add a way to invoke the native text context menu from JS with a canvas backed by text data.

[Timothy Hatcher]

Note: not all the native behaviors and context menu items are desired in an IDE editor, but I do find that I use some of them (like spell checking and dictionary) a lot when coding in Xcode.

[Pavel Feldman]

(In reply to comment #16)

I started with a kind of hybrid approach where I tried to have two layers: invisible text input that has focus and canvas that renders contents nicely. It did not work well for multiple reasons, primarily key handling. Highlighting text both with canvas and divs is double-work, so I'd like avoid it as I can.

There were two reasons I started this:
1) To make viewer scale
2) To allow limited in place editing that would not make developer sick.

I did not think that spell checker was such an important feature given the overall goal. This thing will never (not anywhere soon) be an editor for daily programming.

So I'd focus on a viewing perspective first. As you point out, this change introduces regressions:
- No web search
- No spotlight search
- No dictionary lookup
- No text to speech.

I think losing all but the last one is fine. Last one is tough due to its accessibility nature. Are you ready to lose it?

There are also few hacks we could use. The way custom popup is implemented, it can be tweaked to leave text view / text edit actions in place. Upon context menu toggle, we could create artificial input element with selected text / hover word. Then show original menu items.

[Eric Seidel (no email)]

Why is this bug marked security sensitive?  Does it need to be?

[Maciej Stachowiak]

Is there any approach where we can get good display performance without resorting to Canvas text drawing? I think the costs of losing native text functionality, especially accessibility, will be pretty high. BeSpin has gotten significant criticism for not really having an accessibility story, and I would not want to replicate their approach without having some way to deal with accessibility issues.

[Pavel Feldman]

(In reply to comment #20)
> I would not want to replicate their approach without having some way to deal with accessibility issues.

We have couple of options here:

1) Forget about canvas, do the span-based rendering of visible area. My fear is that it would result in ugly code and would not really address 'native editor experience' issue.
2) Toggle between native and canvas-based editors. That would make syntax highlighting and accessibility features mutually exclusive. Not an ideal accessibility story, but fairly pragmatic one.

[Keishi Hattori]

CJK characters have double the width in monospace font. We need to do something about it because the caret position goes out of sync.
http://en.wikipedia.org/wiki/Fullwidth_form

[Pavel Feldman]

Created attachment 46041 [details]
[PATCH] Same with support for CJK.

Strictly speaking, we are no longer monospace-bound. Ctrl+Alt+M toggles monospace mode.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46041 [details] without any errors.

[Pavel Feldman]

(In reply to comment #22)
> CJK characters have double the width in monospace font. We need to do something
> about it because the caret position goes out of sync.
> http://en.wikipedia.org/wiki/Fullwidth_form

Keishi, I uploaded latest version to http://repenaxa.com/editor/editor.html. Tell me if it behaves as expected!

[Timothy Hatcher]

Nice work on CJK,

This made me think of another issue. We will not have support for input managers, common and often required for Asian and other languages.

[Pavel Feldman]

(In reply to comment #26)
> Nice work on CJK,
> 
> This made me think of another issue. We will not have support for input
> managers, common and often required for Asian and other languages.

I was thinking about it, but at some point decided that "textInput" event that I am using for characters input should actually handle it behind the scenes. Did not check whether that was the case though. I think Keishi would know best.

[Timothy Hatcher]

(In reply to comment #27)
> (In reply to comment #26)
> > Nice work on CJK,
> > 
> > This made me think of another issue. We will not have support for input
> > managers, common and often required for Asian and other languages.
> 
> I was thinking about it, but at some point decided that "textInput" event that
> I am using for characters input should actually handle it behind the scenes.
> Did not check whether that was the case though. I think Keishi would know best.

I am not so sure. THe input manager needs to know where to show up on screen, etc.

[Timothy Hatcher]

Comment on attachment 46041 [details]
[PATCH] Same with support for CJK.

> +    setText: function(text)

Can this be a getter and setter? Why only a setter?


> +            console.log("Repaint %d:%d", firstLine, lastLine);

I think we have WebInspector.log which will log to the current inspector's console to make it easy to see.


> +        this._replaceSelectionWith(e.clipboardData.getData("Text"));

What happens when there is no Text on the pastboard? Like an image. Maybe it should bail and not replace the selection with nothing.


> +        this._isMac = navigator.userAgent.indexOf("Mac OS") !== -1;
> +        this._isWin = navigator.userAgent.indexOf("Windows") !== -1;
> +        this._isLinux = navigator.userAgent.indexOf("Linux") !== -1;

It is good the editor is standalone, but I would like to see this use WebInspector.platform. These classes already require the WebInspector object ro exist for namespace.


> +            else if (this._isLinux)
> +                this._font = "10px Droid Sans Mono";

Does this ship with all Linux platforms? I thought this was a Google font.


> +WebInspector.TextCursor = function(cursorElement)
> +{
> +    this._visible = false;
> +    this._cursorElement = cursorElement;
> +}

Why do you use a DOM element for this? Why paint it in the canvas?


> +WebInspector.TextEditorModel.prototype = {
> +
> +    linesCount: function()

Remove extra empty line. Use a getter?


> +    columnsCount: function()

Getter?

[Pavel Feldman]

(In reply to comment #29)
> (From update of attachment 46041 [details])
> > +    setText: function(text)
> 
> Can this be a getter and setter? Why only a setter?
> 

Is now setter. Will add getter once there are clients!

> 
> > +            console.log("Repaint %d:%d", firstLine, lastLine);
> 

Done.

> 
> 
> > +        this._replaceSelectionWith(e.clipboardData.getData("Text"));
> 
> What happens when there is no Text on the pastboard? Like an image. Maybe it
> should bail and not replace the selection with nothing.

Done.

> 
> 
> > +        this._isMac = navigator.userAgent.indexOf("Mac OS") !== -1;
> > +        this._isWin = navigator.userAgent.indexOf("Windows") !== -1;
> > +        this._isLinux = navigator.userAgent.indexOf("Linux") !== -1;
> 
> It is good the editor is standalone, but I would like to see this use
> WebInspector.platform. These classes already require the WebInspector object ro
> exist for namespace.
> 

Done.

> 
> > +            else if (this._isLinux)
> > +                this._font = "10px Droid Sans Mono";
> 
> Does this ship with all Linux platforms? I thought this was a Google font.
>

Android one. Removed.
 
> 
> > +WebInspector.TextCursor = function(cursorElement)
> > +{
> > +    this._visible = false;
> > +    this._cursorElement = cursorElement;
> > +}
> 
> Why do you use a DOM element for this? Why paint it in the canvas?
> 
>

In order not to repaint too much.
 
> > +WebInspector.TextEditorModel.prototype = {
> > +
> > +    linesCount: function()
> 
> Remove extra empty line. Use a getter?
> 

Done.

> 
> > +    columnsCount: function()
> 
> Getter?

Done.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	A	WebCore/inspector/front-end/JavaScriptHighlighterScheme.js
	M	WebCore/inspector/front-end/KeyboardShortcut.js
	A	WebCore/inspector/front-end/TextEditor.js
	A	WebCore/inspector/front-end/TextEditorHighlighter.js
	A	WebCore/inspector/front-end/TextEditorModel.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/inspector.html
Committed r52945