Last Comment Bug 664436 - [highlighter] Remove the main iframe
: [highlighter] Remove the main iframe
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Paul Rouget [:paul]
:
:
Mentors:
Depends on: 668500
Blocks: 663830 663898 669652
  Show dependency treegraph
 
Reported: 2011-06-15 08:13 PDT by Paul Rouget [:paul]
Modified: 2011-07-11 15:20 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (30.12 KB, patch)
2011-06-15 08:14 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.1 (30.33 KB, patch)
2011-06-15 08:20 PDT, Paul Rouget [:paul]
mihai.sucan: feedback-
Details | Diff | Splinter Review
patch v1.2 (no tests) (32.20 KB, patch)
2011-06-20 02:36 PDT, Paul Rouget [:paul]
mihai.sucan: feedback+
Details | Diff | Splinter Review
patch v1.3 (32.19 KB, patch)
2011-06-20 03:09 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.4 (33.87 KB, patch)
2011-06-20 07:32 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.5 (33.83 KB, patch)
2011-06-20 07:39 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.6 (33.54 KB, patch)
2011-06-20 09:19 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.7 (33.49 KB, patch)
2011-06-20 17:12 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.8 (33.55 KB, patch)
2011-06-21 04:38 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.9 (33.58 KB, patch)
2011-06-27 07:43 PDT, Paul Rouget [:paul]
dao+bmo: review-
rcampbell: review+
Details | Diff | Splinter Review
patch v1.10 (34.80 KB, patch)
2011-06-28 03:59 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.11 (34.87 KB, patch)
2011-06-28 10:40 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.12 (35.33 KB, patch)
2011-06-28 12:09 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.13 (36.73 KB, patch)
2011-06-30 05:51 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.14 (36.68 KB, patch)
2011-07-04 10:24 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.15 (36.63 KB, patch)
2011-07-06 04:56 PDT, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Splinter Review
patch v1.16 (36.64 KB, patch)
2011-07-08 07:15 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2011-06-15 08:13:18 PDT
The browser and the iframe are stacked. The iframe is insensitive to mouse events. To make some elements events sensible (like the close button), we have to filter all the events.

If we take away the iframe and use inline content, we can make the whole highlighter insensitive to mouse events (pointer-events:none) and easily select the elements we want to be sensible (pointer-events:auto).
Comment 1 Paul Rouget [:paul] 2011-06-15 08:14:20 PDT
Created attachment 539528 [details] [diff] [review]
patch v1
Comment 2 Paul Rouget [:paul] 2011-06-15 08:20:03 PDT
Created attachment 539532 [details] [diff] [review]
patch v1.1
Comment 3 Mihai Sucan [:msucan] 2011-06-15 09:28:35 PDT
Comment on attachment 539532 [details] [diff] [review]
patch v1.1

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

Overall this is really good work!

The only major problem is with events:
- You now add click and mousemove event handlers to this.browser on initialization and you pretty much got rid of the attach/detach event listeners logic.
- Please bring that back. Your patch actually simplifies things in a good way: add the click/mousemove event handlers back to attachPageListeners, for this.browser.
- Remove the checks from handleMouseMove and handleClick if(InspectorUI.inspecting) because they will no longer needed once you do the above.
- Move from InspectorUI the listener for scroll event ... to the highlighter init.
- You removed the event listeners for dblclick, mousedown, mouseup. :) They are needed to prevent the page from executing any code when the user clicks to lock on a node (the page may have event listeners for other composed events, not just "click"). Just stopping click events is not sufficient.

Giving the patch f- for the events stuff, for now. Looking forward for the updated patch!

::: browser/base/content/highlighter.css
@@ +1,5 @@
> +#highlighter-container {
> +  -moz-user-focus: ignore;
> +  pointer-events: none;
> +  overflow: hidden;
> +  width: 100%; height: 100%;

Each property needs to be on a separate line, please.
(please check the entire file)

@@ +15,5 @@
> +#highlighter-controls {
> +  position: absolute;
> +  top: 0; left: 0;
> +}
> +#highlighter-controls > * {

I think for performance reasons you want to just set pointer-events: auto to those elements you want.

@@ +51,5 @@
> +  background-color: rgba(0, 0, 0, 0.5);
> +}
> +
> +#highlighter-close-button {
> +  display: block;

display: block is the default for absolute positioning, IIANM.

@@ +53,5 @@
> +
> +#highlighter-close-button {
> +  display: block;
> +  background-image: url("chrome://browser/skin/KUI-close.png");
> +  padding: 0;

Do xul:boxes have any default padding?

::: browser/base/content/inspector.js
@@ +87,2 @@
>  
> +    let ctrlsBox = document.createElement("box");

When naming variables we want them clear, even if they are longer than we like them.

So, please use something like "controlsBox".
Comment 4 Paul Rouget [:paul] 2011-06-20 02:36:21 PDT
Created attachment 540413 [details] [diff] [review]
patch v1.2 (no tests)
Comment 5 Mihai Sucan [:msucan] 2011-06-20 03:03:08 PDT
Comment on attachment 540413 [details] [diff] [review]
patch v1.2 (no tests)

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

Thank you for addressing the feedback comments.

Patch looks fine. All tests pass locally.

::: browser/base/content/inspector.js
@@ +94,5 @@
>      let stack = this.browser.parentNode;
>      this.win = this.browser.contentWindow;
>      this._highlighting = false;
>  
> +    this.highlighterContainer= document.createElement("box");

this.highlighterContainer = document.createElement("box");

@@ +448,5 @@
>          aEvent.preventDefault();
>          break;
> +      case "scroll":
> +        this.highlight();
> +        break;}

Please add a new line after break;
Comment 6 Paul Rouget [:paul] 2011-06-20 03:09:38 PDT
Created attachment 540415 [details] [diff] [review]
patch v1.3
Comment 7 Paul Rouget [:paul] 2011-06-20 03:16:22 PDT
Comment on attachment 540415 [details] [diff] [review]
patch v1.3

Dão, here we introduce some CSS content to style the highlighter. I don't know where this code should lie. In this patch, I include this in browser.css via a #include.
Comment 8 Dão Gottwald [:dao] 2011-06-20 04:18:56 PDT
Comment on attachment 540415 [details] [diff] [review]
patch v1.3

