966787 - Inspector html breadcrumbs is unstable on deeply nested HTML

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Codacoder]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

1 - open google.com (google homepage)
2 - open inspector
3 - in inspector, click the "Pick" button
4 - in the browser, click (select) "I'm feeling lucky" button
   - Wait for inspector to settle down
5 - Mouse around the google home page




Actual results:

Inspector tries to hilite the changing HTML in the inspector pane - watch the html breadcrumbs: for me, it scrolls.  

In my app, which is much more deeply nested than google home page and contains a never-ending animation which modifies the html, the breadcrumb display almost never quits moving - it may pause for, say, 5 secs, but then starts moving again for no apparent reason).  If it does eventually quit, it's when it reaches the outermost HTML element (there are iframes on my page hence the "outermost").

Further, it may prove that this only occurs where the breadcrumbs require "<" and ">" arrows (ie there is more data than can be shown in the available space).  IOW, you may need to shrink your tools window to see this bug.


Expected results:

It should work like the docs say: "HTML breadcrumbs: this shows the complete hierarchy through the document for the branch containing the selected element."

It shouldn't move in response to user moving the mouse or the HTML pane choosing to scroll doc changes into view.

Comment 1

[Girish Sharma [:Optimizer]]

Patric, I have also seen this. This might not be because of the remote highlighter, but it shows up more often because of that. Any idea ?

Comment 2

[Patrick Brosset <:pbro>]

I have seen this too, but I can't replicate that problem on the google homepage though, there doesn't seem to be much mutations going on in that page and the breadcrumbs widget is stable.
I've seen it though on a page like www.mozilla.org, if you select the <section.pillars> node and wait, you should see the widget flicker very rapidly. It does so everytime the "in the news" red section at the bottom animations, even though the its not included inside that node.

I just noticed that the breadcrumbs panel in the inspector listens for markup mutations and doesn't seem to do any sort of filtering. See http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/breadcrumbs.js#119
On a page that either has js-driven animations or rapid DOM mutations, this is very frustrating as the breadcrumbs will constantly re-adjust, and that may cause scrolling as the widget is part of a <arrowscrollbox> element.

There may be several ways to stabilize the widget, but the first thing I'd do is implement a filtering on markup-mutation to only update the widget if the mutation concerns one of the crumbs, and only if it results in a different crumb output.

Comment 3

[Codacoder]

> I have seen this too, but I can't replicate that problem on the google homepage though, there doesn't seem to be much mutations going on in that page and the breadcrumbs widget is stable.

I chose the Feeling lucky button because on mouseover, it animates.  Again, try floating the tools, and shrinking the window.

> ... and only if it results in a different crumb output.

This part I'd agree with.  If this feature is of any use, it should display the selected element (as indeed akin to the old stack display).  But its a tricky thing to get right.  May end up with a setting that controls this possibility...

  [] Update breadcrumbs with live html changes?

Comment 4

[Brian Grinstead [:bgrins]]

> There may be several ways to stabilize the widget, but the first thing I'd
> do is implement a filtering on markup-mutation to only update the widget if
> the mutation concerns one of the crumbs, and only if it results in a
> different crumb output.

Yes, we should only be touching the crumbs if the mutation changes the output of the crumbs.  The fix as described may be pretty easy, but we should also be aware that Bug 822388 is opened for migrating the breadcrumbs used in the inspector over to BreadcrumbsWidget.  If it looks like this fix would not survive that migration we should consider tackling that bug first.  Though it looks like we will still need a wrapper object for the BreadcrumbsWidget to handle inspector related stuff, so a fix here may be easy enough to migrate.

Comment 5

[Codacoder]

Over a month and no update for this? Can someone please turn this "feature" off?  I'm trying to use the new box-model highlighter (great feature!) which sends the crumbs into a fit.

Comment 6

[Patrick Brosset <:pbro>]

New STR:
- Open https://bgrins.github.io/devtools-demos/inspector/jquery-animation.html
- Open the inspector (<body> should be selected by default)
- Click on the replay button in the page if the animation has already ended
=> The breadcrumbs widget in the inspector constantly toggles between showing the child <header> element and not showing it.

Comment 7

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: bz://966787/pbrosset

/r/6707 - Bug 966787 - 1 - Code cleanup in breadcrumbs.js
/r/6709 - Bug 966787 - 2 - Skip inspector breadcrumb updates when the output doesn't change
/r/6711 - Bug 966787 - 3 - Skip inspector breadcrumb updates for some markupmutations

Pull down these commits:

hg pull -r b99f8be2a117927542aabd292aaee14019a33183 https://reviewboard-hg.mozilla.org/gecko/

Comment 8

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/6707/#review5627

Nice cleanup for pt 1!

Comment 9

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/6709/#review5629

Pt 2 looks good

::: browser/devtools/inspector/breadcrumbs.js
(Diff revision 1)
> +      let textOutput = this.prettyPrintNodeAsText(node);

