566085 - Highlighter is oblivious to DOM changes

Status: RESOLVED DUPLICATE of bug 566092

(DevTools :: General, defect, P2)

Description

[(no longer active)]

STR:

1. Go to any bug.
2. Tools->Inspect.
3. Select the link displaying the name of the assignee of the bug.
4. Click the edit link next to it.

This causes the DOM to change dynamically.  Inspect still highlights the old span element, which no longer exists in DOM, and doesn't show the new changes in DOM.  No amount of expanding and collapsing the rows in the tree fixes this situation.

Comment 1

[Rob Campbell [:rc] (:robcee)]

I wonder if we want to haul in full DOM listening into the inspector? We could make it an option (with a checkbox) as there is some performance impact with these installed on a webpage. Alternatively, we could provide a "refresh" button in the toolbar to update the contents of the tree on demand.

Comment 2

[Kevin Dangoor]

The listener can be added/removed as the Inspector is opened/closed, right? So there would only be a potential performance impact when the Inspector is open. Obviously, the ideal is that everything "just works" without user intervention, so it seems worthwhile to head down that route first and see if we can make it work reasonably.

Comment 3

[Mihai Sucan [:msucan]]

Attachment: test case

I began working on this bug with writing a test case, first.

I found, to my surprise, that the current tree panel does update itself dynamically. WFM. Please see this test case.

The new tree panel that is in the works (see bug 572038) is indeed oblivious to changes.

I believe I should write a patch to be applied on top of the patches Robert has in bug 572038. Is this correct?

Comment 4

[Rob Campbell [:rc] (:robcee)]

that is correct, sir, on all counts. Proceed!

Comment 5

[Kevin Dangoor]

Will this also simultaneously fix bug 566092?

Comment 6

[Mihai Sucan [:msucan]]

I believe that bug will need more work. Definitely this bug fix will be of help for that.

The problem at hand is the tree panel automatic updating of structure based on DOM changes. The other panels will be notified that something changes / they'll be asked for a refresh of some sorts, and that will be something to do for the highlighter. Hopefully that will fix the issue.

Thanks for pointing out that bug!

Comment 7

[Mihai Sucan [:msucan]]

Robert:

I am now working on the handling of the cases of when the page is still loading, or when a new iframe/frame is added/removed. I have some thoughts (and questions):

- nsIWebProgressListener does not notify us when an iframe is removed from the 
parent document (with removeChild() or such). Thus, we need to use an iframeWindow.onunload event handler and/or a parentDocument.DOMNodeRemoved 
listener for this. (note that DOMNodeRemoved mutation events are fired *before* the actual remove occurs)

- We need to listen for the DOMContentLoaded/load event for each loading iframe.
We can use an nsIWebProgressListener but we'll end up with a hybrid of unload event handling and nsIWebProgressListeners... not nice, but I am not sure if we
can avoid it. Thoughts?

- If we put only iframeWindow.onload event handlers and remove them immediately after they are executed (to avoid memleaks), we don't catch subsequent loads in
the iframes. So, we either use an nsIWebProgressListener or we keep onload event handlers for later. Thoughts?

- The tree panel structure for the content of iframes become stale once the 
user navigates away in the iframe. We may want to prompt the user about navigating away? (Like we do for the main window in bug 566084). That would seem like overkill. Or do we simply update the structure and attach to the newly loaded page?

My thoughts: I believe we cannot get away without using window.onunload for each window (and iframe/frame), so we use this event. For loading, we should go for the nsIWebProgressListener, because this gives us the ability to track frame navigation. Also, I believe that the inspector tree panel should be live - no user notification.

If we *really* want to avoid window.onunload, we can rely only on DOMNodeRemoved and remove the event listeners from every iframe/frame that is removed. I think this will be kind of shoddy/error prone.

Please let me know how you want me to go further. Thanks!

Comment 8

[Mihai Sucan [:msucan]]

Attachment: patch + test code

This is the patch that fixes the issue. To apply the patch cleanly please do:

- clean default branch from mozilla-central.
- apply attachment 464784 [details] [diff] [review] from bug 573102, then attachment 463629 [details] [diff] [review] from bug 561782, then attachment 463349 [details] [diff] [review] and attachment 463325 [details] [diff] [review] from bug 572038. Finally, apply this patch.

I used a progress listener for tracking navigation inside iframes. The DOMNodeRemoved mutation event is used for removing the events when iframes are removed from the parent document.

Any comments and suggestions for improvements are welcome!

Comment 9

[Mihai Sucan [:msucan]]

Attachment: updated patch + test code