>+++ b/browser/base/content/highlighter.css	Mon Jun 20 12:06:06 2011 +0200

>+.highlighter-veil, #highlighter-veil-middlebox, #highlighter-veil-transparentbox {

nit: line break after comma

>+.highlighter-veil {
>+  background-color: rgba(0, 0, 0, 0.5);
>+}
>+
>+#highlighter-close-button {
>+  pointer-events: auto;
>+  background-image: url("chrome://browser/skin/KUI-close.png");
>+  width: 24px;
>+  height: 24px;
>+  position: absolute;
>+  top: 12px;
>+  right: 12px;
>+  z-index: 1;
>+  cursor: pointer;
>+}

Most of this stuff belongs in theme rather than content CSS.

>+  highlightRectangle: function Highlighter_highlightRectangle(aRect)
>   {
>     if (aRect.left >= 0 && aRect.top >= 0 &&
>         aRect.width > 0 && aRect.height > 0) {
>       // The bottom div and the right div are flexibles (flex=1).
>       // We don't need to resize them.
>-      this.veilTopDiv.style.height = aRect.top + "px";
>-      this.veilLeftDiv.style.width = aRect.left + "px";
>-      this.veilMiddleDiv.style.height = aRect.height + "px";
>-      this.veilTransparentDiv.style.width = aRect.width + "px";
>+      this.veilTopBox.style.height = aRect.top + "px";
>+      this.veilLeftBox.style.width = aRect.left + "px";
>+      this.veilMiddleBox.style.height = aRect.height + "px";
>+      this.veilTransparentBox.style.width = aRect.width + "px";

Is this correct for right-to-left languages?
Comment 9 Paul Rouget [:paul] 2011-06-20 06:56:07 PDT
(In reply to comment #8)
> Comment on attachment 540415 [details] [diff] [review] [review]
> patch v1.3
> 
> >+++ b/browser/base/content/highlighter.css	Mon Jun 20 12:06:06 2011 +0200
> 
> >+.highlighter-veil, #highlighter-veil-middlebox, #highlighter-veil-transparentbox {
> 
> nit: line break after comma

ok.

> >+.highlighter-veil {
> >+  background-color: rgba(0, 0, 0, 0.5);
> >+}
> >+
> >+#highlighter-close-button {
> >+  pointer-events: auto;
> >+  background-image: url("chrome://browser/skin/KUI-close.png");
> >+  width: 24px;
> >+  height: 24px;
> >+  position: absolute;
> >+  top: 12px;
> >+  right: 12px;
> >+  z-index: 1;
> >+  cursor: pointer;
> >+}
> 
> Most of this stuff belongs in theme rather than content CSS.

Should I keep pointer-events & z-index in browser/base/content/browser.css?
Or should I move everything to browser/theme?

> >+  highlightRectangle: function Highlighter_highlightRectangle(aRect)
> >   {
> >     if (aRect.left >= 0 && aRect.top >= 0 &&
> >         aRect.width > 0 && aRect.height > 0) {
> >       // The bottom div and the right div are flexibles (flex=1).
> >       // We don't need to resize them.
> >-      this.veilTopDiv.style.height = aRect.top + "px";
> >-      this.veilLeftDiv.style.width = aRect.left + "px";
> >-      this.veilMiddleDiv.style.height = aRect.height + "px";
> >-      this.veilTransparentDiv.style.width = aRect.width + "px";
> >+      this.veilTopBox.style.height = aRect.top + "px";
> >+      this.veilLeftBox.style.width = aRect.left + "px";
> >+      this.veilMiddleBox.style.height = aRect.height + "px";
> >+      this.veilTransparentBox.style.width = aRect.width + "px";
> 
> Is this correct for right-to-left languages?

I think it is. I'll test.
Comment 10 Paul Rouget [:paul] 2011-06-20 07:10:28 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > >+  highlightRectangle: function Highlighter_highlightRectangle(aRect)
> > >   {
> > >     if (aRect.left >= 0 && aRect.top >= 0 &&
> > >         aRect.width > 0 && aRect.height > 0) {
> > >       // The bottom div and the right div are flexibles (flex=1).
> > >       // We don't need to resize them.
> > >-      this.veilTopDiv.style.height = aRect.top + "px";
> > >-      this.veilLeftDiv.style.width = aRect.left + "px";
> > >-      this.veilMiddleDiv.style.height = aRect.height + "px";
> > >-      this.veilTransparentDiv.style.width = aRect.width + "px";
> > >+      this.veilTopBox.style.height = aRect.top + "px";
> > >+      this.veilLeftBox.style.width = aRect.left + "px";
> > >+      this.veilMiddleBox.style.height = aRect.height + "px";
> > >+      this.veilTransparentBox.style.width = aRect.width + "px";
> > 
> > Is this correct for right-to-left languages?
> 
> I think it is. I'll test.

Just tested, and it doesn't work (left & right inverted).
Comment 11 Dão Gottwald [:dao] 2011-06-20 07:22:28 PDT
> Should I keep pointer-events & z-index in browser/base/content/browser.css?
> Or should I move everything to browser/theme?

Functional stuff should be in content. pointer-events and z-index probably fall into this category.
Comment 12 Paul Rouget [:paul] 2011-06-20 07:32:32 PDT
Created attachment 540459 [details] [diff] [review]
patch v1.4

Made the changes requested in comment 8:

- added liner breaks
- moved themable stuff in brower/theme
- fixed layout of the veil for right-to-left languages
Comment 13 Paul Rouget [:paul] 2011-06-20 07:33:38 PDT
(In reply to comment #11)
> > Should I keep pointer-events & z-index in browser/base/content/browser.css?
> > Or should I move everything to browser/theme?
> 
> Functional stuff should be in content. pointer-events and z-index probably
> fall into this category.

Ok. Read the comment after submitted patch 1.4. I'll update it.
Comment 14 Paul Rouget [:paul] 2011-06-20 07:39:23 PDT
Created attachment 540461 [details] [diff] [review]
patch v1.5

Dao,

Please check the CSS, it should be ok now. It works well for RTL languages as well now.
Comment 15 Dão Gottwald [:dao] 2011-06-20 07:58:14 PDT
Comment on attachment 540461 [details] [diff] [review]
patch v1.5

>+++ b/browser/base/content/highlighter.css	Mon Jun 20 16:35:18 2011 +0200

>+#highlighter-container {
>+  width: 100%;
>+  height: 100%;
>+}

>+#highlighter-veil-topbox,
>+#highlighter-veil-bottombox {
>+  width: 100%;
>+}

>+#highlighter-veil-bottombox {
>+  -moz-box-flex: 1;
>+}

>+#highlighter-veil-middlebox {
>+  display: -moz-box;
>+}

>+#highlighter-veil-leftbox,
>+#highlighter-veil-rightbox {
>+  height: 100%;
>+}

>+#highlighter-veil-rightbox {
>+  -moz-box-flex: 1;
>+}

Aren't some or even all of these styles no-ops?

>+#highlighter-veil {
>+  -moz-box-orient: vertical;
>+}

>+#highlighter-veil-middlebox {
>+  -moz-box-orient: horizontal;
>+}

Why aren't these hboxes and vboxes, respectively?

>+++ b/browser/themes/winstripe/browser/browser.css	Mon Jun 20 16:35:18 2011 +0200

>+#highlighter-close-button {
>+  background-image: url("chrome://browser/skin/KUI-close.png");
>+  width: 24px;
>+  height: 24px;
>+  position: absolute;
>+  top: 12px;
>+  right: 12px;
>+  cursor: pointer;
>+}

position: absolute; should probably be in content CSS as well.
Comment 16 Paul Rouget [:paul] 2011-06-20 09:19:12 PDT
Created attachment 540490 [details] [diff] [review]
patch v1.6

(In reply to comment #15)
> Comment on attachment 540461 [details] [diff] [review] [review]
> patch v1.5
> 
> >+++ b/browser/base/content/highlighter.css	Mon Jun 20 16:35:18 2011 +0200
> 
> >+#highlighter-container {
> >+  width: 100%;
> >+  height: 100%;
> >+}
> 
> >+#highlighter-veil-topbox,
> >+#highlighter-veil-bottombox {
> >+  width: 100%;
> >+}
> 
> >+#highlighter-veil-bottombox {
> >+  -moz-box-flex: 1;
> >+}
> 
> >+#highlighter-veil-middlebox {
> >+  display: -moz-box;
> >+}
> 
> >+#highlighter-veil-leftbox,
> >+#highlighter-veil-rightbox {
> >+  height: 100%;
> >+}
> 
> >+#highlighter-veil-rightbox {
> >+  -moz-box-flex: 1;
> >+}
> 
> Aren't some or even all of these styles no-ops?

I have to keep flex:1, but beside that, I can get rid
of all of these styles.

> >+#highlighter-veil {
> >+  -moz-box-orient: vertical;
> >+}
> 
> >+#highlighter-veil-middlebox {
> >+  -moz-box-orient: horizontal;
> >+}
> 
> Why aren't these hboxes and vboxes, respectively?

Fixed.

> >+++ b/browser/themes/winstripe/browser/browser.css	Mon Jun 20 16:35:18 2011 +0200
> 
> >+#highlighter-close-button {
> >+  background-image: url("chrome://browser/skin/KUI-close.png");
> >+  width: 24px;
> >+  height: 24px;
> >+  position: absolute;
> >+  top: 12px;
> >+  right: 12px;
> >+  cursor: pointer;
> >+}
> 
> position: absolute; should probably be in content CSS as well.

Done.
Comment 17 Dão Gottwald [:dao] 2011-06-20 16:03:28 PDT
Comment on attachment 540490 [details] [diff] [review]
patch v1.6

>+++ b/browser/base/content/highlighter.css	Mon Jun 20 18:17:06 2011 +0200

>+#highlighter-container {
>+  -moz-user-focus: ignore;
>+  pointer-events: none;
>+  overflow: hidden;
>+}
>+
>+#highlighter-veil {
>+  vertical-align: top;
>+  overflow: hidden;
>+  -moz-box-flex: 1;
>+}

Does overflow:hidden really make a difference here?

>+#highlighter-controls {
>+  position: absolute;
>+  top: 0;
>+  left: 0;
>+}

Is this right for right-to-left?

>+.highlighter-veil,
>+#highlighter-veil-middlebox,
>+#highlighter-veil-transparentbox {
>+  -moz-transition: 0.1s;
>+  -moz-transition-timing-function: linear;
>+}

What exactly is supposed to transition here?

>+#highlighter-veil-topbox,
>+#highlighter-veil-bottombox {
>+}

This should be dropped since it's empty.

>+#highlighter-veil-bottombox {
>+  -moz-box-flex: 1;
>+}
>+
>+#highlighter-veil-rightbox {
>+  -moz-box-flex: 1;
>+}

These can be combined.
Comment 18 Paul Rouget [:paul] 2011-06-20 17:12:21 PDT
Created attachment 540631 [details] [diff] [review]
patch v1.7

Dao, thanks for all these reviews. Hopefully, this one is the good one :)

(In reply to comment #17)
> Comment on attachment 540490 [details] [diff] [review] [review]
> patch v1.6
> 
> >+++ b/browser/base/content/highlighter.css	Mon Jun 20 18:17:06 2011 +0200
> 
> >+#highlighter-container {
> >+  -moz-user-focus: ignore;
> >+  pointer-events: none;
> >+  overflow: hidden;
> >+}
> >+
> >+#highlighter-veil {
> >+  vertical-align: top;
> >+  overflow: hidden;
> >+  -moz-box-flex: 1;
> >+}
> 
> Does overflow:hidden really make a difference here?

Only need the one in #highlighter-veil.
Fixed.

> >+#highlighter-controls {
> >+  position: absolute;
> >+  top: 0;
> >+  left: 0;
> >+}
> 
> Is this right for right-to-left?

Yes. Since the middle-veil is reversed, it works.

> >+.highlighter-veil,
> >+#highlighter-veil-middlebox,
> >+#highlighter-veil-transparentbox {
> >+  -moz-transition: 0.1s;
> >+  -moz-transition-timing-function: linear;
> >+}
> 
> What exactly is supposed to transition here?

The width and height of all these boxes.
Updated with explicit transitions.

> >+#highlighter-veil-topbox,
> >+#highlighter-veil-bottombox {
> >+}
> 
> This should be dropped since it's empty.

Fixed.

> >+#highlighter-veil-bottombox {
> >+  -moz-box-flex: 1;
> >+}
> >+
> >+#highlighter-veil-rightbox {
> >+  -moz-box-flex: 1;
> >+}
> 
> These can be combined.

Fixed.
Comment 19 Paul Rouget [:paul] 2011-06-21 02:20:54 PDT
Comment on attachment 540631 [details] [diff] [review]
patch v1.7

Cancelling review. Found bugs (and s/properties/property/).
Comment 20 Paul Rouget [:paul] 2011-06-21 04:38:18 PDT
Created attachment 540710 [details] [diff] [review]
patch v1.8

Should be ok now.

Dao, please see comment 18 for more details. Also, look at content/browser.css. I add highlighter.css via a %include. Do you think it's ok?

Mihai, the main container was a <box>, but then the absolute controlsBox wasn't able to expand horizontally. The only way to fix that was to make the <box> absolute too, but then, the veilBox (flexible) wasn't able to expand vertically.
Using a <stack> fixes the problem.
Comment 21 Dietrich Ayala (:dietrich) 2011-06-23 10:06:17 PDT
Comment on attachment 540710 [details] [diff] [review]
patch v1.8

dao's already done a couple of iterations on this, so removing myself.
Comment 22 Dão Gottwald [:dao] 2011-06-26 14:40:16 PDT
Comment on attachment 540710 [details] [diff] [review]
patch v1.8

For RTL, the close button should be on the left, not on the right.

>+#highlighter-container {
>+  -moz-user-focus: ignore;

Why? I don't see anything in there that could gain focus.

>+#highlighter-veil {
>+  vertical-align: top;
>+  overflow: hidden;

I don't understand what these two properties are doing here. Please annotate them with comments.

>+.highlighter-veil,
>+#highlighter-veil-middlebox,
>+#highlighter-veil-transparentbox {
>+  -moz-transition-property: width, height;
>+  -moz-transition-duration: 0.1s;
>+  -moz-transition-timing-function: linear;
>+
>+}

nit: redundant empty line

>+    /*
>+      <box id="highlighter-veil">
>+        <box id="highlighter-veil-topbox" class="highlighter-veil"/>
>+        <box id="highlighter-veil-middlebox">
>+          <box id="highlighter-veil-leftbox" class="highlighter-veil"/>
>+          <box id="highlighter-veil-transparentbox"/>
>+          <box id="highlighter-veil-rightbox" class="highlighter-veil"/>
>+        </box>
>+        <box id="highlighter-veil-bottombox" class="highlighter-veil"/>
>+      </box>
>+    */

This needs an update for the vbox and hbox.

>+    let closeButton = document.createElement("box");

Why isn't this an <image>?
Comment 23 Paul Rouget [:paul] 2011-06-27 06:10:45 PDT
(In reply to comment #22)
> >+  vertical-align: top;
> >+  -moz-user-focus: ignore;
> 
> Why? I don't see anything in there that could gain focus.

Both were useful when we were using the <iframe>.
I'll drop them.

> >+#highlighter-veil {
> >+  vertical-align: top;
> >+  overflow: hidden;
> 
> I don't understand what these two properties are doing here. Please annotate
> them with comments.

Ok.

> >+.highlighter-veil,
> >+#highlighter-veil-middlebox,
> >+#highlighter-veil-transparentbox {
> >+  -moz-transition-property: width, height;
> >+  -moz-transition-duration: 0.1s;
> >+  -moz-transition-timing-function: linear;
> >+
> >+}
> 
> nit: redundant empty line

Ok.

> >+    /*
> >+      <box id="highlighter-veil">
> >+        <box id="highlighter-veil-topbox" class="highlighter-veil"/>
> >+        <box id="highlighter-veil-middlebox">
> >+          <box id="highlighter-veil-leftbox" class="highlighter-veil"/>
> >+          <box id="highlighter-veil-transparentbox"/>
> >+          <box id="highlighter-veil-rightbox" class="highlighter-veil"/>
> >+        </box>
> >+        <box id="highlighter-veil-bottombox" class="highlighter-veil"/>
> >+      </box>
> >+    */
> 
> This needs an update for the vbox and hbox.

Ok.

> >+    let closeButton = document.createElement("box");
> 
> Why isn't this an <image>?

Because it was originally a <box>. So it should be an image?
Comment 24 Dão Gottwald [:dao] 2011-06-27 06:12:20 PDT
As far as I can tell, yes, it should be an image.
Comment 25 Paul Rouget [:paul] 2011-06-27 07:43:14 PDT
Created attachment 542155 [details] [diff] [review]
patch v1.9

position:absolute doesn't work if I use <image> (but works with <box> and <img>). Keeping <box> in this new version of the patch. Other comments addressed.
Comment 26 Rob Campbell [:rc] (:robcee) 2011-06-27 09:08:03 PDT
Comment on attachment 542155 [details] [diff] [review]
patch v1.9

 /**
- * A highlighter mechanism using a transparent xul iframe.
+ * A highlighter mechanism

nit: should use proper punctuation for doc comments.

-    div.setAttribute("style", "pointer-events: none; -moz-user-focus: ignore");
+    this.highlighterContainer = document.createElement("stack");
+    this.highlighterContainer.id = "highlighter-container";
...
-    }).bind(this), true);
+    stack.appendChild(this.highlighterContainer);

Creating a stack within a stack seems... unusual.
 
   /**
-   * Destroy the iframe and its nodes.
+   * Build the veil

nit: punctuation.

+    /*
+      <vbox id="highlighter-veil">
+        <box id="highlighter-veil-topbox" class="highlighter-veil"/>
+        <hbox id="highlighter-veil-middlebox">
+          <box id="highlighter-veil-leftbox" class="highlighter-veil"/>
+          <box id="highlighter-veil-transparentbox"/>
+          <box id="highlighter-veil-rightbox" class="highlighter-veil"/>
+        </hbox>
+        <box id="highlighter-veil-bottombox" class="highlighter-veil"/>
+      </vbox>
+    */

If this is intended as documentation, you might consider including it in the jsdoc comment preceding the method. I think I'd like that as it serves as a good reference for structure that isn't otherwise available in browser.xul.

Speaking of browser.xul, you might want to mention where these nodes live with respect to the browser.xul document.

in buildVeil...

+    this.veilLeftBox = document.createElement("box");
...
+    let veilRightBox = document.createElement("box");

you're creating a reference to the leftbox but not the rightbox in the highlighter. How come? Both are contained in veilMiddleBox so you could access them through that if needed. If needed, perhaps a comment explaining why.

+  /**
+   * Build the controls
+   * @param nsIDOMNode aParent
+   */
+  buildControls: function Highlighter_buildControls(aParent)

more comment nits: Punctuation (here and elsewhere). Also, what are the controls for? Maybe a bit of description would be nice.

in highlightRectangle, the reason for caching leftBox becomes clearer:

-      this.veilMiddleDiv.style.height = aRect.height + "px";
-      this.veilTransparentDiv.style.width = aRect.width + "px";
+      this.veilTopBox.style.height = aRect.top + "px";
+      this.veilLeftBox.style.width = aRect.left + "px";

overall, the changes to the inspector.js file are decent and very clean. All of my nits were comment-related. :)
Comment 27 Dão Gottwald [:dao] 2011-06-27 11:01:28 PDT
Comment on attachment 542155 [details] [diff] [review]
patch v1.9

The close button is still on the right side for RTL.

>+    let closeButton = document.createElement("box");
>+    closeButton.id = "highlighter-close-button";

This works over here:

    let closeButton = document.createElement("box");
    closeButton.id = "highlighter-close-button";
    closeButton.appendChild(document.createElement("image"));

#highlighter-close-button {
  list-style-image: url("chrome://browser/skin/KUI-close.png");
  top: 12px;
  right: 12px;
  cursor: pointer;
}

>+    this.listenOnce(closeButton, "click",
>+      InspectorUI.closeInspectorUI.bind(InspectorUI, false), false);

This can be just closeButton.setAttribute("onclick", "InspectorUI.closeInspectorUI(false);");, at which point I think you can remove listenOnce since it's unused.
Comment 28 Paul Rouget [:paul] 2011-06-28 02:58:20 PDT
(In reply to comment #26)
> Comment on attachment 542155 [details] [diff] [review] [review]
> nit: should use proper punctuation for doc comments.

ok

> +    this.highlighterContainer = document.createElement("stack");
> +    this.highlighterContainer.id = "highlighter-container";
> ...
> -    }).bind(this), true);
> +    stack.appendChild(this.highlighterContainer);
> 
> Creating a stack within a stack seems... unusual.

This is the only "clean" way to have a the controls (position:absolute) and
the veil (display:-moz-box) stacked. Using a normal <box> creates some problems
(can't have the ctrlsBox on top of the flex box if absolute).

> +    /*
> +      <vbox id="highlighter-veil">
> +        <box id="highlighter-veil-topbox" class="highlighter-veil"/>
> +        <hbox id="highlighter-veil-middlebox">
> +          <box id="highlighter-veil-leftbox" class="highlighter-veil"/>
> +          <box id="highlighter-veil-transparentbox"/>
> +          <box id="highlighter-veil-rightbox" class="highlighter-veil"/>
> +        </hbox>
> +        <box id="highlighter-veil-bottombox" class="highlighter-veil"/>
> +      </vbox>
> +    */
> 
> If this is intended as documentation, you might consider including it in the
> jsdoc comment preceding the method. I think I'd like that as it serves as a
> good reference for structure that isn't otherwise available in browser.xul.

ok

> Speaking of browser.xul, you might want to mention where these nodes live
> with respect to the browser.xul document.

I'll add a comment in browser.xul to reflect the DOM we build in the highlighter.

> in buildVeil...
> 
> +    this.veilLeftBox = document.createElement("box");
> ...
> +    let veilRightBox = document.createElement("box");
> 
> you're creating a reference to the leftbox but not the rightbox in the
> highlighter. How come? Both are contained in veilMiddleBox so you could
> access them through that if needed. If needed, perhaps a comment explaining
> why.

I'll add some comments.

> Also, what are the
> controls for? Maybe a bit of description would be nice.

controlsBox will host all the interactive elements of the highlighter (buttons,
toolbars, ...). I'll add some comments.
Comment 29 Paul Rouget [:paul] 2011-06-28 03:04:30 PDT
(In reply to comment #27)
> Comment on attachment 542155 [details] [diff] [review] [review]
> patch v1.9
> 
> The close button is still on the right side for RTL.
> 
> >+    let closeButton = document.createElement("box");
> >+    closeButton.id = "highlighter-close-button";
> 
> This works over here:
> 
>     let closeButton = document.createElement("box");
>     closeButton.id = "highlighter-close-button";
>     closeButton.appendChild(document.createElement("image"));
> 
> #highlighter-close-button {
>   list-style-image: url("chrome://browser/skin/KUI-close.png");
>   top: 12px;
>   right: 12px;
>   cursor: pointer;
> }

Here you add an <image> inside the <box>, and you're styling the <box>.
I can add an <image> inside the <box>, but I'm not sure to understand why
it would be useful.

> >+    this.listenOnce(closeButton, "click",
> >+      InspectorUI.closeInspectorUI.bind(InspectorUI, false), false);
> 
> This can be just closeButton.setAttribute("onclick",
> "InspectorUI.closeInspectorUI(false);");, at which point I think you can
> remove listenOnce since it's unused.

Ok!
Comment 30 Paul Rouget [:paul] 2011-06-28 03:59:54 PDT
Created attachment 542414 [details] [diff] [review]
patch v1.10

Addressed comment 26 and comment 27.
Comment 31 Dão Gottwald [:dao] 2011-06-28 09:17:35 PDT
Comment on attachment 542414 [details] [diff] [review]
patch v1.10

>--- a/browser/base/content/browser.xul	Tue Jun 28 10:50:29 2011 +0200
>+++ b/browser/base/content/browser.xul	Tue Jun 28 12:57:33 2011 +0200
>@@ -1024,15 +1024,15 @@
> #endif
> 
> </vbox>
>-# <iframe id="highlighter-frame"
>-#   transparent="true"
>-#   type="content"
>-#   src="chrome://content/base/highlighter.html"/> is dynamically appended as
>-#     the last child of #tab-view-deck, only when it is needed, for minimal
>-#     performance impact.
> # <iframe id="tab-view"> is dynamically appended as the 2nd child of #tab-view-deck.
> #     Introducing the iframe dynamically, as needed, was found to be better than
> #     starting with an empty iframe here in browser.xul from a Ts standpoint.
>+#
>+# The hilighter is built dynamically once the Inspector is invoked:
>+# <stack id="highlighter-container">
>+#   <vbox id="highlighter-veil">...</vbox>
>+#   <box id="highlighter-controls>...</vbox>
>+# </stack>
> </deck>

That's not actually the place you're inserting the highlighter at.

>+++ b/browser/base/content/highlighter.css	Tue Jun 28 12:57:33 2011 +0200
>@@ -0,0 +1,38 @@
>+#highlighter-container {
>+  pointer-events: none;
>+}
>+
>+#highlighter-veil {
>+  /* The container can be shrunk in case of scrollbars */
>+  overflow: hidden;

