Closed Bug 1262491 Opened 6 years ago Closed 6 years ago

New node isn't focused when it's added, causing unexpected keyboard behavior

Categories

(DevTools :: Inspector, defect, P2)

46 Branch
defect

Tracking

(firefox48+ fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 + fixed

People

(Reporter: bgrins, Assigned: pbro)

References

Details

(Whiteboard: [btpp-fix-later])

Attachments

(1 file)

STR 1:

Open: data:text/html,<body id="hi">
Open inspector and focus id="hi" attribute
Click the "Add Node" button
Press delete

Expected:

The new node is deleted

Actual:

The attribute is deleted

STR 2:

Open any page
Go to data:text/html,<body id="hi">
Open inspector
Click the "Add Node" button
Press delete

Expected:

The new node is deleted

Actual:

The page navigates backwards
Thanks for filing. Indeed, although the new node is *selected*, it isn't being *focused*.
Gabriel and I were talking earlier today about this and we said that perhaps we should create the node directly in edit mode too (with the tagName focused) so that you can make changes directly. This would make it similar to the "add rule" experience in the rule-view.
So, maybe this could help.
[Tracking Requested - why for this release]: As discussed during triage, this is not a P1 (not a serious crash, breakage ...), but we still want to fix it before the next branch point so it gets released at the same time as bug 1261781.
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(In reply to Patrick Brosset [:pbro] from comment #4)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1e6c862bbc52&group_state=expanded
I'm going to have to fix the test changes I made. It's failing on linux right now. I've cancelled this try build in the meantime.
The code changes can still be reviewed.
Attachment #8741325 - Flags: review?(jdescottes)
Comment on attachment 8741325 [details]
MozReview Request: Bug 1262491 - Focus the newly inserted node in the markup-view when clicking the add button; r=jdescottes

https://reviewboard.mozilla.org/r/46397/#review42951

Thanks Patrick, code change looks good to me, just one (annoying) comment about a console warning!

Waiting for your test update to finish the review, but I wanted to publish my comment so that you can have a look.

::: devtools/client/inspector/inspector-panel.js:1069
(Diff revision 1)
>      let {nodes} = yield this.walker.insertAdjacentHTML(this.selection.nodeFront,
>                                                         "beforeEnd", html);
>      yield onMutations;
>  
>      // Select the new node (this will auto-expand its parent).
> -    this.selection.setNodeFront(nodes[0]);
> +    this.selection.setNodeFront(nodes[0], "node-inserted");

Not sure if this is an issue, but we almost always get a console.warn from breadcrumbs.js when focusing the newly inserted node:

console.warn: Asynchronous operation was aborted as selection changed.

The breadcrumbs are also listening to markupmutation to know when to refresh the breadcrumbs. See ensureFirstChild > getInterestingFirstNode > selectionGuard in breadcrumbs.js). The goal is to make sure that the breadcrumbs display the first child of the currently selected node in the breadcrumbs (seems a bit weird by the way).

On "markupmutation":
1. the breadcrumbs will start checking if there's a new child they should add to the breadcrumbs UI. 
2. at the same time, inspector-panel calls setNodeFront which will now select the node

The breadcrumbs uses a safeguard to make sure that the selection is the same between the moment the markupmutation was received and the moment the UI needs to be updated. Here of course, the selection from inspector-panel occurs in between, and we get a warning.

Don't know how much we need to pay attention to what we log. If we have to fix this we can either remove this warning (it doesn't really highlight an issue, simply a race condition) or the breadcrumbs could make sure to cancel any ongoing update when receiving a "markupmutation" or "new-node-front" event.

::: devtools/client/inspector/markup/markup.js:569
(Diff revision 1)
> +      "browser-context-menu",
> +      // If the user added a new node by clicking in the inspector toolbar.
> +      "node-inserted"
> +    ];
>  
> +    if (reasons.indexOf(reason) !== -1) {

nit: can use reasons.includes(reason)
(In reply to Julian Descottes [:jdescottes] from comment #6)
> Not sure if this is an issue, but we almost always get a console.warn from
> breadcrumbs.js when focusing the newly inserted node:
> 
> console.warn: Asynchronous operation was aborted as selection changed.
Thanks, I didn't notice this one.
> The breadcrumbs are also listening to markupmutation to know when to refresh
> the breadcrumbs. See ensureFirstChild > getInterestingFirstNode >
> selectionGuard in breadcrumbs.js). The goal is to make sure that the
> breadcrumbs display the first child of the currently selected node in the
> breadcrumbs (seems a bit weird by the way).
Yeah it's weird. I wonder if we should just kill this "feature". I don't have the historical reason for adding this in the first place, I suspect it was to ease navigation in the DOM. However, I don't think the breadcrumbs is a widget that people use for navigation a lot.
> On "markupmutation":
> 1. the breadcrumbs will start checking if there's a new child they should
> add to the breadcrumbs UI. 
> 2. at the same time, inspector-panel calls setNodeFront which will now
> select the node
> 
> The breadcrumbs uses a safeguard to make sure that the selection is the same
> between the moment the markupmutation was received and the moment the UI
> needs to be updated. Here of course, the selection from inspector-panel
> occurs in between, and we get a warning.
Right, that makes sense.
> Don't know how much we need to pay attention to what we log. If we have to
> fix this we can either remove this warning (it doesn't really highlight an
> issue, simply a race condition) or the breadcrumbs could make sure to cancel
> any ongoing update when receiving a "markupmutation" or "new-node-front"
> event.
We should make sure we don't log useless information that might confuse end users. Logging things that lead to bugs being filed is good. But logging for something that we know can and will happen is probably useless.
Note that in order to fix the test, I removed the blur/focus handling on the markup-view window in the test. I'm still checking that the activeElement is correct, and just assuming that if it is, then the window must have gained focused in the process anyway. Waiting for the focus even on the window was causing problems on linux, and I'd rather avoid it.
New patch incoming soon.
Comment on attachment 8741325 [details]
MozReview Request: Bug 1262491 - Focus the newly inserted node in the markup-view when clicking the add button; r=jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46397/diff/1-2/
Attachment #8741325 - Flags: review?(jdescottes)
Blocks: 1264907
Comment on attachment 8741325 [details]
MozReview Request: Bug 1262491 - Focus the newly inserted node in the markup-view when clicking the add button; r=jdescottes

https://reviewboard.mozilla.org/r/46397/#review43297

Nice! 
The fix for the test makes sense, I'm not surprised that window focus/blur gave you trouble on Linux :)
Attachment #8741325 - Flags: review?(jdescottes) → review+
Pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17c1f89682ea
All green on linux, but taking a really long time on other platforms, so I won't have time to land this before I go on PTO next week. So asking for checkin instead.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2013ce74d60d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Tracking in case this reopens before we release 48.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.