1166959 - Tabbing and entering after editing a selector should focus the property editor

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

STR:

* open a page ( this bug ), select the body tag
* right-click in the rule view, select 'add rule'
* accept the default rule selector, hit enter

Expected - should be able to add css properties and values

Actual - focus goes nowhere. Hitting tab gets me to the filter rules entry. Interaction should closely follow chrome and firebug here.

See feedback post:

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Sorry, feedback post is http://alfonsoml.blogspot.com.es/2015/05/firefox-devtools-long-way-to-go.html

gl, Patrick - is this a regression? If so we should fix for June 2

Comment 2

[Gabriel [:gl] (ΦωΦ)]

Attachment: 1166959.patch

Patrick, I wanted to get your opinion on this potential solution. I am not too familiar with the inplace editor, but one issue I noticed was that the focus does not get moved to selector text when tabbing around the rule view. Based on my short scan of the code, the focus should be looking for the next editable field, but it seems to skip the selector text.

Comment 3

[Gabriel [:gl] (ΦωΦ)]

Patch depends bug 1139058.

Comment 4

[Gabriel [:gl] (ΦωΦ)]

Attachment: 1166959.patch

Updated patch to work with the latest version 2.0 patches of bug 1139058.

Comment 5

[Patrick Brosset <:pbro>]