Sorry, it's still not clear to me what this means. What scrollbars? Somewhere in content, somewhere in chrome? None of the elements you're creating should get scrollbars, AFAIK, since overflow defaults to 'visible'.

>+   * <vbox id="highlighter-veil">
>+   *   <box id="highlighter-veil-topbox" class="highlighter-veil"/>
>+   *   <hbox id="highlighter-veil-middlebox">
>+   *     <box id="highlighter-veil-leftbox" class="highlighter-veil"/>
>+   *     <box id="highlighter-veil-transparentbox"/>
>+   *     <box id="highlighter-veil-rightbox" class="highlighter-veil"/>
>+   *   </hbox>
>+   *   <box id="highlighter-veil-bottombox" class="highlighter-veil"/>
>+   * </vbox>

Slightly confusing that there's a highlighter-veil id and some other nodes have a highlighter-veil class. Can you find a better class name? highlighter-veil-component?
Comment 32 Paul Rouget [:paul] 2011-06-28 10:10:42 PDT
(In reply to comment #31)
> Comment on attachment 542414 [details] [diff] [review] [review]
> patch v1.10
> 
> >--- a/browser/base/content/browser.xul	Tue Jun 28 10:50:29 2011 +0200
> >+++ b/browser/base/content/browser.xul	Tue Jun 28 12:57:33 2011 +0200
> >@@ -1024,15 +1024,15 @@
> > #endif
> > 
> > </vbox>
> >-# <iframe id="highlighter-frame"
> >-#   transparent="true"
> >-#   type="content"
> >-#   src="chrome://content/base/highlighter.html"/> is dynamically appended as
> >-#     the last child of #tab-view-deck, only when it is needed, for minimal
> >-#     performance impact.
> > # <iframe id="tab-view"> is dynamically appended as the 2nd child of #tab-view-deck.
> > #     Introducing the iframe dynamically, as needed, was found to be better than
> > #     starting with an empty iframe here in browser.xul from a Ts standpoint.
> >+#
> >+# The hilighter is built dynamically once the Inspector is invoked:
> >+# <stack id="highlighter-container">
> >+#   <vbox id="highlighter-veil">...</vbox>
> >+#   <box id="highlighter-controls>...</vbox>
> >+# </stack>
> > </deck>
> 
> That's not actually the place you're inserting the highlighter at.

Yeah, I didn't think about this (just replaced the previous code).
I don't think I should add this to browser.xul. The code is added in the stack
(browserStack) inside the tabpanels. I will move this comment to inspector.js.
 
> >+++ b/browser/base/content/highlighter.css	Tue Jun 28 12:57:33 2011 +0200
> >@@ -0,0 +1,38 @@
> >+#highlighter-container {
> >+  pointer-events: none;
> >+}
> >+
> >+#highlighter-veil {
> >+  /* The container can be shrunk in case of scrollbars */
> >+  overflow: hidden;
> 
> Sorry, it's still not clear to me what this means. What scrollbars?
> Somewhere in content, somewhere in chrome? None of the elements you're
> creating should get scrollbars, AFAIK, since overflow defaults to 'visible'.

If we don't use this, we see the veil on top of the web page scrollbars (if any):

The web page can have scrollbars. And we don't want the veil to cover web page's scrollbars (because
we want the user to be able to use them). So we add a padding to the container, which is pointer-events:none,
so the user can use the scrollbars. But the veil boxes will overflow in this case (we don't take the size
of the scrollbars when we compute the dirtyRect of the bounding box), and the overflow is "visible", so we
see the dark veil on top of the scrollbars.

Hope it's clear.

> >+   * <vbox id="highlighter-veil">
> >+   *   <box id="highlighter-veil-topbox" class="highlighter-veil"/>
> >+   *   <hbox id="highlighter-veil-middlebox">
> >+   *     <box id="highlighter-veil-leftbox" class="highlighter-veil"/>
> >+   *     <box id="highlighter-veil-transparentbox"/>
> >+   *     <box id="highlighter-veil-rightbox" class="highlighter-veil"/>
> >+   *   </hbox>
> >+   *   <box id="highlighter-veil-bottombox" class="highlighter-veil"/>
> >+   * </vbox>
> 
> Slightly confusing that there's a highlighter-veil id and some other nodes
> have a highlighter-veil class. Can you find a better class name?
> highlighter-veil-component?

You're right, will change that to "highlighter-dark-veil"
Comment 33 Paul Rouget [:paul] 2011-06-28 10:40:14 PDT
Created attachment 542515 [details] [diff] [review]
patch v1.11
Comment 34 Rob Campbell [:rc] (:robcee) 2011-06-28 11:49:47 PDT
also need to remove references to highlighter.css in the various jar.mn files. I found another one pushing to try in themes/winstripe/browser/jar.mn.
Comment 35 Paul Rouget [:paul] 2011-06-28 12:03:46 PDT
Good catch. That should be the only one.
Comment 36 Paul Rouget [:paul] 2011-06-28 12:05:36 PDT
Hmmm, there's still some references to inspector.xhtml.
Comment 37 Paul Rouget [:paul] 2011-06-28 12:06:17 PDT
and this is normal :)
Comment 38 Paul Rouget [:paul] 2011-06-28 12:09:06 PDT
Created attachment 542548 [details] [diff] [review]
patch v1.12

removed the reference to highlighter.css from jar.mn
Comment 39 Dão Gottwald [:dao] 2011-06-29 05:38:40 PDT
Comment on attachment 542548 [details] [diff] [review]
patch v1.12

The close button is *still* on the right side for RTL.
Comment 40 Dão Gottwald [:dao] 2011-06-29 05:40:25 PDT
(In reply to comment #32)
> If we don't use this, we see the veil on top of the web page scrollbars (if
> any):
> 
> The web page can have scrollbars. And we don't want the veil to cover web
> page's scrollbars (because
> we want the user to be able to use them). So we add a padding to the
> container, which is pointer-events:none,

This is horrible and shouldn't have passed reviews. As far as I can tell, it doesn't actually work :(
Comment 41 Paul Rouget [:paul] 2011-06-29 06:17:51 PDT
(In reply to comment #40)
> (In reply to comment #32)
> > If we don't use this, we see the veil on top of the web page scrollbars (if
> > any):
> > 
> > The web page can have scrollbars. And we don't want the veil to cover web
> > page's scrollbars (because
> > we want the user to be able to use them). So we add a padding to the
> > container, which is pointer-events:none,
> 
> This is horrible and shouldn't have passed reviews.

I'm not sure to understand why this is horrible. Can you explain me?
What is the right approach then?

(this behavior has been introduced in bug 642471)

> As far as I can tell, it doesn't actually work :(

How can I reproduce this?

To test this I go to yahoo.com, resize the browser to make the scrollbar
show up, then highlight an element "cut" by the scrollbar.

I can still use the scrollbar and the veil is not visible on top of the scrollbar.
Comment 42 Dão Gottwald [:dao] 2011-06-29 07:26:55 PDT
(In reply to comment #41)
> I'm not sure to understand why this is horrible. Can you explain me?

First of all the way the scrollbar width is calculated is a hack (putting temporary junk into browser.xul etc.).

> What is the right approach then?

It's not clear to me that there can be a right approach, given that scrollable areas can appear anywhere on a page, or that a scrollbar could be on the right without spanning the entire height... The most consistent and solid solution might actually be to cover scrollbars and let the user use the keyboard / scroll wheel or temporarily disable the highlighter in order to use scrollbars.

> (this behavior has been introduced in bug 642471)
> 
> > As far as I can tell, it doesn't actually work :(
> 
> How can I reproduce this?
> 
> To test this I go to yahoo.com, resize the browser to make the scrollbar
> show up, then highlight an element "cut" by the scrollbar.
> 
> I can still use the scrollbar and the veil is not visible on top of the
> scrollbar.

When I tested this on Linux, the veil covered part of the scrollbar and the scroll thumb couldn't be dragged.
Comment 43 Mihai Sucan [:msucan] 2011-06-29 08:24:52 PDT
(In reply to comment #42)
> (In reply to comment #41)
> > I'm not sure to understand why this is horrible. Can you explain me?
> 
> First of all the way the scrollbar width is calculated is a hack (putting
> temporary junk into browser.xul etc.).

True, but we use this in another place as well, already. It's ugly, but it works. I would really appreciate an improved approach.

> > What is the right approach then?
> 
> It's not clear to me that there can be a right approach, given that
> scrollable areas can appear anywhere on a page, or that a scrollbar could be
> on the right without spanning the entire height... The most consistent and
> solid solution might actually be to cover scrollbars and let the user use
> the keyboard / scroll wheel or temporarily disable the highlighter in order
> to use scrollbars.

The intent wasn't to not cover all scrollbars, all scrollables. We only wanted to avoid covering the main page scrollbars.

I believe we can improve the code to handle RTL languages.


> > (this behavior has been introduced in bug 642471)
> > 
> > > As far as I can tell, it doesn't actually work :(
> > 
> > How can I reproduce this?
> > 
> > To test this I go to yahoo.com, resize the browser to make the scrollbar
> > show up, then highlight an element "cut" by the scrollbar.
> > 
> > I can still use the scrollbar and the veil is not visible on top of the
> > scrollbar.
> 
> When I tested this on Linux, the veil covered part of the scrollbar and the
> scroll thumb couldn't be dragged.

Uh oh.

Given we have changed the code from the initial "catch all" events approach, to something more usable, sure, we can remove this hack, if you consider it's not worth the trouble.

Please let us know what you consider is best we do. Thank you!
Comment 44 Rob Campbell [:rc] (:robcee) 2011-06-29 08:31:23 PDT
It doesn't seem to work on Mac either. The scrollbars are obscured by the veil. We can still use the scrollbars though, the thumb and scrollbar areas react to mouse clicks as you would expect.

One alternative hack if we wanted to display the scrollbars could be to create them in xul, though we'd have to also match the scrollbar width and height if altered. I think I'd prefer to just cover them with the veil for now and if we feel there's a need later on to fix it in a followup.
Comment 45 Dão Gottwald [:dao] 2011-06-29 09:21:57 PDT
+1 for removal

> True, but we use this in another place as well, already.

This only makes it worse ;)

> The intent wasn't to not cover all scrollbars, all scrollables. We only
> wanted to avoid covering the main page scrollbars.

Right, but this is inconsistent... Building up user expectations and satisfying them only sometimes isn't great.
Comment 46 Paul Rouget [:paul] 2011-06-30 05:51:31 PDT
Created attachment 543111 [details] [diff] [review]
patch v1.13

Changes in v1.13:
× in highlight(), we were checking if we were highlighting the same rectangle, but we didn't take into account that the rectangle could be the same but the size of the window different. I moved this test in highlightRectangle().
× moved closeButton to the left if rtl.
× removed the scrollbars hack

I keep 'overflow:hidden' for the veil because: when the window is resized and then "cut" the highlighted rectangle, we resize the highlighted rectangle, but we do it on the "resize" event, and the window has already been resized, and the veil, which is too big at this time, will overflow (we see a new scrollbar).

Does it make sense? Not sure I'm clear.
Comment 47 Dão Gottwald [:dao] 2011-06-30 05:55:12 PDT
(In reply to comment #46)
> I keep 'overflow:hidden' for the veil because: when the window is resized
> and then "cut" the highlighted rectangle, we resize the highlighted
> rectangle, but we do it on the "resize" event, and the window has already
> been resized, and the veil, which is too big at this time, will overflow (we
> see a new scrollbar).

I don't think that's true... In order to get a scrollbar, #highlighter-veil would need to default to overflow:auto, which it shouldn't...
Comment 48 Paul Rouget [:paul] 2011-06-30 06:10:17 PDT
The veil is too large, so highlighter-container is too large, so the browserStack is too large, so the notificationbox overflow:auto (scrollbars).
Comment 49 Dão Gottwald [:dao] 2011-06-30 06:21:29 PDT
So the notificationbox has overflow-y:hidden which implies overflow-x:auto. I don't think we want that. Can you file a bug on it rather than working around it here?
Comment 50 Paul Rouget [:paul] 2011-06-30 06:56:06 PDT
(In reply to comment #49)
> So the notificationbox has overflow-y:hidden which implies overflow-x:auto.
> I don't think we want that. Can you file a bug on it rather than working
> around it here?

bug
Comment 51 Paul Rouget [:paul] 2011-06-30 06:57:17 PDT
(In reply to comment #49)
> So the notificationbox has overflow-y:hidden which implies overflow-x:auto.
> I don't think we want that. Can you file a bug on it rather than working
> around it here?

bug 668500
Comment 52 Dão Gottwald [:dao] 2011-07-01 09:10:18 PDT
Comment on attachment 543111 [details] [diff] [review]
patch v1.13

>+#highlighter-veil {
>+  overflow: hidden;

Please verify that this isn't needed anymore, and if so, remove it.
Comment 53 Paul Rouget [:paul] 2011-07-04 10:24:13 PDT
Created attachment 543800 [details] [diff] [review]
patch v1.14

(In reply to comment #52)
> Comment on attachment 543111 [details] [diff] [review] [review]
> patch v1.13
> 
> >+#highlighter-veil {
> >+  overflow: hidden;
> 
> Please verify that this isn't needed anymore, and if so, remove it.

It works well now. I removed it.
Comment 54 Dão Gottwald [:dao] 2011-07-04 11:02:37 PDT
Comment on attachment 543800 [details] [diff] [review]
patch v1.14

>+#highlighter-veil
>+#highlighter-veil-bottombox,
>+#highlighter-veil-rightbox {
>+  -moz-box-flex: 1;
>+}

This lacks a comma. Maybe #highlighter-veil doesn't actually need -moz-box-flex: 1;?

Please rename the "highlighter-veil" id to "highlighter-veil-container" and the "highlighter-opaque-veil" class to "highlighter-veil".

Please make the change to the close button requested in comment 27.
Comment 55 Paul Rouget [:paul] 2011-07-04 11:17:11 PDT
(In reply to comment #54)
> Comment on attachment 543800 [details] [diff] [review] [review]
> patch v1.14
> 
> >+#highlighter-veil
> >+#highlighter-veil-bottombox,
> >+#highlighter-veil-rightbox {
> >+  -moz-box-flex: 1;
> >+}
> 
> This lacks a comma. Maybe #highlighter-veil doesn't actually need
> -moz-box-flex: 1;?

Oops. Maybe, I'll check that.

> Please rename the "highlighter-veil" id to "highlighter-veil-container" and
> the "highlighter-opaque-veil" class to "highlighter-veil".

Ok.

> Please make the change to the close button requested in comment 27.

I'm not sure to understand why we should add a <button> inside the <box>.
And you're styling the <box>, not the <image>. It looks like
you're adding an invisible <image> tag. Can you explain?
Comment 56 Dão Gottwald [:dao] 2011-07-04 11:26:20 PDT
<image> will inherit the list-style-image. list-style-image itself has no effect on the <box>.
Comment 57 Paul Rouget [:paul] 2011-07-05 05:45:18 PDT
(In reply to comment #53)
> Created attachment 543800 [details] [diff] [review] [review]
> patch v1.14
> 
> (In reply to comment #52)
> > Comment on attachment 543111 [details] [diff] [review] [review] [review]
> > patch v1.13
> > 
> > >+#highlighter-veil {
> > >+  overflow: hidden;
> > 
> > Please verify that this isn't needed anymore, and if so, remove it.
> 
> It works well now. I removed it.

Not really actually. There's no scrollbars anymore, but:

Once the window is less large than the #highlighter-veil, #highlighter-veil prevents its container to be resized as well. This container also contains the close-button (which is position:absolute;right:-12px), so the close-button disappears from the window.

Using overflow:hidden on the #highlighter-veil fixes the problem.

Do you think we should keep overflow:hidden then or try another approach?
Comment 58 Paul Rouget [:paul] 2011-07-06 04:56:09 PDT
Created attachment 544202 [details] [diff] [review]
patch v1.15

(In reply to comment #55)
> (In reply to comment #54)
> > Comment on attachment 543800 [details] [diff] [review] [review] [review]
> > patch v1.14
> > 
> > >+#highlighter-veil
> > >+#highlighter-veil-bottombox,
> > >+#highlighter-veil-rightbox {
> > >+  -moz-box-flex: 1;
> > >+}
> > 
> > This lacks a comma. Maybe #highlighter-veil doesn't actually need
> > -moz-box-flex: 1;?

Not needed. Removed.

> > Please rename the "highlighter-veil" id to "highlighter-veil-container" and
> > the "highlighter-opaque-veil" class to "highlighter-veil".
> 
> Ok.

Done.

> > Please make the change to the close button requested in comment 27.

Done.


2 questions:
→ I still need to use overflow:hidden (see comment 57). Do you think we should try another approach?
→ The close button is on the top-right corner (in ltr). Should it be on the top-left corner on Mac?
Comment 59 Dão Gottwald [:dao] 2011-07-08 05:01:58 PDT
Comment on attachment 544202 [details] [diff] [review]
patch v1.15

>+    this.browser.parentNode.removeChild(this.highlighterContainer);

less fragile:

this.highlighterContainer.parentNode.removeChild(this.highlighterContainer);
Comment 60 Dão Gottwald [:dao] 2011-07-08 05:02:34 PDT
(In reply to comment #58)
> → The close button is on the top-right corner (in ltr). Should it be on the
> top-left corner on Mac?

No idea...
Comment 61 Paul Rouget [:paul] 2011-07-08 07:15:12 PDT
Created attachment 544806 [details] [diff] [review]
patch v1.16
Comment 62 Paul Rouget [:paul] 2011-07-08 07:20:46 PDT
Thank you dao.
Comment 63 Dão Gottwald [:dao] 2011-07-11 15:20:10 PDT
http://hg.mozilla.org/mozilla-central/rev/4c0e1c194152

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