Highlighter fails when width === 0 && height === 0 (elements could still have border, margin or padding)

RESOLVED FIXED in Firefox 33

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: miker, Assigned: miker)

Tracking

Trunk
Firefox 33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Created attachment 8434981 [details]
highlighter-transform-bug.html

Here is a beginning of a fix but we suspect that this is a getBoxQuads bug.

   _updateBoxModel: function(options) {
     options.region = options.region || "content";
-    this.rect = this.layoutHelpers.getAdjustedQuads(this.currentNode, "margin");
+    this.rect = this.layoutHelpers.getAdjustedQuads(this.currentNode, "content");
 
     if (!this.rect || (this.rect.bounds.width <= 0 && this.rect.bounds.height <= 0)) {
       return false;
     }
 
     for (let boxType in this._boxModelNodes) {
-      let {p1, p2, p3, p4} = boxType === "margin" ? this.rect :
+      let {p1, p2, p3, p4} = boxType === "content" ? this.rect :
Assignee: nobody → mratcliffe
Created attachment 8435797 [details] [diff] [review]
highlighter-fails-on-transforms-width-height-zero-1020984.patch

I decided that it is useful that bounds.width and bounds.height return 0 for regions that are not set as this can be used to see that they are not set.

I have stopped setting dom.send_after_paint_to_content to true when not testing, that was a bug that would cause the tools to be refreshed way too many times.

The changes are simple and easy to follow so the review should be fairly fast.

Anyhow, the highlighter now works fine in all situations, use the attached test file to check it.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=948f347e064d
Attachment #8435797 - Flags: review?(pbrosset)
Created attachment 8437522 [details] [diff] [review]
highlighter-fails-on-transforms-width-height-zero-1020984.patch

Added test.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=a7f8b7b79478
Attachment #8435797 - Attachment is obsolete: true
Attachment #8435797 - Flags: review?(pbrosset)
Attachment #8437522 - Flags: review?(pbrosset)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)
> Created attachment 8435797 [details] [diff] [review]
> highlighter-fails-on-transforms-width-height-zero-1020984.patch
> 
> I decided that it is useful that bounds.width and bounds.height return 0 for
> regions that are not set as this can be used to see that they are not set.
I think you're right, this is likely not a getBoxQuads bug, but rather just the way it's supposed to work.
> I have stopped setting dom.send_after_paint_to_content to true when not
> testing, that was a bug that would cause the tools to be refreshed way too
> many times.
Good thing.
> The changes are simple and easy to follow so the review should be fairly
> fast.
Sorry for the delay, taking a look at the patch now.
Comment on attachment 8437522 [details] [diff] [review]
highlighter-fails-on-transforms-width-height-zero-1020984.patch

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

I've tried the patch, works great. And the code changes look good I think.
Only proposing a performance improvement to cache the computed styles and a quick refactor of the test.

::: browser/devtools/framework/gDevTools.jsm
@@ -57,5 @@
>        // testing/profiles/prefs_general.js so lets set it to the same as it is
>        // in a default browser profile for the duration of the test.
>        Services.prefs.setBoolPref("dom.send_after_paint_to_content", false);
> -    } else {
> -      Services.prefs.setBoolPref("dom.send_after_paint_to_content", true);

I had no idea we were setting that pref to true in this case. 
Good thing that you removed that.

::: browser/devtools/inspector/test/browser_inspector_highlighter.js
@@ +21,5 @@
> +      let basicDiv = doc.getElementById("basic");
> +      inspector = aInspector;
> +      inspector.selection.setNode(basicDiv, null);
> +
> +      inspector.once("inspector-updated", testMouseOverDivHighlights);

There's a helper function in head.js that you can use to avoid having to do this yourself: selectNode(selector, inspector) which returns a promise.
openInspector also returns a promise additionally, so, in order to make this test a little more consistent with the other tests I've recently re-written in styleinspector/markupview, you could rewrite that part as:

Task.spawn(function*() {
  let {inspector} = yield openInspector();
  yield selectNode("#basic", inspector);
});

And then, instead of having to store the inspector, doc, ... globally, I usually prefer passing it to each of the test case functions like so:

Task.spawn(function*() {
  let {inspector} = yield openInspector();
  yield selectNode("#basic", inspector);

  yield testMouseOverDivHighlights(content.document, inspector);
  yield testNonTransformedBoxModelDimensionsNoZoom(content.document, inspector);
  yield testNonTransformedBoxModelDimensionsZoomed(content.document, inspector);
  ... etc ...
});

This way, you can define your test case functions as generator (*) so they can yield promises too, which should help make the code easier to read.

I'm only saying this because you started to refactor a little bit this test by moving the markup outside into a separate support file, so why not take this opportunity to continue and make the file look more like the other ones?

@@ +68,5 @@
> +    isNodeCorrectlyHighlighted(rotatedDiv, "rotated");
> +
> +    testMouseOverWidthHeightZeroDiv();
> +  });
> +  inspector.selection.setNode(rotatedDiv);

Same here, the setNode + inspector-updated can be replaced with:
selectNode("#rotated", inspector).then(...
Or, if you make the function a generator, then you can directly go:
yield selectNode("#rotated", inspector);

@@ +82,4 @@
>  
>      executeSoon(finishUp);
>    });
> +  inspector.selection.setNode(zeroHeightWidthDiv);

same here.

::: toolkit/devtools/server/actors/highlighter.js
@@ +615,5 @@
> +    if (!this.currentNode) {
> +      return false;
> +    }
> +
> +    let style = this.win.getComputedStyle(this.currentNode);

Can you cache the style as long as this.currentNode doesn't change, to avoid accessing it over and over again at each paint event.
Attachment #8437522 - Flags: review?(pbrosset) → review+
Created attachment 8438419 [details] [diff] [review]
highlighter-fails-on-transforms-width-height-zero-1020984.patch

Addressed reviewer's comments... should be green on Try:
https://tbpl.mozilla.org/?tree=Try&rev=361fc645ab25
Attachment #8437522 - Attachment is obsolete: true
Attachment #8438419 - Flags: review+
Comment on attachment 8438419 [details] [diff] [review]
highlighter-fails-on-transforms-width-height-zero-1020984.patch

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

::: toolkit/devtools/server/actors/highlighter.js
@@ +627,5 @@
> +      return false;
> +    }
> +
> +    if (!this._computedStyle) {
> +      this._computedStyle = this.win.getComputedStyle(this.currentNode);

One thing I just realized (sorry I missed it in the previous review) is that if this.currentNode is in a nested frame, this may fail.
It should rather be 
this.currentNode.ownerDocument.defaultView.getComputedStyle(this.currentNode);
Created attachment 8442791 [details] [diff] [review]
highlighter-fails-on-width-height-zero-1020984.patch

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> Comment on attachment 8438419 [details] [diff] [review]
> highlighter-fails-on-transforms-width-height-zero-1020984.patch
> 
> Review of attachment 8438419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +627,5 @@
> > +      return false;
> > +    }
> > +
> > +    if (!this._computedStyle) {
> > +      this._computedStyle = this.win.getComputedStyle(this.currentNode);
> 
> One thing I just realized (sorry I missed it in the previous review) is that
> if this.currentNode is in a nested frame, this may fail.
> It should rather be 
> this.currentNode.ownerDocument.defaultView.getComputedStyle(this.
> currentNode);

Fixed
Attachment #8438419 - Attachment is obsolete: true
Created attachment 8443483 [details] [diff] [review]
highlighter-fails-on-width-height-zero-1020984.patch

Rebased
Attachment #8442791 - Attachment is obsolete: true
Attachment #8443483 - Flags: review+
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/46d3f21b011f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Depends on: 1038651
You need to log in before you can comment on or make changes to this bug.