Closed Bug 566085 Opened 14 years ago Closed 12 years ago

Highlighter is oblivious to DOM changes

Categories

(DevTools :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 566092

People

(Reporter: ehsan.akhgari, Assigned: msucan)

References

Details

(Whiteboard: [minotaur])

Attachments

(1 file, 7 obsolete files)

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.
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [kd4b4]
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.
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.
Assignee: nobody → jviereck
Assignee: jviereck → mihai.sucan
Attached file 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?
that is correct, sir, on all counts. Proceed!
Will this also simultaneously fix bug 566092?
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!
Status: NEW → ASSIGNED
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!
Attachment #463517 - Attachment is patch: false
Attachment #463517 - Attachment mime type: text/plain → text/html
Attached patch patch + test code (obsolete) — Splinter Review
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!
Attachment #464957 - Flags: feedback?(rcampbell)
Attached patch updated patch + test code (obsolete) — Splinter Review
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.
Attachment #464957 - Attachment is obsolete: true
Attachment #465217 - Flags: feedback?(rcampbell)
Attachment #464957 - Flags: feedback?(rcampbell)
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.
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 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)
Attachment #465217 - Flags: feedback?(rcampbell) → feedback+
Whiteboard: [kd4b4] → [kd4b5]
blocking2.0: --- → ?
(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+!
Attached patch updated patch (obsolete) — Splinter Review
Updated patch based on feedback. I added function names to the findNextSibling and findNodeAttrBox methods.
Attachment #465217 - Attachment is obsolete: true
Depends on: 572038
Attachment #465727 - Flags: review?(gavin.sharp)
Blocking+. This is a must-have feature if we're shipping the inspector.
blocking2.0: ? → betaN+
Whiteboard: [kd4b5] → [kd4b6]
Blocks: 592320
Severity: normal → blocker
Inspector feature postponed. Removing blocking flags from Inspector bugs.
blocking2.0: betaN+ → ---
Removing items from kd4b6.
Whiteboard: [kd4b6]
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Attached patch rebased patch (obsolete) — Splinter Review
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).
Attachment #465727 - Attachment is obsolete: true
Attachment #475897 - Flags: review?(gavin.sharp)
Attachment #465727 - Flags: review?(gavin.sharp)
Whiteboard: [patchclean:0916]
Blocks: 597061
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.
Attachment #475897 - Flags: review?(gavin.sharp)
(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.
I don't think so.
(In reply to comment #22)
> I don't think so.

Then I'll just have to rebase the patch. :)
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.
Attached patch rebase to fxteam (obsolete) — Splinter Review
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!
Attachment #475897 - Attachment is obsolete: true
Attachment #545221 - Flags: review?(rcampbell)
Whiteboard: [patchclean:0916]
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.
will this bug fix bug 665421 ?
(In reply to comment #27)
> will this bug fix bug 665421 ?

That one is handled by bug 566084.
(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.
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.
Agree with Gavin, we can reconsider if there's still no solution available when this becomes more pressing.
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 on attachment 545221 [details] [diff] [review]
rebase to fxteam

Yeah, we can cancel review request for now.
Attachment #545221 - Flags: review?(rcampbell)
Whiteboard: [minotaur]
Depends on: 641821
Priority: -- → P2
Whiteboard: [minotaur] → [minotaur][has-patch]
Summary: Inspect is oblivious to DOM changes → Highlighter is oblivious to DOM changes
Attached patch another rebase (obsolete) — Splinter Review
Another patch rebase.
Attachment #545221 - Attachment is obsolete: true
Blocks: 672003
aaq
aaq
rohit: please don't use actual bugs for testing. Use landfill.bugzilla.org.
Attached patch rebased patch (obsolete) — Splinter Review
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/mihai.sucan@gmail.com-60916cd5d720
Attachment #556578 - Attachment is obsolete: true
Attachment #570832 - Flags: review?(rcampbell)
(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.
err, s/browser window/content window/ (since this is where you're adding the event listener)
Blocks: 663830
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.
Attachment #570832 - Flags: review?(rcampbell)
No longer blocks: 592320
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Resolution: FIXED → DUPLICATE
Whiteboard: [minotaur][has-patch] → [minotaur]
Attachment #570832 - Attachment is obsolete: true
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: