Closed
Bug 817558
Opened 12 years ago
Closed 12 years ago
[inspector] When the selection is deleted, we should select the parent, and make sure the breadcrumbs are updated.
Categories
(DevTools :: Inspector, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 20
People
(Reporter: paul, Assigned: Optimizer)
References
Details
Attachments
(1 file, 3 obsolete files)
|
(deleted),
patch
|
Optimizer
:
review+
paul
:
review+
|
Details | Diff | Splinter Review |
As for now, the selection turns null (which is good, but not optimal).
|
Reporter | |
Updated•12 years ago
|
Priority: -- → P3
|
Assignee | |
Comment 2•12 years ago
|
||
Switch to parent node when the selected node is deleted.
|
|
Assignee | |
Comment 3•12 years ago
|
||
I have really lost the touch of mercurial :|
Attachment #692581 -
Attachment is obsolete: true
Attachment #692581 -
Flags: review?(paul)
Attachment #692583 -
Flags: review?(paul)
|
|
Assignee | |
Comment 4•12 years ago
|
||
green try at https://tbpl.mozilla.org/?tree=Try&rev=92c23c2ffc42
|
|
Assignee | |
Updated•12 years ago
|
Whiteboard: [has-patch]
|
|
Reporter | |
Comment 5•12 years ago
|
||
Comment on attachment 692583 [details] [diff] [review]
Actiual patch v1.0 with tests
> Selection.prototype = {
> _node: null,
>
> _onMutations: function(mutations) {
> let attributeChange = false;
> let detached = false;
> + let parentNode = null;
> for (let m of mutations) {
> if (!attributeChange && m.type == "attributes") {
> attributeChange = true;
> }
> if (m.type == "childList") {
> if (!detached && !this.isConnected()) {
> detached = true;
> + parentNode = m.target;
> }
> }
> }
>
> if (attributeChange)
> this.emit("attribute-changed");
> - if (detached)
> + if (detached && parentNode) {
> this.emit("detached");
> + this.setNode(parentNode, "detached");
So this is interesting. A couple of remarks:
1) Selection is now in charge of re-selecting a node
when it gets detached. I'm not sure this is the best
thing to do, but I can be convinced otherwise.
2) Is it possible to NOT have a parentNode?
3) Do we have to emit "detached" in this case? emit is synchronous,
so that means when we will get the "detached" event, the selected node
is still the one that has been deleted (not the parent yet). So we could
set the node first, then emit detach. But then… what's the point of
emitting "detached"? Being notified that the node has been deleted? We
know it thanks to the "reason" attribute.
> + } else {
> + this.emit("detached");
> + }
This means "detached" will be sent if (detached == false).
Attachment #692583 -
Flags: review?(paul)
|
|
Assignee | |
Comment 6•12 years ago
|
||
(In reply to Paul Rouget [:paul] from comment #5)
> So this is interesting. A couple of remarks:
>
> 1) Selection is now in charge of re-selecting a node
> when it gets detached. I'm not sure this is the best
> thing to do, but I can be convinced otherwise.
Other wise the other way is to add this.selection.on("detached", this) in InspectorPanel, and then I would also have to pass the parent node along with the this.emit("detached") call from selection. The effect will be same, its just that ownership will be different. I am okay with both. But..
> 2) Is it possible to NOT have a parentNode?
May be when deleting <html> ?
> 3) Do we have to emit "detached" in this case? emit is synchronous,
> so that means when we will get the "detached" event, the selected node
> is still the one that has been deleted (not the parent yet). So we could
> set the node first, then emit detach. But then… what's the point of
> emitting "detached"? Being notified that the node has been deleted? We
> know it thanks to the "reason" attribute.
I did this because the breadcrumbs were not getting refreshed when I was just using the setNode without emit("detached") . So I thought that it will be good to emit a detached, and then shift to its parent node because that is what actually is happening in reality too.
> > + } else {
> > + this.emit("detached");
> > + }
>
> This means "detached" will be sent if (detached == false).
Yes, its wrong. Will fix.
|
|
Reporter | |
Comment 7•12 years ago
|
||
(In reply to Girish Sharma [:Optimizer] from comment #6)
> (In reply to Paul Rouget [:paul] from comment #5)
> > So this is interesting. A couple of remarks:
> >
> > 1) Selection is now in charge of re-selecting a node
> > when it gets detached. I'm not sure this is the best
> > thing to do, but I can be convinced otherwise.
>
> Other wise the other way is to add this.selection.on("detached", this) in
> InspectorPanel, and then I would also have to pass the parent node along
> with the this.emit("detached") call from selection. The effect will be same,
> its just that ownership will be different. I am okay with both.
I like that very much.
> But..
But what?
|
|
Assignee | |
Comment 8•12 years ago
|
||
(In reply to Paul Rouget [:paul] from comment #7)
> But what?
Damnit, now even I forgot. I will reply when I remember. Maybe while implementing the patch ..
|
|
Assignee | |
Comment 9•12 years ago
|
||
So my But..
Right now, the only consumer of the "detached" event are the highlighter and breadcrumbs which calls the highlight() and update() method and if that method was called due to detached event, it hides the infobar and breadcrumbs.
So now, if I move the logic of selecting the parent node on detached event fired is moved to inspector panel, then it would still mean that I will add an on("detached") in inspectorPanel, check whether a parentNode is non null, and do a setNode().
What I am worried now is, that since emit is synchronous, it might lead to the calling of the detached handler of inspectorPanel firstly and in turn setNode method would be called before the detached handler of breadcrumbs and selection .
|
|
Assignee | |
Comment 10•12 years ago
|
||
Note : Tilt is also consuming the detached event ..
|
|
Reporter | |
Comment 11•12 years ago
|
||
What if only the inspector listen to "detached"?
Then, on detach, it select the parent, and that's it.
What's wrong with that? You're afraid that cascading the setNode() (because of the synchronous nature of emot) will do something weird?
Can you try?
|
|
Assignee | |
Comment 12•12 years ago
|
||
Now only inspectorPanel consumes the detached event. It refreshes the breadcrumbs and then selects the parent node.
Asking Victor review too, as I deleted some lines from Tilt, which might or might not break something. Although I checked Tilt, it was working fine.
Attachment #692583 -
Attachment is obsolete: true
Attachment #693072 -
Flags: review?(vporof)
Attachment #693072 -
Flags: review?(paul)
|
|
Reporter | |
Comment 13•12 years ago
|
||
Comment on attachment 693072 [details] [diff] [review]
patch v2.0
r+ for the code. But I need to test. Please rebase on fx-team.
Attachment #693072 -
Flags: review?(vporof)
Attachment #693072 -
Flags: review?(paul)
Attachment #693072 -
Flags: review+
|
|
Assignee | |
Comment 14•12 years ago
|
||
Paul, please test this and land it in fx-team if all is good :)
Attachment #693490 -
Flags: review+
Flags: needinfo?(paul)
|
||
Updated•12 years ago
|
Whiteboard: [has-patch] → [r+][needinfo]
|
|
Reporter | |
Comment 15•12 years ago
|
||
Comment on attachment 693490 [details] [diff] [review]
Patch rebased on latest fx-team
Tested. It works pretty well :D
Attachment #693490 -
Flags: review+
Flags: needinfo?(paul)
|
|
Reporter | |
Updated•12 years ago
|
Attachment #693072 -
Attachment is obsolete: true
|
|
Reporter | |
Updated•12 years ago
|
Whiteboard: [r+][needinfo] → [land-in-fx-team]
|
|
Reporter | |
Comment 16•12 years ago
|
||
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
|
||
Comment 17•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 20
|
||
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•