Closed Bug 368608 Opened 18 years ago Closed 10 years ago

Rewrite DOM Inspector's flasher (blink selected element broken)

Categories

(Other Applications :: DOM Inspector, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stanshebs, Assigned: neil)

References

Details

Attachments

(3 files, 1 obsolete file)

The flasher (or "blink") code in the DOM inspector, that it uses when an element is first selected, is wrong for Thebes; it tries to grab a graphic context and draw lines on it. While works for Linux (and Windows I think), it doesn't work at all on Macs, and can even crash the browser (362201 etc). There are a couple options. One simple option is simply to do the flashing by temporarily replacing the element's style with a red outline, then restoring it. While this is simple, and seems to work OK in practice, it's not great for a data structure inspector to alter the structure while it's being inspected; there are side effects, not least of which is that the textual display of the style changes while the blinking is happening. Even so, it only takes a few lines of JavaScript. The other option is to rewrite the inFlasher.cpp code to do things in a properly Thebes-ish way. This could be done by adding some kind of a switchable drawing layer that superimposes the red outline, presumably moving the drawing into generic widget rendering. More complicated, but cleaner in the long run, and more flexible.
Assignee: stanshebs → dom-inspector
Component: General → DOM Inspector
Product: Firefox → Other Applications
QA Contact: general → timeless
Target Milestone: Firefox 3 → ---
Assignee: dom-inspector → stanshebs
Having a global overlay layer that can be used by extensions as well would be useful; roc suggested that we might want to place the entire browser chrome in a <stack> such that temporary elements can be placed on top in this way. Note that this would require fixing bug 130078 to work.
Blocks: 361925
i wouldn't mind an overlay layer. i would object to a style change. inspector should not result in a cat dying. the object being observed should not notice it's being observed. unless you explicitly choose to pet it (using the apply :hover features). note that i'd rather have the drawing option, because not all of us have an easy way to deal with these layers you're imagining.
Blocks: 385908
I should clarify that point. I'm currently playing with inIFlasher in an embedding context, we really don't have any xul, so stacks and similar aren't an option. note that we're using the flasher to draw solid non flashing boxes more often than we flash them. on a linux workstation in my office there seems to be some case where the painting doesn't last (something seems to send a repaint message and the drawn box goes away), but it's not that simple since I was able to convince some boxes sometimes to persist.
Flags: blocking1.9?
Target Milestone: --- → mozilla1.9beta
QA Contact: timeless → dom-inspector
I came in here to nominate this, because not having the flasher working for DOMI or Firebug is like typing with oven mitts on when it comes to trying to get a CSS layout to play nicely. It's already nominated, I see, but I'm adding my enthusiastic support nonetheless!
So we now have translucent popup window support on all platforms; I would say that we should use that, and just position that window in the right spot (which we should already be able to figure out). This will also fix all sorts of problems regarding trying to flash elements that are obscured by native widgets and similar issues.
Blocking for after M9
Flags: blocking1.9? → blocking1.9+
Target Milestone: mozilla1.9 M9 → ---
I waffled between P2 and P3 for priority, P2 seems like it would be most appropriate if this affected all platforms, not just Mac.
Priority: -- → P3
This isn't going to get cycles from me in the foreseeable future, so de-assigning.
Assignee: stanshebs → nobody
More details here: we should rewrite the flasher by entirely removing any nsIWidget flashing ability, and having DOMI use a translucent popup to outline the area it wants to outline. This would be significantly cleaner than any other solution. (It would also let it identify elements that are outside of the current window's bounds, which would be kind of neat!)
Note: this means that the only changes needed would be in JS code (to show and position the window)
(In reply to comment #10) > Note: this means that the only changes needed would be in JS code (to show and > position the window) Well - that's not entirely true since we'd be replacing the existing component. I also know that other add-ons out there depend on inIFlasher (and use it in odd ways). I do like the idea of using a transparent panel for this as well. However, does transparency work on all platforms? I was under the impression that it doesn't work on OS X still...
Blocks: 405318
Will this be fixed before ff3 release?
(In reply to comment #9) > More details here: we should rewrite the flasher by entirely removing any > nsIWidget flashing ability, and having DOMI use a translucent popup to outline > the area it wants to outline. This would be significantly cleaner than any > other solution. (It would also let it identify elements that are outside of > the current window's bounds, which would be kind of neat!) > I'd like to get a better highlight for XUL than the one used by Firebug (which does not work will for XUL). Can someone explain what "translucent popup" means? Thanks, John.
(In reply to comment #14) > the XUL panel element. See http://developer.mozilla.org/en/docs/XUL:panel Thanks, but as far as I can tell this is FF3 only. The panel has no showPopup and FF2 has no openPopup. I tried using tooltip which does have showPopup but nothing happens. I'll look into this again when FF3 works.
No longer blocks: 405318
Blocks: 373580
Flags: tracking1.9+
ok, so that i'm unuseful to solve this bug, i'll try to motivate the importance to a have a translucent-popup-red-outlined working properly. Please help me, i tried nightly build, i have both ff2 and ff3 installed (in ff2 works fine for me) but i don't like to downgrade to ff2 again as i was saying in this duplicate 459048 i have problem with all ff3 release on osx 10.4 and 10.5
No longer blocks: 361925
FYI. bug 532355 adds an ability to use flasher for accessibleTree view. However it's not correct to blink DOM node of the accessible object when we want to flash the accessible object because different accessible objects might have the same DOM node. Therefore the flasher should have an ability to work with accessible objects directly (like flashAccessible(nsIAccessible *aAccessible) or blink a rectangle (like flashRect). Also that's a reason styles approach won't work.
Summary: Rewrite DOM Inspector's flasher → Rewrite DOM Inspector's flasher (blink selected element broken on Mac)
I just found this. Still a bug on the Mac. Going to try Vlad's suggestion of using a panel in JS-land. Ultimately, I think we're going to want something a little slicker. The content overlay seems like the best way to do this.
One thing I noticed while debugging: the call to frame->GetSize() is returning coordinates that are out-to-lunch. (mRect.width = 65k, mRect.height = 45k). Probably just a symptom but it seems like a bug in nsIFrame.
Note that these numbers aren't pixels, they're app units, so usually 60 times the number you expect.
ah, good to know. Still out-of-bounds though.
Looks like hardware acceleration on windows has broken us there as well.
Summary: Rewrite DOM Inspector's flasher (blink selected element broken on Mac) → Rewrite DOM Inspector's flasher (blink selected element broken)
Attached file WIP JS component (deleted) —
WIP to get the ball rolling, because not having this is annoying me now that I'm actually working on things using m-c. This isn't a patch, just the JS component - things I need to get stuff patch-like: - DOMi build instructions: does that require being built as part of a Mozilla tree? (that will hurt, I'm on a Windows laptop and builds are measured in hours) - Testing infrastructure: Any ideas on how I can write tests for this? I'm guessing mochichrome... Known problems: - Not tested on multi-monitor configurations - Requires a XUL window. (Probably won't work on Camino) I wanted to use the hidden window, but creating XUL elements there (about:blank) is no longer allowed. - Bad behaviour when the rectangle needs to go near a screen edge, because <panel> doesn't like to do so (it's original intent seems to be more like popups). The Firefox 4 devtools inspector has this problem as well.
(In reply to comment #32) ZOMG <3 > - DOMi build instructions: does that require being built as part of a Mozilla > tree? (that will hurt, I'm on a Windows laptop and builds are measured in > hours) Sadly, yeah, it assumes it is being built as part of m-c. You might be able to just run configure to get the makefiles, and then just build in the extension directory, but I've never tried that. Also, pymake makes building on windows not painful at all (you can use -j!). > - Testing infrastructure: Any ideas on how I can write tests for this? I'm > guessing mochichrome... mochichrome, however, I don't think we have any exists tests that you can build off of (sorry) > - Not tested on multi-monitor configurations Follow-up > - Requires a XUL window. (Probably won't work on Camino) I wanted to use the > hidden window, but creating XUL elements there (about:blank) is no longer > allowed. Follow-up (not tier 1) > - Bad behaviour when the rectangle needs to go near a screen edge, because > <panel> doesn't like to do so (it's original intent seems to be more like > popups). The Firefox 4 devtools inspector has this problem as well. Follow-up Again, <3
(In reply to comment #32) > - DOMi build instructions: does that require being built as part of a Mozilla > tree? (that will hurt, I'm on a Windows laptop and builds are measured in > hours) See bug 469787, comment 2. With the right build environment, I can do a build in seconds. Additionally, I have written a shell script to swap out the relevant pieces in a development profile in a minimal XULRunner app. With a disabled XUL cache and unjarred chrome, I can clone from hg, configure, build, and swap out in literally less than a minute. Most of that time is spent in hg clone and configure, which you aren't going to be doing everytime, obviously. It really helps for fast iterations.
hey mook. nice! does the transparent attribute on the panels work on linux and still get you a border? I tried using transparency via an opacity style rule when working on the firefox inspector, but that was a no-go there. There are definitely some positioning issues when moving the parent widget (in most cases, the <browser> you're overlaying) partially off-screen. The coordinates on the panel go wonky. Should file a xul widget bug. :)
(In reply to comment #35) > does the transparent attribute on the panels work on linux and still get you a > border? I tried using transparency via an opacity style rule when working on > the firefox inspector, but that was a no-go there. Hmm, I'll need to double check (I tested on Window since that's my primary platform, and a quick test on OSX since that was specifically mentioned in comment 0). > There are definitely some positioning issues when moving the parent widget (in > most cases, the <browser> you're overlaying) partially off-screen. The > coordinates on the panel go wonky. Should file a xul widget bug. That's by design, according to https://developer.mozilla.org/en/XUL/panel#m-openPopupAtScreen ("This position may be adjusted if it would cause the popup to be off of the screen ").
(In reply to comment #36) > (In reply to comment #35) [snip] > > There are definitely some positioning issues when moving the parent widget (in > > most cases, the <browser> you're overlaying) partially off-screen. The > > coordinates on the panel go wonky. Should file a xul widget bug. > That's by design, according to > https://developer.mozilla.org/en/XUL/panel#m-openPopupAtScreen ("This position > may be adjusted if it would cause the popup to be off of the screen "). yeah, I know. I think better behavior (for our purposes) would be to resize the panel or clip it at the screen edge. in any case, probably follow-up-worthy.
What's the advantage of making the overlay a panel vs. making it a normal XUL box? For example, inserting a <box> into the Firefox chrome XUL with style="position:fixed;z-index:1000;pointer-events:none;left:0;top:0;right:0;bottom:0;background:rgba(255,255,0,0.3);" seems to work fine.
(In reply to comment #38) > What's the advantage of making the overlay a panel vs. … > inserting a <box> into the Firefox chrome XUL 1. Observer effects. 2. That doesn't work for HTML. 3. That requires special knowledge of the inspected document. DOM Inspector supports inspecting more than just browser.xul, and that's not a general solution. Having said that, #3 entails that the panel would have to reside in DOM Inspector's own document and be manipulated to give the desired effect. Is that even possible? Is it possible to achieve the kind of introspection into the window manager's window stacking order and the manipulation required to have the panel appear immediately above the inspected document? That is, we need to i) ensure the panel appears above the inspected document, and ii) ensure the panel doesn't appear over the DOM Inspector if its window is occluding the inspected document, and iii) ensure that the panel's position in the window stack is not below any third-party windows which are below the inspected document, nor above any third-party windows which are above the inspected document.
The best thing in the long run would be to sweet talk/push hard for the ability to obtain a 2d graphics context for window/element areas and that would allow you to arbitrarily paint on them using canvas APIs. roc has mentioned something like this in the past (before there was HTML5 canvas). Unfortunately, everyone seems content to continue writing panel and iframe hacks.
Blocks: 677956
Note bug 1018324 (remove inIFlasher).
OK, so there are two things that could help us out here: 1. scrollIntoView was added to Element in Gecko 19 by bug 804950, which presumably could be used to replace scrollElementIntoView 2. :-moz-devtools-highlighted was added in Gecko 26 by bug 913440, which could be adapted to replace drawElementOutline
Attached patch Possible patch (deleted) — Splinter Review
This implements an alternative to the inFlasher object. The existing Flasher object can then use this object instead of the inFlasher object. Creation of the alternative object fails if the underlying Gecko doesn't support it, so this prefers the new object on Gecko 25a2 or later.
Attachment #8432150 - Flags: feedback?(philip.chee)
Attachment #8432150 - Flags: feedback?(iann_bugzilla)
Attachment #8432150 - Flags: feedback?(Sevenspade)
Attached patch Alternate approach (obsolete) (deleted) — Splinter Review
This duplicates the Flasher object and merges the duplicate with the Shell object from attachment 8432150 [details] [diff] [review] (this allows for some simplification although a small amount of duplication still remains).
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #8432276 - Flags: feedback?(philip.chee)
Attachment #8432276 - Flags: feedback?(iann_bugzilla)
Attachment #8432276 - Flags: feedback?(Sevenspade)
Aside from comment 40 and that bug 913440 was probably not a very good idea, I have no strong feelings about this.
Attachment #8432276 - Flags: feedback?(Sevenspade)
Attachment #8432150 - Flags: feedback?(Sevenspade)
(In reply to Colby Russell :crussell from comment #40) > The best thing in the long run would be to sweet talk/push hard for the > ability to obtain a 2d graphics context for window/element areas and that > would allow you to arbitrarily paint on them using canvas APIs. roc has > mentioned something like this in the past (before there was HTML5 canvas). "Just draw into the window" can't work in today's world, where we have compositing happening off the main thread. In fact it has never worked reliably, since highlights always got overwritten by any animations that triggered their own painting. The approach of stacking an element over the content window and putting highlights in it is currently the only way to make this work reliably with zero impact on the content document. That approach is currently used on desktop (good!). It can also be used in B2G and Android with a bit more work. In B2G the overlay would have to go into the system process, and in Android the overlay would have to be injected into browser.xul.
Attached patch Adjusted patch (deleted) — Splinter Review
I managed to get scrollElementIntoView moved to inIDOMUtils but now we have to detect it so that the code works on versions with the pseudoclass and the flasher (where we prefer the pseudoclass because it paints correctly) as well as versions without the flasher. I could also make this change to attachment 8432150 [details] [diff] [review] but I don't have that currently applied so I'll only do it on request.
Attachment #8432276 - Attachment is obsolete: true
Attachment #8432276 - Flags: feedback?(philip.chee)
Attachment #8432276 - Flags: feedback?(iann_bugzilla)
Attachment #8437549 - Flags: review?(philip.chee)
Attachment #8437549 - Flags: review?(iann_bugzilla)
<glandium> \o/ it works [inIFlasher crashes on GTK3 builds (bug 1022135) so I gave him an XPI of attachment 8437549 [details] [diff] [review] to test with.]
Blocks: 1023938
Comment on attachment 8437549 [details] [diff] [review] Adjusted patch This patch adds some trailing whitespace. r=me with the whitespace fixed.
Attachment #8437549 - Flags: review?(philip.chee) → review+
> Adjusted patch > + get thickness() { return 0 | this.mThickness; }, [ I have a slight preference for foo|0 asm.js does it this way ]
Comment on attachment 8432150 [details] [diff] [review] Possible patch I prefer the other approach.
Attachment #8432150 - Flags: feedback?(philip.chee)
Pushed dom-inspector changeset 42d670ba3bb3.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #8432150 - Flags: feedback?(iann_bugzilla)
Attachment #8437549 - Flags: review?(iann_bugzilla)
Blocks: DOMi2.0.15
(In reply to neil@parkwaycc.co.uk from comment #52) > Pushed dom-inspector changeset 42d670ba3bb3. Correct changeset is http://hg.mozilla.org/dom-inspector/rev/77a21a719c0f
No longer blocks: 1023938
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: