Closed
Bug 1262491
Opened 9 years ago
Closed 9 years ago
New node isn't focused when it's added, causing unexpected keyboard behavior
Categories
(DevTools :: Inspector, defect, P2)
Tracking
(firefox48+ fixed)
RESOLVED
FIXED
Firefox 48
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
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
[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.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46397/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46397/
Attachment #8741325 -
Flags: review?(jdescottes)
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Comment 5•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8741325 -
Flags: review?(jdescottes)
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Comment 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
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)
Comment 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
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
Comment 12•9 years ago
|
||
Keywords: checkin-needed
Comment 13•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Tracking in case this reopens before we release 48.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•