Closed Bug 1269226 Opened 8 years ago Closed 8 years ago

Breadcrumbs in inspector don't update on element delete when deletion causes inner text to be selected

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Version:
36 Branch
Type:
defect

Tracking

(firefox50 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
firefox50 --- verified

People

(Reporter: steve.j.melia, Assigned: steve.j.melia)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(1 file, 8 obsolete files)

1269226-rebase-context-menu.patch
(deleted), patch
pbro
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160304114936

Steps to reproduce:

1. Paste "data:text/html,<body><div><div><div><div><div><div><div><div>hi<div>h2</div>." in address bar
2. Right click "inspect element" on h2 text
3. Press delete key

Also occurs in Nightly (trunk?)


Actual results:

Node is deleted but breadcrumb trail is not updated - & clicking on the last div will cause a crash


Expected results:

Breadcrumb trail should be updated to remove deleted node.
To clarify - will crash the breadcrumb trail... not the whole browser. I'm going to take a look at this when I get a chance so could assign it to me if possible?
Assignee: nobody → steve.j.melia
URL: data:text/html,<body><div><div><div><...
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → Developer Tools: Inspector
Ever confirmed: true
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=be4ba3d5ca9a&tochange=74edfbf0e6a3
Has Regression Range: --- → no
Has STR: --- → yes
Keywords: regression, regressionwindow-wanted
Version: 45 Branch → 36 Branch
Attached patch 1269226.patch (obsolete) (deleted) — — Splinter Review 
Hi Patrick hope it's OK to ask you for a review - if not please let me know who else is available.

The patch ensures functions involved in updating breadcrumbs after mutation can cope with the current selection being a text node, then removes the shortcut for text nodes.

Added a mochitest for deletion with a trailing text node; and linted the test file.
Attachment #8749452 - Flags: review?(pbrosset)
Removing regressionwindow-wanted keyword because the pushlog was already provided in comment 2. After doing a regression I found exactly the same pushlog.
Keywords: regressionwindow-wanted
Comment on attachment 8749452 [details] [diff] [review]
1269226.patch

Oh ok I've misunderstood I didn't realise that pushlog had been confirmed as the regression point. I gave it a quick read but couldn't see how a regression had occurred, looked like style changes only.

On the understanding that this is definitely where the regression occured, I'd like to take the time to understand why before committing the patch
Attachment #8749452 - Flags: review?(pbrosset)
Priority: -- → P2
Comment on attachment 8749452 [details] [diff] [review]
1269226.patch

Ok I am pretty sure i've found where the issue first occurs; but it does contradict the pushlog; I think it's this commit https://hg.mozilla.org/mozilla-central/rev/8f2f3001ab0b

I struggled (and gave up) building any of the revisions in the pushlog due to mach using system python packages (e.g. psutil) so I found this by looking for changes to likely files around the same time as the identified by mozregression. I'm not sure why it's not captured by mozregression - maybe something to do with timing of CI builds? Unless it is unexpected that the commit might be outside of the pushlog; i'm pretty confident this is the cause.

This changes the behaviour after deletion to select the next sibling instead of the parent (container) node; so I think my patch makes sense in this context.
Attachment #8749452 - Flags: review?(pbrosset)
Comment on attachment 8749452 [details] [diff] [review]
1269226.patch

Review of attachment 8749452 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay here.
First, bug 1264907 (which I've been working on) is landing soon and will remove the ensureFirstChild function, so there's going to be some merging needed.
But, I'm afraid this isn't the right approach. After spending time investigating the issue, I found out that the problem lies with the way we select another element after the current one has been deleted.
In fact, the bug isn't about the breadcrumbs, it's about the fact that there are no selected elements in the markup-view after you've delete the selected one. This should not happen.

The logic happens in deleteNode in devtools/client/inspector/markup/markup.js
In this method, we call this.walker.removeNode(node) which removes the node from the tree and sends back a next and prev sibling that we can use as a new selection instead.
In the provided test case, the prev sibling that is returned is a text node. But, and that's where the problem is, when we have only 1 text node as the only child of an element, we use a special display for that element: the element isn't expandable anymore, and the text node appears inline.
Which means that the text node isn't a separate entry in the markup-view anymore. So we can't select it.

So, in deleteNode, instead of doing:
this.navigate(this.getContainer(focusNode));

we should first check if focusNode is an only text node child. If it is, then we should select its parent instead. Something like this: focusNode.parentNode().singleTextChild
Attachment #8749452 - Flags: review?(pbrosset)
No worries about the merge.

I see what you mean about the case where there is a single node; it sounds like the in-lined text node is supposed to now be unselectable; but can be selected by deleting its last sibling. I can add another test case and implement your fix for this.

But as you say this is "when we only have 1 text node as the only child of an element" - what about the given test case, where there are two text nodes? In this instance; the markup pane would surely still select a text node, even with your fix, since singleTextChild != true. So I think my changes are still required to prevent the breadcrumb trail choking.

I did think perhaps you could in-line when >1 text nodes; or select the parent when the container contains only text nodes; but this would not really solve this particular defect, since for example:

data:text/html,<body><div><div><div><div><div><div><div><div>1<div>2</div>3<div>4</div>

deleting the <div>4</div> in-lined node would result in the selection of (non-in-lined) text node 3.
(In reply to Steve Melia from comment #8)
> But as you say this is "when we only have 1 text node as the only child of
> an element" - what about the given test case, where there are two text
> nodes? In this instance; the markup pane would surely still select a text
> node, even with your fix, since singleTextChild != true. So I think my
> changes are still required to prevent the breadcrumb trail choking.
You are right, if after deleting the node there are 2 text nodes remaining, then there will be no inlining and a text node will be selected. So we should make sure the breadcrumbs gets updated properly in this case.
Having said this, the fix should probably live in 'update', right?
Here there's a DOM mutation, a node is being removed, so 'update' should know how to deal with this and re-render the right content of the breadcrumbs.
Right now, it basically only assumes that markupmutations change existing nodes, but there's no code in there to remove elements from the breadcrumbs when those elements are removed from the DOM.

I think there should be an extra step in 'update' that checks if reason === "markupmutation" and if there are childList mutations with nodes being removed. When this happens, it should cycle through this.nodeHierarchy to check if there are nodes in there that are actually being removed. If there are, it should call this.cutAfter with the corresponding index.

At least, that's how I would envisage the fix to be.
Attached patch 1269226-handle-mutation.patch (obsolete) (deleted) — — Splinter Review 
Ok i've implemented the delete mutation as you suggested, I might have got a bit carried away refactoring the test... also I mix and match using the context menu and the delete key for the multiple text node scenario as per Bug 1272969.

Regarding the inlined node scenario; i've had a go at implementing your suggestion in Comment 7 but got stuck because (I think) focusNode.parentNode() returns the stale client-side parent; prior to the delete, which therefore has no singleTextChild. I can't figure out how to round-trip and refresh the parent; and it's making me think perhaps i've got the wrong end of the stick somewhere.

This leaves my browser_inspector_delete-selected-node-02.js:91 incorrect as it should assert for Node.ELEMENT_NODE, but is selecting the parent definitely the expected behaviour? It seems that Bug 892935 adds some inconsistency in that non-inlined nodes can be selected; dragged around, deleted etc. while inlined nodes may not. 

I think I would expect that if I click the text node portion of the inline text node; that i've selected that inner node; the little focus border reinforces that expectation.
Attachment #8749452 - Attachment is obsolete: true
Attachment #8753103 - Flags: feedback?(pbrosset)
Attached patch 1269226-pick-parent-locally.patch (obsolete) (deleted) — — Splinter Review 
Updated the patch to add picking the parent locally - if it had two children prior to the deletion; and the sibling is a text node; then it's going to be in-lined.
Attachment #8753103 - Attachment is obsolete: true
Attachment #8753103 - Flags: feedback?(pbrosset)
Attachment #8755042 - Flags: feedback?(pbrosset)
Comment on attachment 8755042 [details] [diff] [review]
1269226-pick-parent-locally.patch

Review of attachment 8755042 [details] [diff] [review]:
-----------------------------------------------------------------

Cool! This seems to work great.
Just one question below about the numChildren which I found strange.

::: devtools/client/inspector/markup/markup.js
@@ +826,5 @@
> +
> +          // If the (stale) parent nodes' other child is a text
> +          // node; it's going to get inlined and we should focus
> +          // the parent.
> +          let nextSiblingText = nextSibling ?

Please rename to isNextSiblingText and next one to isPrevSiblingText.

@@ +831,5 @@
> +            nextSibling.nodeType === Ci.nsIDOMNode.TEXT_NODE : false;
> +          let prevSiblingText = prevSibling ?
> +            prevSibling.nodeType === Ci.nsIDOMNode.TEXT_NODE : false;
> +
> +          if (parent.numChildren === 2

Why 2 here? Has the parent node's numChildren not been updated yet when this code runs?

::: devtools/client/inspector/test/browser_inspector_delete-selected-node-02.js
@@ +148,5 @@
> +    let breadcrumbs = inspector.panelDoc.getElementById(
> +        "inspector-breadcrumbs");
> +    is(breadcrumbs.querySelector("button[checked=true]").textContent,
> +        crumbLabel,
> +        "The right breadcrumb is selected");

There are many instances of this in this file, it's a minor comment, but the goal is to have a consistent formatting so that the code is easily readable and predictable:

we normally indent wrapped lines with 2 spaces, or we align arguments vertically. So, for instance:

let breadcrumbs = inspector.panelDoc.getElementById("inspector-breadcrumbs");

this line is too long, we normally wrap it as you did, but indent the 2nd line with 2 spaces only:

let breadcrumbs = inspector.panelDoc.getElementById(
  "inspector-breadcrumbs");

Another example is 

is(breadcrumbs.querySelector("button[checked=true]").textContent, crumbLabel, "The right breadcrumb is selected");

We also wrap it as you did, but in this case, we usually vertically align arguments:

is(breadcrumbs.querySelector("button[checked=true]").textContent,
   crumbLabel,
   "The right breadcrumb is selected");
Attachment #8755042 - Flags: feedback?(pbrosset) → feedback+
Attached patch 1269226-style.patch (obsolete) (deleted) — — Splinter Review 
Regarding numChildren yes I understand "parent" as being a proxy/DTO for a network/RPC call; I couldn't figure out how to round-trip it (Comment 10) so I've given up and calculate it locally... (Comment 11)

I think that we could modify the removeNode call to also return the parent to the resolve; but I read around a few bugs and it looked like this introduces complexity due to versioning between clients/servers?

Anyway, i've had a go at clarifying the comment on this point, in case you are happy with my current implementation!
Attachment #8755042 - Attachment is obsolete: true
Attachment #8756112 - Flags: review?(pbrosset)
Comment on attachment 8756112 [details] [diff] [review]
1269226-style.patch

Review of attachment 8756112 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, thanks!
Can you push to TRY or do you want me to do it?
Attachment #8756112 - Flags: review?(pbrosset) → review+
I don't have access - could you do it for me? I'll apply for this tonight if you will vouch for me!
Attached patch 1269226-rebased.patch (obsolete) (deleted) — — Splinter Review 
I had to rebase this on top of the latest fx-team, nothing serious, just formatting merges.
Pending TRY build for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51a6725da40e
Attachment #8756112 - Attachment is obsolete: true
Attachment #8756335 - Flags: review+
Keywords: checkin-needed
Attached patch 1269226-windows.patch (obsolete) (deleted) — — Splinter Review 
It is a Windows specific error I think, as you can probably guess from the try; the test works fine on Linux but I was able to replicate it in Windows 10. (Although not running through the tests steps manually - which works fine)

The VK_DELETE keypress was not working, my hypothesis is that the "synthetic focus" is lost, possibly due to the context menu. To correct this, I have added in a synthetic mouse click on the node in the markup view.

I am not sure it is a satisfactory solution; mainly because I have not really been able to understand why the behaviour differs across platforms; I don't know whether you think this needs pursuing, it is possibly these kinds of differences are not unexpected?

I have only retested browser_inspector_delete-selected-node-02.js on Windows, I have not tried this on Linux or re-run any other tests, as the production code has not changed.
Attachment #8756335 - Attachment is obsolete: true
Attachment #8759419 - Flags: review?(pbrosset)
Comment on attachment 8759419 [details] [diff] [review]
1269226-windows.patch

Review of attachment 8759419 [details] [diff] [review]:
-----------------------------------------------------------------

The test change looks good to me.
You need to update the test once more though (see below).
I have the change locally and pushed to try to see if it works now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=511cadb4bbd8

::: devtools/client/inspector/test/browser_inspector_delete-selected-node-02.js
@@ +133,5 @@
> +    is(actualNodeType, expectedNodeType, "The node has the right type");
> +
> +    let breadcrumbs = inspector.panelDoc
> +                               .getElementById("inspector-breadcrumbs")
> +                               .childNodes;

Unfortunately, bug 1259812 breaks this, so you'll need to change it to:

    let breadcrumbs = inspector.panelDoc.querySelectorAll(
      "#inspector-breadcrumbs .html-arrowscrollbox-inner > *");
Attachment #8759419 - Flags: review?(pbrosset) → review+
Just an update: The Try indicates the test is now failing on Mac OSX which I can't really debug locally; i'm thinking about tackling the test differently although I have not made any progress on this yet.

The try also didn't run any Windows tests which is a bit strange since the syntax seems to be correct.
Attached patch 1269226-no-synth-click.patch (obsolete) (deleted) — — Splinter Review 
Same patch... but no synth click this time just focussing the markup pane before the VK_DELETE. Reran browser_inspector_delete-selected-node-02.js locally on Windows and Linux.
Attachment #8759419 - Attachment is obsolete: true
Attachment #8762331 - Flags: review?(pbrosset)
Attached patch 1269226-rebase-context-menu.patch (obsolete) (deleted) — — Splinter Review 
As above but rebased; requiring a small change to close the context menu before synthesizing the delete keypress.
Attachment #8762331 - Attachment is obsolete: true
Attachment #8762331 - Flags: review?(pbrosset)
ashamed - forgot to eslint after my latest changes...
Attachment #8763710 - Attachment is obsolete: true
Attachment #8763716 - Flags: review?(pbrosset)
Comment on attachment 8763716 [details] [diff] [review]
1269226-rebase-context-menu.patch

Review of attachment 8763716 [details] [diff] [review]:
-----------------------------------------------------------------

I looked at the code changes made since my last review and didn't see anything worth commenting on. If TRY is happy, then I am too.
Attachment #8763716 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/3e793c26a910
Explicitly handle node deletion in inspector breadcrumbs. r=pbro
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3e793c26a910
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
I have reproduced this Bug on Nightly 49.0a1 (2016-05-01) on Windows 10, 64 Bit!

The bug's fix is now verified on latest Firefox Developer Edition 50.0a2 

Aurora   50.0a2 :
Build ID	   : 20160825004011
User Agent 	   : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

[bugday-20160824]
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
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: