Closed Bug 642471 Opened 13 years ago Closed 13 years ago

Rewrite PanelHighlighter using transparent xul iframe and HTML

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 7

People

(Reporter: rcampbell, Assigned: rcampbell)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [best: 1d, likely: 3d, worst: 1w][highlighter][fixed-in-devtools])

Attachments

(1 file, 18 obsolete files)

158.81 KB, patch
Details | Diff | Splinter Review
The inspector uses a PanelHighlighter to highlight regions of a webpage. We want to replace this with a transparent xul iframe with a canvas element.

This is merely replicating the current functionality. Further enhancements and capabilities will be filed as follow-ups.
Whiteboard: [best: 1d, likely: 3d, worst: 1w][highlighter] → [best: 1d, likely: 3d, worst: 1w][highlighter][started-3/17]
Attached file highlighter-frame.js (obsolete) —
highlighter-frame.js. Workspace file with some prototype and test code.
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Blocks: 650794
Alias: highlighter
Attached patch highlighter-0.1 WIP (obsolete) — Splinter Review
saving work. Highlighter is "highlighting". Still need to write shutdown code, properly comment and clean up.
Depends on: 653528
Blocks: 653531
Blocks: 653545
Blocks: 653549
Blocks: 653550
Attached patch highlighter-0.2 WIP (obsolete) — Splinter Review
rebased on top of patch in bug 653528. Still trying to fix some unittests.
Attachment #526722 - Attachment is obsolete: true
Attachment #526987 - Attachment is obsolete: true
Attachment #529797 - Flags: feedback?(mihai.sucan)
Attached patch highlighter-0.2.1 WIP (obsolete) — Splinter Review
some errors from yesterday's rebased. Cleaned and updated.
Attachment #529797 - Attachment is obsolete: true
Attachment #529797 - Flags: feedback?(mihai.sucan)
Attachment #529988 - Flags: feedback?(mihai.sucan)
looks like we've got two failing unittests:

inspector_iframe.js and inspector_tab_switch.js.

iframe appears to be from this patch, tab_switch a result of stripping the dom and style panels.
Attached patch highlighter-0.2.2 WIP (obsolete) — Splinter Review
rebased on new strip style and dom patch.
Attachment #529988 - Attachment is obsolete: true
Attachment #529988 - Flags: feedback?(mihai.sucan)
Attached patch highlighter-0.2.3 WIP (obsolete) — Splinter Review
fixed some extra breakage.
Attachment #530049 - Attachment is obsolete: true
Attached patch highlighter-0.2.4 WIP (obsolete) — Splinter Review
Fixed tab switch and initialization tests.

There are still failures in the other tests (mainly highlighter stuff).
Attachment #530066 - Attachment is obsolete: true
Attached patch highlighter-0.2.5 WIP (obsolete) — Splinter Review
updated patch.

passes individual tests, but not in suite. iframes still broken, I think. See prev. patch for debugging statements, stripped from here.
Blocks: 654810
Attached patch highlighter-0.3 (obsolete) — Splinter Review
updated and rebased. All tests except the iframe test are passing. Still need to investigate that, but the base code is ready for review.
Attachment #530145 - Attachment is obsolete: true
Attachment #530313 - Attachment is obsolete: true
Attachment #532314 - Flags: review?(gavin.sharp)
note that I'm considering this already r+ from mihai since we did some joint hacking on this to clean up the initialization bits. Those results ended up back in the style and dom panel removal code.

Mihai, feel free to drop me some feedback if you have any.
Attached patch highlighter-0.3.1 (obsolete) — Splinter Review
rebased after updating remove style panel patch
Attachment #532314 - Attachment is obsolete: true
Attachment #532314 - Flags: review?(gavin.sharp)
Attachment #532452 - Flags: review?(gavin.sharp)
Whiteboard: [best: 1d, likely: 3d, worst: 1w][highlighter][started-3/17] → [best: 1d, likely: 3d, worst: 1w][highlighter][rebased-05-14]
Attached patch highlighter-0.4 (obsolete) — Splinter Review
incorporates close button, unittest restructuring and some HTML and CSS. All tests are passing, though I'm going to hold off on a review request until I get the tests fixed in the style and dom panel removal.
Attachment #532452 - Attachment is obsolete: true
Attachment #532452 - Flags: review?(gavin.sharp)
todo: remove preprocessor directive from the theme/highlighter.css files.
Blocks: 587134
Attached patch highlighter-0.4.1 (obsolete) — Splinter Review
updated patch. Incorporated review bits from (closed) bug 653550.
Attachment #534880 - Attachment is obsolete: true
Attachment #536607 - Flags: feedback?(mihai.sucan)
note that this also incorporates the changes from bug 654810. Now all unittests for the Inspector live under an inspector subdirectory in browser/base/content/test.

Run 'em with TEST_PATH=browser/base/content/test/inspector make mochitest-browser-chrome from your objdir.

Currently all tests passing locally.
Comment on attachment 536607 [details] [diff] [review]
highlighter-0.4.1

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

Usage comments:

- The highlighter covers the scrollbars which means users cannot scroll the page, unless they use the mouse scroll wheel.

- If I try to use the Up/Down arrow keys it also fails to scroll the page.

Usability is very much impaired by these two issues. Once you start the Inspector you cannot easily go to a different location in the page.

- If you try to highlight elements in iframes the highlighter is wrongly positioned. Really broken behavior. Try it in the new Yahoo Mail (beta), when reading a message, for example.

- You need to allow mouse and keyboard interaction with the web page when the Inspect option is not active. Currently the user is frozen in a state that requires him/her to close and reopen the Inspector whenever the user needs to interact with the page. For example, if I am playing with Gmail or Yahoo Mail I cannot open up emails from the list, until I close the Inspector, then I have to reopen to see how it works with the email view, and so on.

General comments:

- The new highlighter.css files need the MPL tri-license header, IIANM.
- The X (the cross) displayed in the iframe needs to be on the left when an RTL language is used.

Code is fine, I would give it f+, but the usability/accessibility issues are, I believe, important to fix before we can move forward. We need the keyboard and mouse usage figured out, we need to not cover the scrollbars, and, finally, we need to deal with iframes correctly - this is basic functionality that needs to work properly, and we already have a test that shows it does not.

::: browser/base/content/inspector.js
@@ +82,5 @@
> +    iframe.setAttribute("id", "highlighter-frame");
> +    iframe.setAttribute("transparent", "true");
> +    iframe.setAttribute("type", "content");
> +    iframe.addEventListener("DOMTitleChanged", function(event) {
> +      event.stopPropagation();

Why do you do this?

@@ +97,5 @@
> +      self.resizeCanvas();
> +      self.ready = true;
> +      let closeButton = self.iframeDoc.getElementById("close-button");
> +      closeButton.addEventListener("click", function IFH_closeClicked(evt) {
> +        evt.target.removeEventListener("click", IFH_closeClicked, false);

Nit: you could just do closeButton.removeEventListener().

@@ +100,5 @@
> +      closeButton.addEventListener("click", function IFH_closeClicked(evt) {
> +        evt.target.removeEventListener("click", IFH_closeClicked, false);
> +        InspectorUI.closeInspectorUI();
> +      }, false);
> +      Services.obs.notifyObservers(null, "highlighter-ready", null);

Nit: you could do self.ready = true; just before you send out the notification.

@@ +149,5 @@
> +  },
> +
> +  createBGPattern: function IFrameHighlighter_createBGPattern()
> +  {
> +    this.bgPattern = this.context.createPattern(this.bgImage, 'repeat');

Nit: double quotes (for consistency).

@@ +401,5 @@
>    //// Event Handling
>  
> +  attachInspectListeners: function IFH_attachInspectListeners()
> +  {
> +    if (!this.iframeDoc) {

I think we do not want this silent failure here.

We should see the code throw if we call attachInspectListeners() too early.

@@ +896,5 @@
> +  highlighterReady: function IUI_highlighterReady()
> +  {
> +    Services.obs.removeObserver(InspectorUI.highlighterReady, "highlighter-ready", false);
> +
> +    InspectorUI.highlighter.ready = true;

This needs to be cleaned up. No longer needed.

(I believe it's a remainder of the work we did on the initialization code.)

@@ +964,5 @@
>              break;
>          }
>          break;
>        case "mousemove":
> +        // not implemented here.

Why not remove the case?

@@ +1040,2 @@
>      this.highlighter.highlightNode(aNode);
> +    Services.obs.notifyObservers(null, "inspector-inspecting-node", null);

Doesn't this notification duplicate the inspector-highlighting one?

This notification is not used anywhere in this patch.

::: browser/base/content/tabbrowser.xml
@@ +2591,5 @@
>              return;
>  
>            var tab = this._getTabForContentWindow(contentWin);
> +          if (!tab)
> +            return;

Why is this needed?

::: browser/base/content/test/inspector/browser_inspector_iframeTest.js
@@ +116,5 @@
> +  dump("testComparisons2\n");
> +  Services.obs.removeObserver(performTestComparisons2, "inspector-highlighting", false);
> +
> +  // todo(InspectorUI.selection, div2, "selection matches div2 node");
> +  // todo(InspectorUI.highlighter.highlitNode, div2, "highlighter matches selection");

This means you punted on making the iframeTest pass? :)

::: browser/base/content/test/inspector/browser_inspector_treeSelection.js
@@ +58,5 @@
> +    "proident, sunt in culpa qui officia deserunt mollit anim id est laborum.";
> +  div.appendChild(h1);
> +  div.appendChild(p1);
> +  div.appendChild(p2);
> +  // doc.body.addEventListener("DOMSubtreeModified", , false);

Please remove this line if it is not needed.
Attachment #536607 - Flags: feedback?(mihai.sucan) → feedback-
Thanks for the feedback. Comments+replies below...

(In reply to comment #17)
> Comment on attachment 536607 [details] [diff] [review] [review]
> highlighter-0.4.1
> 
> Review of attachment 536607 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Usage comments:
> 
> - The highlighter covers the scrollbars which means users cannot scroll the
> page, unless they use the mouse scroll wheel.

Yeah, I'm wondering what to do about that. Since we're in a <stack> we can't really "let the scrollbars through" as they're in the lower level <browser> element.

I've been considering detecting the scrollbars and adding my own scrollbars (if present) to the iframe that send the appropriate scroll coordinates to the content browser.

> - If I try to use the Up/Down arrow keys it also fails to scroll the page.

Adding a listener for these keys is another #todo.

> Usability is very much impaired by these two issues. Once you start the
> Inspector you cannot easily go to a different location in the page.

Yeah, agreed. I've been trying to keep this patch as simple as possible, but these features might be a requirement.

> - If you try to highlight elements in iframes the highlighter is wrongly
> positioned. Really broken behavior. Try it in the new Yahoo Mail (beta),
> when reading a message, for example.

Yeah, there's some funky behavior there. Need to debug.

> - You need to allow mouse and keyboard interaction with the web page when
> the Inspect option is not active. Currently the user is frozen in a state
> that requires him/her to close and reopen the Inspector whenever the user
> needs to interact with the page. For example, if I am playing with Gmail or
> Yahoo Mail I cannot open up emails from the list, until I close the
> Inspector, then I have to reopen to see how it works with the email view,
> and so on.

I'm not sure we want all events to pass through to the content layer, though some should. I was considering adding buttons to the highlighter to the actively selected node to control things like :hover to activate content, but it would be a selective thing.

What do you think?

> General comments:
> 
> - The new highlighter.css files need the MPL tri-license header, IIANM.

We have CSS files in browser without the license header.

> - The X (the cross) displayed in the iframe needs to be on the left when an
> RTL language is used.

probably true, though I think we need UX feedback on the position anyway. It might be more appropriate to have it on the left on Mac, right on Windows/Linux, for example.

> Code is fine, I would give it f+, but the usability/accessibility issues
> are, I believe, important to fix before we can move forward. We need the
> keyboard and mouse usage figured out, we need to not cover the scrollbars,
> and, finally, we need to deal with iframes correctly - this is basic
> functionality that needs to work properly, and we already have a test that
> shows it does not.

Understood. Let's figure out how to fix those issues and get it done.

> ::: browser/base/content/inspector.js
> @@ +82,5 @@
> > +    iframe.setAttribute("id", "highlighter-frame");
> > +    iframe.setAttribute("transparent", "true");
> > +    iframe.setAttribute("type", "content");
> > +    iframe.addEventListener("DOMTitleChanged", function(event) {
> > +      event.stopPropagation();
> 
> Why do you do this?

a DOMTitleChanged event fires from the iframe which causes the tabbrowser to throw an error which causes further breakage.

[nits removed, I'll address them in an update]

> ::: browser/base/content/tabbrowser.xml
> @@ +2591,5 @@
> >              return;
> >  
> >            var tab = this._getTabForContentWindow(contentWin);
> > +          if (!tab)
> > +            return;
> 
> Why is this needed?

an alternative fix for the above-mentioned DOMTitleChanged issue. Was recommended by zpao, I believe.

> ::: browser/base/content/test/inspector/browser_inspector_iframeTest.js
> @@ +116,5 @@
> > +  dump("testComparisons2\n");
> > +  Services.obs.removeObserver(performTestComparisons2, "inspector-highlighting", false);
> > +
> > +  // todo(InspectorUI.selection, div2, "selection matches div2 node");
> > +  // todo(InspectorUI.highlighter.highlitNode, div2, "highlighter matches selection");
> 
> This means you punted on making the iframeTest pass? :)

yup. :)

...

Thanks!
(In reply to comment #18)
> Thanks for the feedback. Comments+replies below...
> 
> (In reply to comment #17)
> > Comment on attachment 536607 [details] [diff] [review] [review] [review]
> > highlighter-0.4.1
> > 
> > Review of attachment 536607 [details] [diff] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > Usage comments:
> > 
> > - The highlighter covers the scrollbars which means users cannot scroll the
> > page, unless they use the mouse scroll wheel.
> 
> Yeah, I'm wondering what to do about that. Since we're in a <stack> we can't
> really "let the scrollbars through" as they're in the lower level <browser>
> element.
> 
> I've been considering detecting the scrollbars and adding my own scrollbars
> (if present) to the iframe that send the appropriate scroll coordinates to
> the content browser.

How about detecting the scrollbars and just making the highlighter iframe smaller? Such that they don't cover the scrollbars. That should be doable. The scrollbar width and height can be determined easily (I did it for the autocomplete popup).

Also, I believe you can check the width and height of the web page viewport, so you eliminate the complications. You just keep the highlighter at the same size as the viewport.


> > - You need to allow mouse and keyboard interaction with the web page when
> > the Inspect option is not active. Currently the user is frozen in a state
> > that requires him/her to close and reopen the Inspector whenever the user
> > needs to interact with the page. For example, if I am playing with Gmail or
> > Yahoo Mail I cannot open up emails from the list, until I close the
> > Inspector, then I have to reopen to see how it works with the email view,
> > and so on.
> 
> I'm not sure we want all events to pass through to the content layer, though
> some should. I was considering adding buttons to the highlighter to the
> actively selected node to control things like :hover to activate content,
> but it would be a selective thing.
> 
> What do you think?

Yeah, the highlighter buttons would be selective things, only regions that don't allow events to go through.

Do you know the pointer-events CSS property? That might be of use, for this patch.


> > General comments:
> > 
> > - The new highlighter.css files need the MPL tri-license header, IIANM.
> 
> We have CSS files in browser without the license header.

I thought we need the license header for almost everything. ;)
(In reply to comment #19)
> (In reply to comment #18)
[...]
> > I've been considering detecting the scrollbars and adding my own scrollbars
> > (if present) to the iframe that send the appropriate scroll coordinates to
> > the content browser.
> 
> How about detecting the scrollbars and just making the highlighter iframe
> smaller? Such that they don't cover the scrollbars. That should be doable.
> The scrollbar width and height can be determined easily (I did it for the
> autocomplete popup).

Hm, would that work? I was working under the impression that stacks were... stacked. If you can cover only a part of the content area then that'll be easy. I'll give it a try and report back.

> Also, I believe you can check the width and height of the web page viewport,
> so you eliminate the complications. You just keep the highlighter at the
> same size as the viewport.

Sure.

> > I'm not sure we want all events to pass through to the content layer, though
> > some should. I was considering adding buttons to the highlighter to the
> > actively selected node to control things like :hover to activate content,
> > but it would be a selective thing.
> > 
> > What do you think?
> 
> Yeah, the highlighter buttons would be selective things, only regions that
> don't allow events to go through.
> 
> Do you know the pointer-events CSS property? That might be of use, for this
> patch.

I do. I'll take a look, though some of that might be better off in a followup.

> > > - The new highlighter.css files need the MPL tri-license header, IIANM.
> > 
> > We have CSS files in browser without the license header.
> 
> I thought we need the license header for almost everything. ;)

Maybe so. Doesn't hurt to add them anyway.

thanks!
(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> [...]
> > > I've been considering detecting the scrollbars and adding my own scrollbars
> > > (if present) to the iframe that send the appropriate scroll coordinates to
> > > the content browser.
> > 
> > How about detecting the scrollbars and just making the highlighter iframe
> > smaller? Such that they don't cover the scrollbars. That should be doable.
> > The scrollbar width and height can be determined easily (I did it for the
> > autocomplete popup).
> 
> Hm, would that work? I was working under the impression that stacks were...
> stacked. If you can cover only a part of the content area then that'll be
> easy. I'll give it a try and report back.

If this it not working, here is another idea: put the iframe in a container, make the container insensible to mouse events (pointer-events: none), make the iframe sensible (pointer-events: auto), and add a padding to the container (for the scrollbar). Quick test: http://jsfiddle.net/Y2ytc/1/
(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > (In reply to comment #18)
> > [...]
> > > > I've been considering detecting the scrollbars and adding my own scrollbars
> > > > (if present) to the iframe that send the appropriate scroll coordinates to
> > > > the content browser.
> > > 
> > > How about detecting the scrollbars and just making the highlighter iframe
> > > smaller? Such that they don't cover the scrollbars. That should be doable.
> > > The scrollbar width and height can be determined easily (I did it for the
> > > autocomplete popup).
> > 
> > Hm, would that work? I was working under the impression that stacks were...
> > stacked. If you can cover only a part of the content area then that'll be
> > easy. I'll give it a try and report back.

I didn't manage to make that work.

> If this it not working, here is another idea: put the iframe in a container,
> make the container insensible to mouse events (pointer-events: none), make
> the iframe sensible (pointer-events: auto), and add a padding to the
> container (for the scrollbar). Quick test: http://jsfiddle.net/Y2ytc/1/

This is working (just tested):
A container (flex: 1; padding: 0 14px 0 0; pointer-event: none),
the iframe (flex: 1; pointer-events: auto).

You "just" have to dynamically update the values of the padding.
Aha! Well done.

Do you want to attach a follow-up patch with these changes here?
Copy paste:

    let iframe = document.createElement("iframe");

    // We create the container
    let div = document.createElement("div");

    iframe.setAttribute("id", "highlighter-frame");
    iframe.setAttribute("transparent", "true");
    iframe.setAttribute("type", "content");
    iframe.addEventListener("DOMTitleChanged", function(event) {
      event.stopPropagation();
    }, true);

    // flex=1 for both the container and the iframe
    div.flex = 1;
    iframe.flex = 1;

    // Padding. Must be updated dynamicaly
    div.setAttribute("style", "padding: 0 14px 14px 0; border: 1px solid black; pointer-events: none;");
    // The magic happens here. We cancel the point-events: none of the parent
    iframe.setAttribute("style", "pointer-events: auto");

    /* ...... */

    // We add the iframe in the container, then we append the container to the stack
    div.appendChild(iframe);
    stack.appendChild(div);
    this.iframe = iframe;
Blocks: 663100
Attached patch highlighter-0.5.0 (obsolete) — Splinter Review
Work in progress patch.

Addressed the nit-picks from comment 19, and some more.

Fixed the iframe bugs.

Fixed the scrollbars covering. Thanks to Paul!

Merged Paul's work for Canvas to HTML migration, taken from bug 663100, with some additional fixes.

Remaining work: some additional minor fixes and overall fine-tuning, and obviously I need to fix the usability issues mentioned in comment 19 - mouse and keyboard events handling.
Attachment #536607 - Attachment is obsolete: true
Attached patch highlighter-0.5.1 (obsolete) — Splinter Review
Completed patch changes, addressed all the issues raised in comment 19.

- fixed keyboard and mouse usability issues. Matching Chrome's Web Inspector behavior as agreed with Kevin:
  * mouse moves are allowed to go to the web page even during inspection.
  * mousedown/up/clicks are not allowed during inspection, but after you are locked on an element, these events are allowed.
  * all keyboard events are allowed, except Escape/Return during inspection.
- fixed further iframe-related issues and scrolling. The viewContainsRect() and highlightVisibleRegion() methods were not really working - removed them and made the code properly deal with iframes, partially visible or entirely invisible elements, and so on.
- further code cleanup and bug fixes.

Asking for review.

Dietrich: please note that bug 566084 and bug 566085 are relevant during testing, these are visible issues. We are not tackling them in this patch because we are trying to keep it small. Thank you!
Attachment #538481 - Attachment is obsolete: true
Attachment #538691 - Flags: review?(dietrich)
Since the latest patch has additional features and behaviour changes, please do a feedback-r? round on those changes with someone on the devtools team. I'll review after that!
(In reply to comment #27)
> Since the latest patch has additional features and behaviour changes, please
> do a feedback-r? round on those changes with someone on the devtools team.
> I'll review after that!

Robert worked on this patch, and I also did some work (as you can see). We've been providing each other feedback. He's now on vacation.

I'll ask for feedback one of the new team members, Paul, who is also starting work on the highlighter.
Attached patch highlighter-0.5.1b (obsolete) — Splinter Review
Minor patch update: added Paul to the contributors list, in inspector.js. (forgot to do this in the previous update)

Asking for feedback from Paul. Please note that I am going to attach an interdiff to make it easier to give feedback on the latest changes.
Attachment #538691 - Attachment is obsolete: true
Attachment #538691 - Flags: review?(dietrich)
Attachment #538694 - Flags: feedback?(paul)
Attached patch changes between 0.4.1 and 0.5.1b (obsolete) — Splinter Review
Interdiff showing differences between highlighter 0.4.1 (attachment 536607 [details] [diff] [review]) from Robert, and highlighter 0.5.1b (attachment 538694 [details] [diff] [review]).

This should make it easier to provide feedback on the changes I did. Please note that this diff also shows the changes you made in bug 663100.

Looking forward to your feedback, thanks!
Attachment #538695 - Flags: feedback?(paul)
Comment on attachment 538694 [details] [diff] [review]
highlighter-0.5.1b

Looks good. (I didn't check the tests)

/browser/base/content/highlighter.xhtml
>+<html xmlns="http://www.w3.org/1999/xhtml">
>+<head>
>+  <link rel="stylesheet" href="chrome://browser/skin/highlighter.css" type="text/css"/>
>+</head>
>+<body>
>+<div id="close-button"/>

What about using a real <button>? (Better for accessibility)

/browser/base/content/inspector.js
>+    // clientRect is read-only, we need to be able to change properties.
>+    let rect = {top: clientRect.top,
>+                left: clientRect.left,
>+                width: clientRect.width,
>+                height: clientRect.height};
>+
>+    if (rect == this._highlightRect) {
>+      return; // same rectangle
>+    }

This will be always false. You must compare the properties.
Attachment #538694 - Flags: feedback?(paul) → feedback+
(In reply to comment #31)
> Comment on attachment 538694 [details] [diff] [review] [review]
> highlighter-0.5.1b
> 
> Looks good. (I didn't check the tests)
> 
> /browser/base/content/highlighter.xhtml
> >+<html xmlns="http://www.w3.org/1999/xhtml">
> >+<head>
> >+  <link rel="stylesheet" href="chrome://browser/skin/highlighter.css" type="text/css"/>
> >+</head>
> >+<body>
> >+<div id="close-button"/>
> 
> What about using a real <button>? (Better for accessibility)

We can use role="button" from ARIA.

> /browser/base/content/inspector.js
> >+    // clientRect is read-only, we need to be able to change properties.
> >+    let rect = {top: clientRect.top,
> >+                left: clientRect.left,
> >+                width: clientRect.width,
> >+                height: clientRect.height};
> >+
> >+    if (rect == this._highlightRect) {
> >+      return; // same rectangle
> >+    }
> 
> This will be always false. You must compare the properties.

Yeah, good point. I forgot to do this yesterday late in the evening (haste makes waste :) ).

Thanks for your feedback+!
Attached patch highlighter-0.5.2 (obsolete) — Splinter Review
Updated the patch. Added the ARIA role=button attribute to the close button, and fixed the _highlightRect check.

The close button didn't catch the click event, which means I had to change the handleClick() method to proxy clicks to the iframe. The way we handle the click event is open for discussion, so I am asking you for feedback again.

Looking forward to your comments, thanks!
Attachment #538694 - Attachment is obsolete: true
Attachment #538695 - Attachment is obsolete: true
Attachment #538695 - Flags: feedback?(paul)
Attachment #538772 - Flags: feedback?(paul)
Attachment #538772 - Flags: feedback?(paul) → feedback+
Comment on attachment 538772 [details] [diff] [review]
highlighter-0.5.2

Asking for review from Dietrich.

Thanks for the f+ Paul!
Attachment #538772 - Flags: review?(dietrich)
Blocks: 663830
Blocks: 663781
Comment on attachment 538772 [details] [diff] [review]
highlighter-0.5.2

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

first pass. just a few nit fixes so far, but a couple of questions that need answering. please do a try-run w/ these tests as soon as possible and post results on the bug.

::: browser/app/profile/firefox.js
@@ +1009,4 @@
>  pref("devtools.errorconsole.enabled", false);
> +
> +// Enable the Inspector
> +pref("devtools.inspector.enabled", true);

the bug summary doesn't hint at this at all. i'm not familiar enough with why the inspector was turned off to make this call. what were those issues, and how are they now addressed?

::: browser/base/content/browser.xul
@@ +1026,5 @@
>  </vbox>
> +# <iframe id="highlighter-frame"
> +#   transparent="true"
> +#   type="content"
> +#   src="chrome://content/base/highlighter.html"/>

Can you add a comment as to why this is here, as was done below for tab-view?

::: browser/base/content/insideOutBox.js
@@ +199,5 @@
>    {
>      let objectBox = this.createObjectBox(aObject);
> +    if (!objectBox) {
> +      return;
> +    }

in what circumstances can this happen?

also, i'm not a fan of early return, since i think it causes bugs due to lack of visibility into side-effects when looking at patches (full function often not in patch, even with 8 lines of context). would prefer just a block around the success condition code. but that's stylistic, up to you really.

also also, since this function actually declares a return value, you should 1) document what it returns in the failure condition and 2) return that.

::: browser/base/content/inspector.js
@@ +95,5 @@
> +    iframe.setAttribute("style", "-moz-user-focus: ignore");
> +
> +    let self = this;
> +    iframe.addEventListener("load", function iframeLoaded() {
> +      self.iframe.removeEventListener("load", iframeLoaded, true);

here you do self.iframe, which is unnecessary, as shown later in the listener when you refer to iframe directly. should pick one and be consistent. also, could use bind() maybe? death to self!

@@ +107,5 @@
> +      let closeButton = self.iframeDoc.getElementById("close-button");
> +      closeButton.addEventListener("click", function IFH_closeClicked(evt) {
> +        closeButton.removeEventListener("click", IFH_closeClicked, false);
> +        InspectorUI.closeInspectorUI();
> +      }, false);

my kingdom for a listenOnce helper.

@@ +130,5 @@
> +   */
> +  destroy: function IFH_destroy()
> +  {
> +    this.browser.removeEventListener("click", this, true);
> +    this.ready = false;

is this necessary?

@@ +276,5 @@
>    {
> +    this._highlighting = false;
> +    this.veilMiddleDiv.style.height = 0;
> +    this.veilTransparentDiv.style.width = 0;
> +    Services.obs.notifyObservers(null, "inspector-unhighlighting", null);

please make sure to document these notifications on the MDC page for notifications.

@@ +302,5 @@
>     * Return the node under the highlighter rectangle. Useful for testing.
>     * Calculation based on midpoint of diagonal from top left to bottom right
>     * of panel.
>     *
> +   * @returns nsIDOMNode|null

Should document the conditions under which this would return null.

If you're going to use API-doc-like comments, I'll hold you to that ;)

@@ +499,5 @@
> +    hbox.appendChild(scrollbar);
> +
> +    document.documentElement.appendChild(hbox);
> +    this._scrollbarWidth = scrollbar.clientWidth;
> +    document.documentElement.removeChild(hbox);

whoa, is there seriously not a proper way to do this?

::: browser/base/content/tabbrowser.xml
@@ +2591,5 @@
>              return;
>  
>            var tab = this._getTabForContentWindow(contentWin);
> +          if (!tab)
> +            return;

not enough context to tell whether this is a behaviour change. high chance of side-effect, and my unfamiliarity with this code means you should get review from Gavin or Dao. from the change, it looks like it should be it's own bug w/ tests...
(In reply to comment #35)
> Comment on attachment 538772 [details] [diff] [review] [review]
> highlighter-0.5.2
> ::: browser/app/profile/firefox.js
> @@ +1009,4 @@
> >  pref("devtools.errorconsole.enabled", false);
> > +
> > +// Enable the Inspector
> > +pref("devtools.inspector.enabled", true);
> 
> the bug summary doesn't hint at this at all. i'm not familiar enough with
> why the inspector was turned off to make this call. what were those issues,
> and how are they now addressed?

The Inspector feature was actually started as part of Firefox 4 and was turned off at the feature freeze time (in September!) so that we could devote our time to the Web Console.

Since then, we've had the change to the rapid release process. I believe the general idea with the new process is to land features turned on in Nightly and then turn them off in Aurora if it's decided that they're not ready yet. That's right, isn't it?

I am certainly hoping that we'll have these features in good enough shape for Firefox 7, but we do have this pref in front so that we can turn the Highlighter off easily if need be.

Separate nit: perhaps the pref should be changed to devtools.highlighter.enabled, since that's what we're calling the tool now.
(In reply to comment #36)
> Since then, we've had the change to the rapid release process. I believe the
> general idea with the new process is to land features turned on in Nightly
> and then turn them off in Aurora if it's decided that they're not ready yet.
> That's right, isn't it?
> 
> I am certainly hoping that we'll have these features in good enough shape
> for Firefox 7, but we do have this pref in front so that we can turn the
> Highlighter off easily if need be.

Good enough for me. Killswitch ftw.
(In reply to comment #35)
> Comment on attachment 538772 [details] [diff] [review] [review]
> highlighter-0.5.2
> 
> Review of attachment 538772 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> first pass. just a few nit fixes so far, but a couple of questions that need
> answering. please do a try-run w/ these tests as soon as possible and post
> results on the bug.

Sure, will do!


> ::: browser/base/content/browser.xul
> @@ +1026,5 @@
> >  </vbox>
> > +# <iframe id="highlighter-frame"
> > +#   transparent="true"
> > +#   type="content"
> > +#   src="chrome://content/base/highlighter.html"/>
> 
> Can you add a comment as to why this is here, as was done below for tab-view?

Will do.

> ::: browser/base/content/insideOutBox.js
> @@ +199,5 @@
> >    {
> >      let objectBox = this.createObjectBox(aObject);
> > +    if (!objectBox) {
> > +      return;
> > +    }
> 
> in what circumstances can this happen?

I believe this can happen when stale objects are supposed to be rendered in the DOM tree.

I haven't asked Robert about this specific change. If you look at createObjectBox() it can return null. The change to select() aligns with that.


> also also, since this function actually declares a return value, you should
> 1) document what it returns in the failure condition and 2) return that.

Agreed. Will do this.


> ::: browser/base/content/inspector.js
> @@ +95,5 @@
> > +    iframe.setAttribute("style", "-moz-user-focus: ignore");
> > +
> > +    let self = this;
> > +    iframe.addEventListener("load", function iframeLoaded() {
> > +      self.iframe.removeEventListener("load", iframeLoaded, true);
> 
> here you do self.iframe, which is unnecessary, as shown later in the
> listener when you refer to iframe directly. should pick one and be
> consistent. also, could use bind() maybe? death to self!

Sure, will fix this.


> @@ +107,5 @@
> > +      let closeButton = self.iframeDoc.getElementById("close-button");
> > +      closeButton.addEventListener("click", function IFH_closeClicked(evt) {
> > +        closeButton.removeEventListener("click", IFH_closeClicked, false);
> > +        InspectorUI.closeInspectorUI();
> > +      }, false);
> 
> my kingdom for a listenOnce helper.

You mean I should write a minimal listenOnce helper? Will do.


> @@ +130,5 @@
> > +   */
> > +  destroy: function IFH_destroy()
> > +  {
> > +    this.browser.removeEventListener("click", this, true);
> > +    this.ready = false;
> 
> is this necessary?

You mean we don't need "this.ready"? I can look into removing the property. Since we have notifications we could do away with this .ready property.


> @@ +302,5 @@
> >     * Return the node under the highlighter rectangle. Useful for testing.
> >     * Calculation based on midpoint of diagonal from top left to bottom right
> >     * of panel.
> >     *
> > +   * @returns nsIDOMNode|null
> 
> Should document the conditions under which this would return null.
> 
> If you're going to use API-doc-like comments, I'll hold you to that ;)

Hehe, will do!


> @@ +499,5 @@
> > +    hbox.appendChild(scrollbar);
> > +
> > +    document.documentElement.appendChild(hbox);
> > +    this._scrollbarWidth = scrollbar.clientWidth;
> > +    document.documentElement.removeChild(hbox);
> 
> whoa, is there seriously not a proper way to do this?

I am open for better solutions. :)


> ::: browser/base/content/tabbrowser.xml
> @@ +2591,5 @@
> >              return;
> >  
> >            var tab = this._getTabForContentWindow(contentWin);
> > +          if (!tab)
> > +            return;
> 
> not enough context to tell whether this is a behaviour change. high chance
> of side-effect, and my unfamiliarity with this code means you should get
> review from Gavin or Dao. from the change, it looks like it should be it's
> own bug w/ tests...

See comment 18 for why this was added. I can remove it, if nothing breaks.

Thanks for your comments!
(In reply to comment #36)
> Separate nit: perhaps the pref should be changed to
> devtools.highlighter.enabled, since that's what we're calling the tool now.

Sure, but it would create a backwards compat issue. Firefox 4 has the devtools.inspector.enabled pref. If we change to "highlighter", the behavior will be that of a new pref.

Do you want me to rename the pref?
(In reply to comment #39)
> (In reply to comment #36)
> > Separate nit: perhaps the pref should be changed to
> > devtools.highlighter.enabled, since that's what we're calling the tool now.
> 
> Sure, but it would create a backwards compat issue. Firefox 4 has the
> devtools.inspector.enabled pref. If we change to "highlighter", the behavior
> will be that of a new pref.
> 
> Do you want me to rename the pref?

I don't have strong feelings about it, because it's a hidden pref that's really just there as a killswitch. On the one hand, backwards compat for a hidden pref for an unreleased feature is not really a consideration. On the other hand, if someone had opted into the Inspector and the Highlighter needed to be turned off, at least they'll still be opted in.

The theory would be that the pref would be removed at some future point anyhow. Therefore, *shrug*.

I have a minor preference for renaming the pref for consistency's sake.
(In reply to comment #35)
> @@ +276,5 @@
> >    {
> > +    this._highlighting = false;
> > +    this.veilMiddleDiv.style.height = 0;
> > +    this.veilTransparentDiv.style.width = 0;
> > +    Services.obs.notifyObservers(null, "inspector-unhighlighting", null);
> 
> please make sure to document these notifications on the MDC page for
> notifications.

Sure. The (new) notifications are:

- highlighter-ready - fires when the highlighter component is initialized.
  - note that this is used by the Inspector to determine its initialization process. 
- inspector-opened - fires once the Inspector tool completes initialization.
- inspector-highlighting - fires every time when a different node in the page is highlighted.
- inspector-unhighlighting - fires every time when the highlighter stops highlighting a node.
- inspector-closed - fires once the Inspector tool is closed.
Keywords: dev-doc-needed
(In reply to comment #40)
> 
> I don't have strong feelings about it, because it's a hidden pref that's
> really just there as a killswitch. On the one hand, backwards compat for a
> hidden pref for an unreleased feature is not really a consideration. On the
> other hand, if someone had opted into the Inspector and the Highlighter
> needed to be turned off, at least they'll still be opted in.
> 
> The theory would be that the pref would be removed at some future point
> anyhow. Therefore, *shrug*.
> 
> I have a minor preference for renaming the pref for consistency's sake.

The point that it will be removed at some future point anyhow is also a point for not renaming the pref - it's not something that sticks around, and there's no real point / benefit in adding another pref to old profiles for current users.
Attached patch highlighter-0.5.3 (obsolete) — Splinter Review
Made the changes requested in comment 35:

- added a comment to browser.xul as requested.
- added a comment for IOBox_select().
- added a comment as requested for IOBox_createObjectBox()
- corrected the self.iframe use and switched to .bind().
- added a listenOnce() helper.
- removed IFH.ready since it's not really needed.
- added a comment to the IFH.highlightNode getter.
- removed the change from tabbrowser.xml. This is no longer needed since we have the DOMTitleChanged event handler on the iframe that stops the event propagation.

Thanks for your comments! Looking forward to your review.


I sent the patch to the try server. Results:

http://tbpl.mozilla.org/?tree=Try&rev=24f386ecfcb5

Builds and logs:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-24f386ecfcb5
Attachment #538772 - Attachment is obsolete: true
Attachment #538772 - Flags: review?(dietrich)
Attachment #539302 - Flags: review?(dietrich)
Just a quick note: all tests passed, on all systems, in the try server run.
Mihai, in your patch, I see this:
+-   // open inspector UI
I think it's a typo.
Comment on attachment 539302 [details] [diff] [review]
highlighter-0.5.3

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

thanks for making the changes, r=me.

::: browser/base/content/inspector.js
@@ +107,5 @@
> +
> +      this.browser.addEventListener("click", this, true);
> +      iframe.contentWindow.addEventListener("resize", this, false);
> +      this.handleResize();
> +      Services.obs.notifyObservers(null, "highlighter-ready", null);

ack, i meant to mention this before: can you move the topic strings to consts w/ documentation comments at the top of the file?

@@ +515,5 @@
> +    aTarget.addEventListener(aName, function listenOnce_handler(aEvent) {
> +      aTarget.removeEventListener(aName, listenOnce_handler, aCapturing);
> +      aCallback.call(this, aEvent);
> +    }, aCapturing);
> +  },

this pattern is pervasive throughout the codebase. i wonder if there's somewhere we could stash it for general use. maybe iq.js? that's for another bug though.
Attachment #539302 - Flags: review?(dietrich) → review+
(In reply to comment #45)
> Mihai, in your patch, I see this:
> +-   // open inspector UI
> I think it's a typo.

Missed that, thanks for pointing it out!


(In reply to comment #46)
> Comment on attachment 539302 [details] [diff] [review] [review]
> highlighter-0.5.3
> 
> Review of attachment 539302 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> thanks for making the changes, r=me.

Thanks for the review+!

> ::: browser/base/content/inspector.js
> @@ +107,5 @@
> > +
> > +      this.browser.addEventListener("click", this, true);
> > +      iframe.contentWindow.addEventListener("resize", this, false);
> > +      this.handleResize();
> > +      Services.obs.notifyObservers(null, "highlighter-ready", null);
> 
> ack, i meant to mention this before: can you move the topic strings to
> consts w/ documentation comments at the top of the file?

Sure. Will update the patch accordingly.
Updated the patch to address comment 45 and comment 46.

This is ready to land!
Attachment #539302 - Attachment is obsolete: true
Whiteboard: [best: 1d, likely: 3d, worst: 1w][highlighter][rebased-05-14] → [best: 1d, likely: 3d, worst: 1w][highlighter][land-in-devtools]
Whiteboard: [best: 1d, likely: 3d, worst: 1w][highlighter][land-in-devtools] → [best: 1d, likely: 3d, worst: 1w][highlighter][fixed-in-devtools]
Comment on attachment 539752 [details] [diff] [review]
[in-devtools] highlighter-0.5.4

http://hg.mozilla.org/projects/devtools/rev/96c76749e35f
Attachment #539752 - Attachment description: highlighter-0.5.4 → [in-devtools] highlighter-0.5.4
http://hg.mozilla.org/mozilla-central/rev/96c76749e35f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
just updating the summary to reflect what landed. No canvas!
Summary: Rewrite PanelHighlighter using transparent xul iframe and canvas → Rewrite PanelHighlighter using transparent xul iframe and HTML
Target Milestone: Firefox 7 → ---
No longer blocks: 650794
Alias: highlighter
Target Milestone: --- → Firefox 7
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.