Summary: | Web Inspector: Breakpoint condition field should use CodeMirror | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Matt Baker
2015-11-16 17:15:34 PST
(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. Created attachment 265646 [details]
[Patch] Proposed Fix
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. 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. 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. 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. Created attachment 265707 [details]
[Patch] Proposed Fix
Comment on attachment 265707 [details]
[Patch] Proposed Fix
r=me
Comment on attachment 265707 [details] [Patch] Proposed Fix Clearing flags on attachment: 265707 Committed r192551: <http://trac.webkit.org/changeset/192551> All reviewed patches have been landed. Closing bug. |