Bug 151333 – Web Inspector: Breakpoint condition field should use CodeMirror

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* SUMMARY
Breakpoint condition field should use CodeMirror. Unlike eval/probe actions, the condition field should be a single-line editor. Tab and escape will need special handling, since they are currently used to dismiss the popover.

* NOTES
Using CodeMirror as a single-line editor:
http://stackoverflow.com/questions/13026285/codemirror-for-just-one-line-textfield

[Matt Baker]

(In reply to comment #0)
> * SUMMARY
> Breakpoint condition field should use CodeMirror. Unlike eval/probe actions,
> the condition field should be a single-line editor. Tab and escape will need
> special handling, since they are currently used to dismiss the popover.

Enter/escape require special handling. Tab should be disabled to allow tabbing to the next element in the tab order.

[Matt Baker]

Created attachment 265646 [details]
[Patch] Proposed Fix

[Radar WebKit Bug Importer]

<rdar://problem/23578315>

[Joseph Pecoraro]

Comment on attachment 265646 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=265646&action=review

r=me

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:-142
> -        conditionLabel.setAttribute("for", conditionInput.id);

Hmm, is there any way for this to still work? Maybe you can get the CodeMirror input field, give that an id, and keep this relationship.

> cm.getInputField() → Element
> Returns the input field for the editor. Will be a textarea or an editable div,
> depending on the value of the inputStyle option.

The end effect is when you click on the label "Condition" it should focus the input field for inputting the Condition.

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:154
> +        this._conditionCodeMirror.addKeyMap({
> +            "Esc": this._conditionCodeMirrorEscapeOrEnterKey.bind(this),
> +            "Enter": this._conditionCodeMirrorEscapeOrEnterKey.bind(this),
> +        });

Nit: You could bind once instead of twice by using a local.

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:168
> +        conditionEditorElement.id = "edit-breakpoint-popover-condition";

This does not need to be an `id` anymore. It can be a class name. Using `id` was just for the <label for="..id.."> relationship.

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:169
>          conditionLabel.textContent = WebInspector.UIString("Condition");

Style: This should probably go up by the creation of the label now instead of after everything.

> Source/WebInspectorUI/UserInterface/Views/BreakpointPopoverController.css:74
> +#edit-breakpoint-popover-condition > .CodeMirror .CodeMirror-placeholder {
> +    font-family: -apple-system, sans-serif;
> +    color: hsl(0, 0%, 65%);
>  }

Given that these are the same as CSSStyleDeclarationTextEditor.css for its placeholder, maybe we should put these in CodeMirrorOverrides.css as the default for placeholders.

[Blaze Burg]

Comment on attachment 265646 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=265646&action=review

> Source/WebInspectorUI/ChangeLog:8
> +        Updated breakpoint popover to use CodeMirror.

Nit: breakpoint :condition: popover

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:164
> +            this._conditionCodeMirror.refresh();

Yuck, but I guess we have no choice?

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:243
> +        let newtext = change.text.join("").replace(/\n/g, "");

typo: newText

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:244
> +        change.update(change.from, change.to, [newtext]);

Hopefully this is not too jarring while typing.

[Matt Baker]

Comment on attachment 265646 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=265646&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:-142
>> -        conditionLabel.setAttribute("for", conditionInput.id);
> 
> Hmm, is there any way for this to still work? Maybe you can get the CodeMirror input field, give that an id, and keep this relationship.

CodeMirror's inputStyle option determines the type of element underlying the editor. On desktop it is a textarea by default, which we can access and assign an ID to. I believe the default on mobile is a div.

>> Source/WebInspectorUI/UserInterface/Views/BreakpointPopoverController.css:74
>>  }
> 
> Given that these are the same as CSSStyleDeclarationTextEditor.css for its placeholder, maybe we should put these in CodeMirrorOverrides.css as the default for placeholders.

I'll break out the common CSS.

[Matt Baker]

Comment on attachment 265646 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=265646&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:164
>> +            this._conditionCodeMirror.refresh();
> 
> Yuck, but I guess we have no choice?

If we omit this the input field is sized incorrectly. We have to do this in other places too. :(

>> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:244
>> +        change.update(change.from, change.to, [newtext]);
> 
> Hopefully this is not too jarring while typing.

During my testing it wasn't noticeable at all.

[Matt Baker]

Created attachment 265707 [details]
[Patch] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 265707 [details]
[Patch] Proposed Fix

r=me

[WebKit Commit Bot]

Comment on attachment 265707 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 265707

Committed r192551: <http://trac.webkit.org/changeset/192551>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.