Last Comment Bug 653550 - Add a close button to the highlighter
: Add a close button to the highlighter
Status: VERIFIED FIXED
[best: 1h, likely: 4h, worst: 2d][hig...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Rob Campbell [:rc] (:robcee)
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 642471 654810
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-28 13:37 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2011-10-07 05:01 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
highlighter-close (18.87 KB, patch)
2011-05-17 12:18 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review-
Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2011-04-28 13:37:06 PDT
The highlighter needs a close button to close the highlighter UI.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-05-17 12:18:05 PDT
Created attachment 533037 [details] [diff] [review]
highlighter-close

first patch
Comment 2 Rob Campbell [:rc] (:robcee) 2011-05-17 12:20:56 PDT
also note: this patch requires the patch from bug 654810, split unittests. I want to break those out before going further.

Still todo: add a unittest.
Comment 3 Mihai Sucan [:msucan] 2011-05-18 12:24:04 PDT
Comment on attachment 533037 [details] [diff] [review]
highlighter-close

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

The new file names highlighter.* should perhaps be inspector-highlighter.*. That's because "highlighter" is rather confusing.

Please note that the gnomestripe highlighter.css comments apply to the other two CSS files as well.

Giving the patch r- for the moment.

::: browser/base/content/highlighter.xhtml
@@ +34,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

The license block is an old one. Needs updates. Same applies to all of the other license blocks in all of the other new files.

::: browser/base/content/inspector.js
@@ +94,5 @@
>        self.iframeDoc = iframe.contentDocument;
>        self.canvas = self.iframeDoc.getElementById("surface");
>        self.resizeCanvas();
>        self.ready = true;
> +      self.closeButton = self.iframeDoc.getElementById("close-button");

You do not need the closeButton node ref. You don't really use it anywhere but when you clear the ref, on inspector-close.

@@ +104,5 @@
> +    this.bgImage = new Image();
> +    this.bgImage.src = "chrome://browser/content/lines.png";
> +    this.bgImage.onload = function imageLoaded() {
> +      self.bgImage.loaded = true;
> +    }

Nit: add a ; at the end of the function curly bracket.

Otherwise: why do you add the background image here? It should be in the iframe XHTML. You shouldn't work with such little details in the main inspector code. You should rely on the UI of the iframe itself. The iframe should have the backgrounds, the images, the CSS, etc. Or maybe I am wrong. Please explain.

Do you ever use bgImage.loaded?

@@ +106,5 @@
> +    this.bgImage.onload = function imageLoaded() {
> +      self.bgImage.loaded = true;
> +    }
> +
> +    iframe.setAttribute("src", "chrome://browser/content/highlighter.xhtml");

This is something that belongs to the master patch, not something I would've expected in an "add a close button" patch. This is basic UI stuff.

@@ +480,5 @@
> +  /**
> +   * Close button in the Highlighter frame has been clicked.
> +   */
> +  closeClicked: function IFrameHighlighter_closeClicked() {
> +    this.closeButton.removeEventListener("click", this.closeClicked, false);

Doesn't event.target provide you with a ref back to the closeButton node?

::: browser/base/jar.mn
@@ +27,5 @@
>  *       content/browser/browser.js                    (content/browser.js)
>  *       content/browser/browser.xul                   (content/browser.xul)
>  *       content/browser/browser-tabPreviews.xml       (content/browser-tabPreviews.xml)
>  *       content/browser/fullscreen-video.xhtml        (content/fullscreen-video.xhtml)
> +*       content/browser/highlighter.xhtml             (content/highlighter.xhtml)

Do you need pre-processing for the file?

::: browser/themes/gnomestripe/browser/highlighter.css
@@ +38,5 @@
> +%endif
> +
> +body {
> +  margin-top: 0px;
> +  margin-left: 0px;

I think this could be just margin: 0;

@@ +50,5 @@
> +  width: 24px;
> +  height: 24px;
> +  position: fixed;
> +  top: 12px;
> +  right: 12px;

This doesn't deal with RTL text. You need to position the close-button in the opposite direction for RTL users.

@@ +55,5 @@
> +  z-index: 1;
> +}
> +
> +#surface {
> +  margin: 0px;

margin: 0;
Comment 4 Rob Campbell [:rc] (:robcee) 2011-05-20 07:10:09 PDT
> This is something that belongs to the master patch, not something I
> would've expected in an "add a close button" patch. This is basic UI stuff.

I considered folding this all back into bug 642471 as it really does belong there. If you think that's a better approach rather than tacking it on after-the-fact, I can do that.

Let me know what you think.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-05-24 11:10:26 PDT
(In reply to comment #3)
> Comment on attachment 533037 [details] [diff] [review] [review]
> highlighter-close
> 
> Review of attachment 533037 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> The new file names highlighter.* should perhaps be inspector-highlighter.*.
> That's because "highlighter" is rather confusing.

I wouldn't say it's confusing. It's a feature of the inspector perhaps. I'm not a fan of huge filenames.

> Please note that the gnomestripe highlighter.css comments apply to the other
> two CSS files as well.
> 
> Giving the patch r- for the moment.
> 
> ::: browser/base/content/highlighter.xhtml
> @@ +34,5 @@
> > +# and other provisions required by the GPL or the LGPL. If you do not delete
> > +# the provisions above, a recipient may use your version of this file under
> > +# the terms of any one of the MPL, the GPL or the LGPL.
> > +#
> > +# ***** END LICENSE BLOCK *****
> 
> The license block is an old one. Needs updates. Same applies to all of the
> other license blocks in all of the other new files.

It's the same license text. This is done in other browser files and preprocessed to remove the header.

I'll convert to <!-- --> style formatting and remove the preprocessor directive.

> ::: browser/base/content/inspector.js
> @@ +94,5 @@
> >        self.iframeDoc = iframe.contentDocument;
> >        self.canvas = self.iframeDoc.getElementById("surface");
> >        self.resizeCanvas();
> >        self.ready = true;
> > +      self.closeButton = self.iframeDoc.getElementById("close-button");
> 
> You do not need the closeButton node ref. You don't really use it anywhere
> but when you clear the ref, on inspector-close.

Good point. Removing.

> @@ +104,5 @@
> > +    this.bgImage = new Image();
> > +    this.bgImage.src = "chrome://browser/content/lines.png";
> > +    this.bgImage.onload = function imageLoaded() {
> > +      self.bgImage.loaded = true;
> > +    }
> 
> Nit: add a ; at the end of the function curly bracket.

done.

> Otherwise: why do you add the background image here? It should be in the
> iframe XHTML. You shouldn't work with such little details in the main
> inspector code. You should rely on the UI of the iframe itself. The iframe
> should have the backgrounds, the images, the CSS, etc. Or maybe I am wrong.
> Please explain.
> 
> Do you ever use bgImage.loaded?

I check it in createBGPattern().

> @@ +106,5 @@
> > +    this.bgImage.onload = function imageLoaded() {
> > +      self.bgImage.loaded = true;
> > +    }
> > +
> > +    iframe.setAttribute("src", "chrome://browser/content/highlighter.xhtml");
> 
> This is something that belongs to the master patch, not something I would've
> expected in an "add a close button" patch. This is basic UI stuff.

Yes, but it needed to live somewhere and the initial "rewrite highlighter using iframes" didn't require it yet.

After discussing in IRC, we've decided to move this all back into the main highlighter rewrite bug 642471.

> @@ +480,5 @@
> > +  /**
> > +   * Close button in the Highlighter frame has been clicked.
> > +   */
> > +  closeClicked: function IFrameHighlighter_closeClicked() {
> > +    this.closeButton.removeEventListener("click", this.closeClicked, false);
> 
> Doesn't event.target provide you with a ref back to the closeButton node?

Yes. Also removed this function and placed it inline with the addEventListener call.

> ::: browser/base/jar.mn
> @@ +27,5 @@
> >  *       content/browser/browser.js                    (content/browser.js)
> >  *       content/browser/browser.xul                   (content/browser.xul)
> >  *       content/browser/browser-tabPreviews.xml       (content/browser-tabPreviews.xml)
> >  *       content/browser/fullscreen-video.xhtml        (content/fullscreen-video.xhtml)
> > +*       content/browser/highlighter.xhtml             (content/highlighter.xhtml)
> 
> Do you need pre-processing for the file?

I did. I don't now.

> ::: browser/themes/gnomestripe/browser/highlighter.css
> @@ +38,5 @@
> > +%endif
> > +
> > +body {
> > +  margin-top: 0px;
> > +  margin-left: 0px;
> 
> I think this could be just margin: 0;

ok.

> @@ +50,5 @@
> > +  width: 24px;
> > +  height: 24px;
> > +  position: fixed;
> > +  top: 12px;
> > +  right: 12px;
> 
> This doesn't deal with RTL text. You need to position the close-button in
> the opposite direction for RTL users.

I think we should handle that with a follow-up bug. Not sure if having it on the top-right corner makes sense on all platforms either.

> @@ +55,5 @@
> > +  z-index: 1;
> > +}
> > +
> > +#surface {
> > +  margin: 0px;
> 
> margin: 0;

OK.

Thanks for the review pass! Closing this down as the code's been moved back to bug 642471.
Comment 6 Rob Campbell [:rc] (:robcee) 2011-05-24 11:12:56 PDT
> Otherwise: why do you add the background image here? It should be in the
> iframe XHTML. You shouldn't work with such little details in the main
> inspector code. You should rely on the UI of the iframe itself. The iframe
> should have the backgrounds, the images, the CSS, etc. Or maybe I am wrong.
> Please explain.

Just a holdover from the way I was originally building the iframe in JS. I'll add it to the xhtml file.
Comment 7 Teodosia Pop 2011-10-07 05:01:52 PDT
Verified fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1

Note You need to log in before you can comment on or make changes to this bug.