I wish prettyPrintNodeAsText and prettyPrintNodeAsXUL shared more bits of code so we could always be sure that if the textOutput hasn't changed the XUL output is guaranteed to have not changed also, but I don't think it should block anything here

::: browser/devtools/inspector/breadcrumbs.js
(Diff revision 1)
> +          currentOutput: this.prettyPrintNodeAsText(node)

Nit: currentOutput would be more clearly named as currentNodeText or currentPrettyPrintedText

Comment 10

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/6711/#review5631

Please add a test for the breadcrumbs focused on mutations (unfortunately the existing test only deals with selection changes).  This will let us codify cases where we do and don't want the crumbs to change.

From my testing, there is one case this misses.  If you select the last element in the chain like querySelector("header a") on the demo page, then open split console and run $0.innerHTML = "<b>1</b>" it will not realize that there should be a new element added at the end of the crumbs.  This could be handled by:

  // This is the last element in the list and it has had children change  
  if (type === "childList" && this.indexOf(target) === this.nodeHierarchy.length - 1) {
     return true;
   }

::: browser/devtools/inspector/breadcrumbs.js
(Diff revision 1)
> +      if (reason === "markupmutation" && mutations.length) {

No need to check mutations.length here, the loop just won't happen and false will be returned if for some reason that array was empty

::: browser/devtools/inspector/breadcrumbs.js
(Diff revision 1)
> +          if (type === "childList" && (added.length || removed.length)) {

Same comment about added.length / removed.length.. I think empty arrays will sort themselves out in the condition below

::: browser/devtools/inspector/breadcrumbs.js
(Diff revision 1)
> +    let hasInterestingMutations = (() => {

This would be more clear as a separate function on the prototype instead of nested here

Comment 11

[Brian Grinstead [:bgrins]]

Here is a try push - looks like there may be some orange coming in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2be822f674fa

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8589625 [details]
MozReview Request: bz://966787/pbrosset

Clearing reviews

Comment 13

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> Here is a try push - looks like there may be some orange coming in:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2be822f674fa
I had pushed to try too: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f0073647035
I knew this was going to come up, because a lot of unrelated inspector tests have steps where they wait for the inspector to update (inspector-updated event) to avoid exceptions in the logs when the test ends. Now that the breadcrumbs updates far less often, many of these steps need to be removed.

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8589625 [details]
MozReview Request: bz://966787/pbrosset

/r/6707 - Bug 966787 - 1 - Code cleanup in breadcrumbs.js; r=bgrins
/r/6709 - Bug 966787 - 2 - Skip inspector breadcrumb updates when the output doesn't change; r=bgrins
/r/6711 - Bug 966787 - 3 - Skip inspector breadcrumb updates for some markupmutations; r=bgrins
/r/6779 - Bug 966787 - 4 - Test fixes; r=bgrins

Pull down these commits:

hg pull -r c5e71ed867adb358c466853d94217d4a2760594c https://reviewboard-hg.mozilla.org/gecko/

Comment 15

[Patrick Brosset <:pbro>]

I have made the test fixes described in comment 13 in a separate commit to ease review, but I will fold parts 2, 3 and 4 before landing, to be bisect-safe.
New pending try build : https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce8a7d597041

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8589625 [details]
MozReview Request: bz://966787/pbrosset

https://reviewboard.mozilla.org/r/6705/#review5671

Great!

Comment 17

[Patrick Brosset <:pbro>]

The last try build had failures too. The new test I had written passed fine locally, but failed on try. I found the root cause: the test was using a MutationObserver to detect whether or not the DOM in the breadcrumbs widget changed when changes were made to the test page. But because screen size varies on TRY meant that on some VMs, the widget was forced to sometimes overflow and that caused mutations.
I have now filtered these mutations out.

Comment 18

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bb7ca6c6de4

Comment 19

[Patrick Brosset <:pbro>]

Better now, but browser/devtools/styleinspector/test/browser_ruleview_add-property-cancel_03.js permafails with e10s. It shouldn't be too bad, I'll take a look a this very soon.

Comment 20

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8c325f0034c
This last try push should be ok now. I've fixed the last remaining problem.

Comment 21

[Patrick Brosset <:pbro>]

remote:   https://hg.mozilla.org/integration/fx-team/rev/bc90a74a549b
remote:   https://hg.mozilla.org/integration/fx-team/rev/5ac6a0ec6274

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bc90a74a549b
https://hg.mozilla.org/mozilla-central/rev/5ac6a0ec6274

Comment 25

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 966787 - 1 - Code cleanup in breadcrumbs.js; r=bgrins

Comment 26

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 966787 - 4 - Test fixes; r=bgrins

Comment 27

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 966787 - 2 - Skip inspector breadcrumb updates when the output doesn't change; r=bgrins

Comment 28

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 966787 - 3 - Skip inspector breadcrumb updates for some markupmutations; r=bgrins