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)

Product:
DevTools
Component:
Inspector
Platform:
x86
All
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 20

People

(Reporter: paul, Assigned: Optimizer)

References

Details

Attachments

(1 file, 3 obsolete files)

Patch rebased on latest fx-team
8.77 KB, patch
Optimizer
: review+
paul
: review+
Details | Diff | Splinter Review 
As for now, the selection turns null (which is good, but not optimal).
Priority: -- → P3
Attached patch Patch v1.0 with tests (obsolete) — — Splinter Review 
Switch to parent node when the selected node is deleted.
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Attachment #692581 - Flags: review?(paul)
Attached patch Actiual patch v1.0 with tests (obsolete) — — Splinter Review 
I have really lost the touch of mercurial :|
Attachment #692581 - Attachment is obsolete: true
Attachment #692581 - Flags: review?(paul)
Attachment #692583 - Flags: review?(paul)
Whiteboard: [has-patch]
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)
(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.
(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?
(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 ..
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 .
Note : Tilt is also consuming the detached event ..
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?
Attached patch patch v2.0 (obsolete) — — Splinter Review 
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)
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+
Paul, please test this and land it in fx-team if all is good :)
Attachment #693490 - Flags: review+
Flags: needinfo?(paul)
Whiteboard: [has-patch] → [r+][needinfo]
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)
Attachment #693072 - Attachment is obsolete: true
Whiteboard: [r+][needinfo] → [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/0248cb0ceeb6
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0248cb0ceeb6
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 20
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: