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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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 :

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: highlighter-fails-on-transforms-width-height-zero-1020984.patch

Added test.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=a7f8b7b79478

Comment 3

[Patrick Brosset <:pbro>]

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

[Patrick Brosset <:pbro>]

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.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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

Comment 6

[Patrick Brosset <:pbro>]

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

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: highlighter-fails-on-width-height-zero-1020984.patch

Rebased

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://hg.mozilla.org/integration/fx-team/rev/46d3f21b011f

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/46d3f21b011f