Updated patch. Yesterday I forgot to clean mutation listeners from the progress listener when the user navigates away in iframes. I also made some fixes to the test code.

Comment 10

[Mihai Sucan [:msucan]]

Regarding bug 566092: the highlighter fails to show properly on Linux and I was not able to test this entirely.

The test code in the above patch highlights an element (see panelRefreshTest()), then it changes the class to apply a style which changes the size of the element. The code then checks if the highlighter properly changed its size after the DOM mutation event. The test passes, but I have not seen this working in user testing, because the highlighter has display issues. Thus, I cannot confirm if bug 566092 is fixed by this patch. Please test. I expect the bug is fixed, because the automated test code does something quite similar to the bug description.

Comment 11

[Rob Campbell [:rc] (:robcee)]

I think overall this looks really solid.

Some questions:

https://bugzilla.mozilla.org/attachment.cgi?id=465217&action=diff#a/browser/base/content/domplate.jsm_sec2

why the removal of SourceText check above?

domplate.DIV({"class": "docType"},

why the removal of "$object" from that template?

insertChildBoxBefore:
removeChildBox:

I see you've re-added some of the API we removed during a previous sweep. I guess we know why it was there now. ;)

in https://bugzilla.mozilla.org/attachment.cgi?id=465217&action=diff#a/browser/base/content/inspector.js_sec8

we had an iterateWindows method before and it was removed because it was deemed somewhat risky. It doesn't appear to be used anywhere?

findNextSibling and findNodeAttrBox don't have named functions.

The progress listener looks fine.

And the test code looks pretty fine, on a quick glance. Thanks for the patch!

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 465217 [details] [diff] [review]
updated patch + test code

with decent answers to the above and the couple of minor cleanups (missing function names, mostly)

Comment 13

[Mihai Sucan [:msucan]]

(In reply to comment #11)
> I think overall this looks really solid.
> 
> Some questions:
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=465217&action=diff#a/browser/base/content/domplate.jsm_sec2
> 
> why the removal of SourceText check above?

DOM.SourceText is undefined. It did throw when I worked on that.


> domplate.DIV({"class": "docType"},
> 
> why the removal of "$object" from that template?

I reported this bug in the main tree panel bug 572038 and I think I included this fix in one of my patches (the test code patch).

If we put "class": "docType $object" we get something like this from domplate:

  <div class="docType [XPCNativeWrapper [object ...]]">&lt;!DOCTYPE html&gt;</div>

By removing "$object" we only output class="docType ". 

(I do have a nicely indented domplate source output laying around. I used it for the patch of this bug. It's really interesting to study the output of the domplate-based tree panel.)


> insertChildBoxBefore:
> removeChildBox:
> 
> I see you've re-added some of the API we removed during a previous sweep. I
> guess we know why it was there now. ;)

Hehe. "back to the roots!" :)


> in
> https://bugzilla.mozilla.org/attachment.cgi?id=465217&action=diff#a/browser/base/content/inspector.js_sec8
> 
> we had an iterateWindows method before and it was removed because it was deemed
> somewhat risky. It doesn't appear to be used anywhere?

We had ... in Firebug or in Inspector?

I use the iterateWindows method in attachMutationListeners and detachMutationListeners. This allows me to easily/quickly add/remove the mutation listeners from nested iframes (eg. when a DOMNodeRemoved event is fired).

If there's a problem with this, please let me know.


> findNextSibling and findNodeAttrBox don't have named functions.

Oh, I forgot to fix that. Will fix now.

> The progress listener looks fine.
> 
> And the test code looks pretty fine, on a quick glance. Thanks for the patch!

You're welcome! Thanks for the feedback+!

Comment 14

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch based on feedback. I added function names to the findNextSibling and findNodeAttrBox methods.

Comment 15

[Dietrich Ayala (:dietrich)]

Blocking+. This is a must-have feature if we're shipping the inspector.

Comment 16

[Kevin Dangoor]

Inspector feature postponed. Removing blocking flags from Inspector bugs.

Comment 17

[Kevin Dangoor]

Removing items from kd4b6.

Comment 18

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 19

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch on top of the latest mozilla-central default branch code. Nothing else changed, except for updates related to the changes in m-c (nothing big).

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 475897 [details] [diff] [review]
rebased patch

This was bitrotten by bug 653528. I kind of wish we could avoid going the mutation event listener route, since they have such bad side effects.

Comment 21

[Mihai Sucan [:msucan]]

(In reply to comment #20)
> Comment on attachment 475897 [details] [diff] [review] [review]
> rebased patch
> 
> This was bitrotten by bug 653528. I kind of wish we could avoid going the
> mutation event listener route, since they have such bad side effects.

Are there different ways to keep up with DOM changes I could use? Something with better performance.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

I don't think so.

Comment 23

[Mihai Sucan [:msucan]]

(In reply to comment #22)
> I don't think so.

Then I'll just have to rebase the patch. :)

Comment 24

[Mihai Sucan [:msucan]]

We need to investigate if the new API from bug 641821 would be sufficient for our needs here. If yes, then that API would give us a nice perf boost - DOM mutation events are much slower.

Comment 25

[Mihai Sucan [:msucan]]

Attachment: rebase to fxteam

Rebased the patch on top of the latest fxteam repo.

We cannot yet use the API from bug 641821 - there are still some planned changes over there. Once that lands we can make a follow up patch - should be simple to migrate. That will give us a really nice perf boost.

Looking forward to your review. Thank you!

Comment 26

[:Gavin Sharp [email: [e-mail]]]

I don't think we should fix this until we can use bug 641821 - we don't need to rush a fix for this, IMO.

Comment 27

[Paul Rouget [:paul]]

will this bug fix bug 665421 ?

Comment 28

[Mihai Sucan [:msucan]]

(In reply to comment #27)
> will this bug fix bug 665421 ?

That one is handled by bug 566084.

Comment 29

[Mihai Sucan [:msucan]]

(In reply to comment #26)
> I don't think we should fix this until we can use bug 641821 - we don't need
> to rush a fix for this, IMO.

I expect that bug 641821 is going to take quite some more time, depending on the specs, the talks there, the API decisions and lack of decisions. It started in March. I am fine with waiting, but the Inspector tool will lack an important bit of polish.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Things seem to be moving at a decent pace, I don't expect it to take too much time. This bug has been around for a while too, so while it would obviously be nice to fix, I don't think we need to spend time on a solution we know we're going to want to replace.

Comment 31

[Dave Camp (:dcamp)]

Agree with Gavin, we can reconsider if there's still no solution available when this becomes more pressing.

Comment 32

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 545221 [details] [diff] [review]
rebase to fxteam

I thought we didn't want to do this until the new API was ready?

Comment 33

[Dave Camp (:dcamp)]

Comment on attachment 545221 [details] [diff] [review]
rebase to fxteam

Yeah, we can cancel review request for now.

Comment 34

[Mihai Sucan [:msucan]]

Attachment: another rebase

Another patch rebase.

Comment 35

[rohit]

aaq

Comment 36

[rohit]

aaq

Comment 37

[Rob Campbell [:rc] (:robcee)]

rohit: please don't use actual bugs for testing. Use landfill.bugzilla.org.

Comment 38

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch.

As agreed with dcamp we are aiming to land this patch rebased, maybe in time for Firefox 10 aurora merge (if you have time to review it!). We'll switch to the new mutations API when it will be available. We are putting this feature behind a pref - default to false.

Added an option to the register tools API to allow a mutation event handler. Added mutation event handling mechanisms to the Style Inspector and to the HTML Tree Panel.

Please let me know if further changes are needed. Thank you!


I pushed the patch to try:
  https://tbpl.mozilla.org/?tree=Try&rev=60916cd5d720
Builds:
  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-60916cd5d720

Comment 39

[Dão Gottwald [:dao]]

(In reply to Rob Campbell [:rc] (robcee) from comment #1)
> I wonder if we want to haul in full DOM listening into the inspector? We
> could make it an option (with a checkbox) as there is some performance
> impact with these installed on a webpage.

(In reply to Kevin Dangoor from comment #2)
> The listener can be added/removed as the Inspector is opened/closed, right?
> So there would only be a potential performance impact when the Inspector is
> open.

You misunderstood. Just adding a mutation event listeners once will slow down all DOM mutations in that browser window for the rest of its life. This is unacceptable and there's no point in landing it pref'd off, as we'll never want to flip the pref.

Comment 40

[Dão Gottwald [:dao]]

err, s/browser window/content window/ (since this is where you're adding the event listener)

Comment 41

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 570832 [details] [diff] [review]
rebased patch

we're not likely to use this approach, so cancelling this review request. Feel free to re-ask if this becomes important again.

Comment 42

[Kevin Dangoor]

Based on the original STR, this bug is now fixed. When I inspect the assignee's name, and click Edit, the highlight is no longer displayed and the "infobar" goes to the top of the window because the original span is no longer visible.

As I understand it, the solution to the problem reported here fixes the highlighter but does not allow other tools to update based on DOM mutations. Still, it seems to me that the problem tracked in this bug is fixed. I'm sure someone will reopen it if I'm wrong.

See bug 566092.