The highlighter needs a close button to close the highlighter UI.

first patch

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 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;

> 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.

(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.

> 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.

Verified fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1