(In reply to Jeff Griffiths (:canuckistani) from comment #1)
> Sorry, feedback post is
> http://alfonsoml.blogspot.com.es/2015/05/firefox-devtools-long-way-to-go.html
> 
> gl, Patrick - is this a regression? If so we should fix for June 2
No I don't think it's a regression.
Gabriel, I see you've removed the feedback flag there, will you have a new patch to upload? I've just finished reviewing bug 1139058, so I can give you a hand investigating here if you need.

Comment 6

[Gabriel [:gl] (ΦωΦ)]

Attachment: 1166959.patch

Comment 7

[Gabriel [:gl] (ΦωΦ)]

There are a couple ways of going about this bug. I could remove the stopOnTab and stopOnReturn options in the this.selectorText editableField, but I would run to an error for edit_selector_commit test because after editing to a new selector and tabbing or returning I would run into an error because it is trying to modify the properties for a rule that is no longer there.

Alternatively, we can add a new function to move the selector focus, which is what I have done with the latest patch. One problem that I am encountering is if I enter on a selector text that has not changed, the next editable field that is focused is not selected (eg, input field displays, but the text is not selected). Either options we need to handle the next focusable item because we are replacing the existing RuleEditor.

The missing text selection after tabbing / entering only seems to happen under the condition I enter / tab when the selector text is the same.

Comment 8

[Gabriel [:gl] (ΦωΦ)]

To comment on the first solution, I would need to add checks for whether or not this rule is still existing when modify properties and returning early. I didn't drive too deep with this solution since the alternative solution seems more straight forward.

Comment 9

[Gabriel [:gl] (ΦωΦ)]

Attachment: 1166959.patch

Comment 10

[Gabriel [:gl] (ΦωΦ)]

Attachment: 1166959.patch

Fixed the earlier selection range problem by setting the selection range manually. Still need to figure out how to make browser_ruleview_edit-selector-commit.js pass.

Error shown:
onsole.error:
  Message: TypeError: sheet.ownerNode is null
  Stack:
    StyleRuleActor<.getDocument@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/styles.js:990:7
StyleRuleActor<.modifyProperties<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/styles.js:1097:18
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1006:19
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1456:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:561:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
StyleRuleActor<.getDocument@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/styles.js:990:7
StyleRuleActor<.modifyProperties<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/styles.js:1097:18
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1006:19
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1456:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:561:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
[74715] WARNING: '!widget', file /Users/oracle/Projects/fx-team/dom/events/IMEStateManager.cpp, line 380

Comment 11

[Gabriel [:gl] (ΦωΦ)]

Attachment: 1166959.patch [1.0]

Comment 12

[Gabriel [:gl] (ΦωΦ)]

Added a check for sheet.ownerNode in styles#getDocument in the case it is null as seen in comment 10, and return the rule. Thinking about it some more this doesn't seem to be the best solution.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccd41a865b33

Comment 13

[Gabriel [:gl] (ΦωΦ)]

Attachment: 1166959.patch [2.0]

Comment 14

[Gabriel [:gl] (ΦωΦ)]

Comment on attachment 8624078 [details] [diff] [review]
1166959.patch [2.0]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b783cfa9c530

Comment 15

[Gabriel [:gl] (ΦωΦ)]

Attachment: 1166959.patch [3.0]

Comment 16

[Gabriel [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6702f3b3910c

Comment 17

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Gabriel Luong [:gl] from comment #16)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6702f3b3910c

Is this something we can test?

Comment 18

[Gabriel [:gl] (ΦωΦ)]

(In reply to Jeff Griffiths (:canuckistani) from comment #17)
> (In reply to Gabriel Luong [:gl] from comment #16)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=6702f3b3910c
> 
> Is this something we can test?

Ya, you should be able to just grab a try build and test out the focus after moving from editing the selector. The try looks like it is on fire, but this is due to other things.

Comment 19

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Gabriel Luong [:gl] from comment #18)
> (In reply to Jeff Griffiths (:canuckistani) from comment #17)
> > (In reply to Gabriel Luong [:gl] from comment #16)
> > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=6702f3b3910c
> > 
> > Is this something we can test?
> 
> Ya, you should be able to just grab a try build and test out the focus after
> moving from editing the selector. The try looks like it is on fire, but this
> is due to other things.

Based on my understanding of how this is supposed to work, it's not working yet. What I did:

1. open the inspector and focus on an element
2. right-click -> add new rule ( a new rule is populated )
3. hit enter

I'm expecting for focus to shift so that I can enter a css property, but nothing happens.

See https://dl.dropboxusercontent.com/u/1212936/lice/add-rule-focus.gif

Comment 20

[Gabriel [:gl] (ΦωΦ)]

Comment on attachment 8624402 [details] [diff] [review]
1166959.patch [3.0]

It appears I introduced a bug with the !aDirection check in moveSelectorFocus.

Comment 21

[Gabriel [:gl] (ΦωΦ)]

Attachment: 1166959.patch [4.0]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be82b892c309

Comment 22

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Gabriel Luong [:gl] from comment #21)
> Created attachment 8624842 [details] [diff] [review]
> 1166959.patch [4.0]
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=be82b892c309

looks good!

Comment 23

[Gabriel [:gl] (ΦωΦ)]

Attachment: 1166959.patch [5.0]

Updated the tests to also check the active element is correct after ENTER, TAB, SHIFT+TAB and ESCAPE.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23d5e56b629d

Comment 24

[Patrick Brosset <:pbro>]

Comment on attachment 8624950 [details] [diff] [review]
1166959.patch [5.0]

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

::: browser/devtools/shared/inplace-editor.js
@@ +959,5 @@
>        }
>        if ((this.stopOnReturn &&
>             aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_RETURN) ||
> +          (this.stopOnTab && aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_TAB &&
> +           !aEvent.shiftKey)) {

My god, this _onKeyPress method is really complex, long, lot's of conditions, and none of the conditions are self-explanatory. Can you file a bug for cleaning up inplace-editor.js and in particular reduce the complexity of this method?

::: browser/devtools/styleinspector/rule-view.js
@@ +3008,2 @@
>     */
> +  _onSelectorDone: function(aValue, aCommit, aDirection) {

Since you're changing this method declaration, can you take this opportunity to get rid of the aArgument style notation? https://wiki.mozilla.org/DevTools/CodingStandards#Code_style

@@ +3063,5 @@
> +  },
> +
> +  /**
> +   * Handle moving the focus change after pressing tab and return from the
> +   * selector inplace editor

Can you please add a comment here explaining that the focused element after TAB was pressed is lost, because the rule editor is replaced?

@@ +3070,5 @@
> +   *        The Rule object.
> +   * @param {number} aDirection
> +   *        The move focus direction number.
> +   */
> +  _moveSelectorFocus: function(aRule, aDirection) {

Can you rename aRule to rule and aDirection to direction.

@@ +3078,5 @@
> +
> +    if (aRule.textProps.length > 0) {
> +      aRule.textProps[0].editor.nameSpan.click();
> +      let input = aRule.textProps[0].editor.nameSpan.inplaceEditor.input;
> +      input.setSelectionRange(0, input.textLength);

As discussed, the inplace-editor should do this normally, so if it can't, we should at least have a comment here that explains why, and why we can't fix it.

Comment 25

[Gabriel [:gl] (ΦωΦ)]

Attachment: 1166959.patch [6.0]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e35aacf9195b

Comment 26

[Pulsebot]

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

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c73a81f1802b