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

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: canuckistani, Assigned: gl)

Tracking

41 Branch
Firefox 41
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 10 obsolete attachments)

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:
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
Flags: needinfo?(pbrosset)
Flags: needinfo?(gabriel.luong)
Assignee: nobody → gabriel.luong
Flags: needinfo?(gabriel.luong)
Depends on: 1139058
Status: NEW → ASSIGNED
Posted patch 1166959.patch (obsolete) — Splinter Review
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.
Attachment #8608547 - Flags: feedback?(pbrosset)
Patch depends bug 1139058.
Posted patch 1166959.patch (obsolete) — Splinter Review
Updated patch to work with the latest version 2.0 patches of bug 1139058.
Attachment #8608547 - Attachment is obsolete: true
Attachment #8608547 - Flags: feedback?(pbrosset)
Attachment #8609216 - Flags: feedback?(pbrosset)
Attachment #8609216 - Flags: feedback?(pbrosset)
(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.
Flags: needinfo?(pbrosset)
Posted patch 1166959.patch (obsolete) — Splinter Review
Attachment #8609216 - Attachment is obsolete: true
Attachment #8610064 - Flags: feedback?(pbrosset)
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.
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.
Posted patch 1166959.patch (obsolete) — Splinter Review
Attachment #8610064 - Attachment is obsolete: true
Attachment #8610064 - Flags: feedback?(pbrosset)
Attachment #8610323 - Flags: feedback?(pbrosset)
Posted patch 1166959.patch (obsolete) — Splinter Review
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
Attachment #8610323 - Attachment is obsolete: true
Attachment #8610323 - Flags: feedback?(pbrosset)
Summary: Add rule focus issue → Tapping "Enter" after adding a new rule should focus the property editor
Summary: Tapping "Enter" after adding a new rule should focus the property editor → Tabbing and entering after editing a selector should focus the property editor
No longer blocks: 1165122
Blocks: 1165122
Posted patch 1166959.patch [1.0] (obsolete) — Splinter Review
Attachment #8610395 - Attachment is obsolete: true
Attachment #8624064 - Flags: review?(pbrosset)
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
Attachment #8624064 - Flags: review?(pbrosset)
Posted patch 1166959.patch [2.0] (obsolete) — Splinter Review
Attachment #8624064 - Attachment is obsolete: true
Posted patch 1166959.patch [3.0] (obsolete) — Splinter Review
Attachment #8624078 - Attachment is obsolete: true
Attachment #8624078 - Flags: review?(pbrosset)
Attachment #8624402 - Flags: review?(pbrosset)
(In reply to Gabriel Luong [:gl] from comment #16)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6702f3b3910c

Is this something we can test?
Flags: needinfo?(gabriel.luong)
(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.
Flags: needinfo?(gabriel.luong)
(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 on attachment 8624402 [details] [diff] [review]
1166959.patch [3.0]

It appears I introduced a bug with the !aDirection check in moveSelectorFocus.
Attachment #8624402 - Flags: review?(pbrosset)
Posted patch 1166959.patch [4.0] (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be82b892c309
Attachment #8624402 - Attachment is obsolete: true
Attachment #8624842 - Flags: review?(pbrosset)
(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!
Posted patch 1166959.patch [5.0] (obsolete) — Splinter Review
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
Attachment #8624842 - Attachment is obsolete: true
Attachment #8624842 - Flags: review?(pbrosset)
Attachment #8624950 - Flags: review?(pbrosset)
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.
Attachment #8624950 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/c73a81f1802b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.