902966 - Pressing escape while editing a style property should undo current changes

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Tomko [:kibaurufu]]

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.

Comment 1

[Mike Tomko [:kibaurufu]]

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.

Comment 2

[Mike Tomko [:kibaurufu]]

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.

Comment 3

[Patrick Brosset <:pbro>]

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.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Patch

Comment 5

[Heather Arthur [:harth]]

Try:

https://tbpl.mozilla.org/?tree=Try&rev=7df37d768aac

Comment 6

[Patrick Brosset <:pbro>]

Attachment: bug-902966.patch

Added a new mochitest to make sure live preview happens on keypress

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug-902966-ESC-revert-value.patch

Removed the trailing whitespaces

Comment 9

[Heather Arthur [:harth]]

https://hg.mozilla.org/integration/fx-team/rev/5de48b36c574

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5de48b36c574