Closed Bug 902966 Opened 8 years ago Closed 8 years ago

Pressing escape while editing a style property should undo current changes

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: kibaurufu, Assigned: pbro)

Details

Attachments

(1 file, 2 obsolete files)

In the styles inspector, when editing a style property, pressing [esc] currently does the same action as [enter]. This is not intuitive, as with most editing software, pressing [esc] reverts any 'unsaved changes', which is currently what the html view of the inspect tab does.


Steps to reproduce:
Begin editing any style property in the styles inspector, change the value from what it currently is and hit [esc].

What happens:
The changes are saved.

What should have happened:
The changes should have reverted to the value it had before it was edited.
This is reproducible in both the current release (FF23) and the nightly (20130807).

I assume it is also reproducible on most (if not all) platforms, but I cannot confirm this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 23 Branch → Trunk
After hunting through the relevant code, it seems the issue is that the value is getting applied on every keyup because inplace-editor.js's _onKeyup function calls _onBlur which always calls _apply.

I'm not sure why _onBlur is being called by _onKeyup and I couldn't find a previous bug explaining a need for it, so i'm a little hesitant to include a patch which removes the _onBlur call.


Based on this info, I have changed the bug's platform info to reflect the platform independence of this bug.
OS: Mac OS X → All
Hardware: x86_64 → All
Assignee: nobody → pbrosset
Thanks Mike, indeed the inplace-editor used for this is defining a validate function, which, with the current code, was forcing a call to _onBlur on each key up event.
This was basically committing the value at each key stroke.
I've fixed this and added a test for it (for info, just not calling _onBlur anymore basically kills the live preview feature, so I added this back in the validate function itself).

Also, another use case appeared, pressing [Shift+TAB] would end up doing the same as pressing [ESC]. Other dev tools however commit the value on both [TAB] and [Shift-TAB], so I changed that as well and added a test.

Attaching a patch now.
Attached patch Patch (obsolete) — Splinter Review
Attached patch bug-902966.patch (obsolete) — Splinter Review
Added a new mochitest to make sure live preview happens on keypress
Attachment #793541 - Attachment is obsolete: true
Attachment #793670 - Flags: review?(mratcliffe)
Comment on attachment 793670 [details] [diff] [review]
bug-902966.patch

Review of attachment 793670 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me with only one nit.

::: browser/devtools/styleinspector/test/browser_ruleview_bug_902966_revert_value_on_ESC.js
@@ +54,5 @@
> +  let idRuleEditor = ruleView.element.children[1]._ruleEditor;
> +  let propEditor = idRuleEditor.rule.textProps[0].editor;
> +  waitForEditorFocus(propEditor.element, function(aEditor) {
> +    is(inplaceEditor(propEditor.valueSpan), aEditor, "Focused editor should be the value.");
> +    

Trailing space
Attachment #793670 - Flags: review?(mratcliffe) → review+
Removed the trailing whitespaces
Attachment #793670 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/5de48b36c574
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5de48b36c574
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.