663778 - Implement box model highlighter

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Paul Rouget [:paul]]

Once a node is locked, we should draw the different layout regions:
× margins
× padding
× borders / scrollbars

Comment 1

[Paul Rouget [:paul]]

Attachment: screenshot firebug

Comment 2

[Paul Rouget [:paul]]

Attachment: screenshot google-chrome

Comment 3

[Paul Rouget [:paul]]

Attachment: patch v1

The layout boxes are drawn as soon as the user lock the node.
In the future, the behavior will probably different (only if the node is hovered? A toggle button?).

Comment 4

[Paul Rouget [:paul]]

Comment on attachment 538959 [details] [diff] [review]
patch v1

need tests.

Comment 5

[Mihai Sucan [:msucan]]

Comment on attachment 538959 [details] [diff] [review]
patch v1

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

Patch looks good!

Please address the comments and include a test. It would also be nice to include a screenshot of how this looks (you included screenshots from chrome and firebug ;) ).

::: browser/base/content/inspector.js
@@ +354,4 @@
>     */
>    lock: function IFH_lock()
>    {
> +    let bbox_rect = this._highlightOriginalRect;

bbox_rect is not really a usual variable name in the Mozilla coding style.

I would recommend something like bboxRect.

@@ +378,5 @@
> +   *        The bounding box of the selected node.
> +   * @param CSSStyleDeclaration style
> +   *        Computed-style of the selected node.
> +   */
> +  drawSelectionBox: function IFH_drawSelectionBox(bbox_rect, style)

Please use the "a" prefix for arguments.

aBoxRect, aStyle.

Also use a similar style for the rest of the variable names and function arguments.

Comment 6

[Paul Rouget [:paul]]

Attachment: screenshot Firefox: layout boxes + layout view

Comment 7

[Paul Rouget [:paul]]

Attachment: patch v2

Only asking for feedback right now. I need to rebase this patch (WIP).

The abstract view lie in a doorhanger. This is temporary. We want to host this tool in a sidebar (bug 674476).

About the UI, the only thing I want to change is the color of the node in the blueprint. I think we should re-use the purple design we have in the highlighter.

Comment 8

[Paul Rouget [:paul]]

Attachment: patch v2.1

Updated with tests.

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 549074 [details] [diff] [review]
patch v2.1

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

Patch looks fine, but there are still some problems that need to be addressed. The layout view loks awesome, and the text is readable on my system, yay!

Screenshot with some comments:
http://img.i7m.de/show/87p15.png

(sorry for the ugly callouts :) )

1. The doorhanger button is no longer clickable once the Layout view opens. I cannot close the view once it's open.

2. There's a regression that causes links I click in the page to be followed, when I click on a node that I want to lock on. All click-related events need to be canceled. The code that does this is now broken, unfortunately.

3. Margin indicators when the node is locked are barely visible. Please see the screenshot.

4. When I scroll the page the element position (coordinates info) also change. The padding and margin highlights do not properly update as well. There seem to be some problems with the code that calculates the window-relative coords of the element.

5. When I lock on a different element the layout view does not update.

6. Strings need to be localized.

7. Most of the Inspector tests fail to run with the patch applied.

Some of the problems might be caused by the minimal rebase I did. Am I missing some patches that would fix (some of) these problems?

Please also see the review comments below. Thank you!

::: browser/base/content/inspector.js
@@ +258,5 @@
> +                     left: clientRect.left,
> +                     width: clientRect.width,
> +                     height: clientRect.height};
> +
> +    let orgRect = {top: clientRect.top,

Please add a comment here that explains the reason you want two rect objects. Also please explain what is the purpose of the two properties, offsetX and offsetY.

Also please rename "orgRect" to something more clear.

@@ +933,5 @@
> +      document.getElementById("inspector-inspect-toolbutton").checked = false;
> +      this.detachPageListeners();
> +      this.inspecting = false;
> +      if (this.highlighter.node) {
> +        this.select(this.highlighter.node, true, !aPreventScroll);

This line throws. aPreventScroll is undefined.

@@ +1516,5 @@
> +///////////////////////////////////////////////////////////////////////////
> +//// Layout
> +
> +/**
> + * Layout View

Please add a comment here explaining what is Layout View.

@@ +1521,5 @@
> + *
> + * @param nsIDOMNode aBrowser
> + *        The xul:browser object for the content window being highlighted.
> + */
> +function LayoutView(aBrowser)

I would suggest renaming this to InspectorLayoutView, or ElementLayoutView.

@@ +1538,5 @@
> +
> +  /**
> +   * Open the panel, build the layout boxes and add observers.
> +   */
> +  onShow: function Layout_onShow(aNode)

aNode is unused.

@@ +1547,5 @@
> +    Services.obs.addObserver(this, INSPECTOR_NOTIFICATIONS.LOCKED, false);
> +    Services.obs.addObserver(this, INSPECTOR_NOTIFICATIONS.UNLOCKED, false);
> +
> +    let btn = document.getElementById("inspector-layout-toolbutton");
> +    let node = InspectorUI.selection;

This is unused.

@@ +1548,5 @@
> +    Services.obs.addObserver(this, INSPECTOR_NOTIFICATIONS.UNLOCKED, false);
> +
> +    let btn = document.getElementById("inspector-layout-toolbutton");
> +    let node = InspectorUI.selection;
> +    this.panel.openPopup(btn, "overlap", 0, 0, false, false);

"overlap" is the problem here with the doorhanger covering the Layout button.

Please see:

https://developer.mozilla.org/en/XUL/Attribute/popup.position

I think you want "after_start" here.

@@ +1583,5 @@
> +   */
> +  onLocked: function Layout_onLocked()
> +  {
> +    let node = InspectorUI.selection;
> +    let layout = this.compute(node,

node is not used in other places in this method.

Just do this.compute(InspectorUI.selection, ...).

@@ +1605,5 @@
> +   * When a node is selected, we do nothing.
> +   */
> +  onSelect: function Layout_onSelect(aNode)
> +  {
> +  },

This is empty, please cleanup.

@@ +1609,5 @@
> +  },
> +
> +
> +  /**
> +   * Event handler.

This is not a useful comment. Please explain what the event handler does.

@@ +1628,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Observers.

Same case as with handleEvent().

@@ +1643,5 @@
> +     }
> +   },
> +
> +  /**
> +   * Register the Layout tool

Please elaborate. Something like "Register the Layout tool into the Inspector".

@@ +1650,5 @@
> +  {
> +    InspectorUI.registerTool({
> +      id: "layout",
> +      context: this,
> +      label: "Layout",

Please make all of the new strings localizable.

Please see the patch from bug 566084. You need:

+XPCOMUtils.defineLazyGetter(InspectorUI, "strings", function () {
+  return Services.strings.
+         createBundle("chrome://browser/locale/inspector.properties");
+});

... and a new file:

browser/locales/en-US/chrome/browser/inspector.properties

... where you add your strings. For how to use the string bundle, see the patch from the aforementioned bug.

@@ +1799,5 @@
> +    layout.node.top = Math.round(rect.top + win.scrollY);
> +    layout.node.left = Math.round(rect.left + win.scrollX);
> +
> +    layout.offset.left = rect.offsetX - win.scrollX;
> +    layout.offset.top = rect.offsetY - win.scrollY;

This might be wrong.

In highlight() you compute rect.offsetX/Y (which there is orgRect) by adding the x/y values from all of the iframes. Here you use win.scrollY where win is a reference only to the window of the highlighted element. This is surprising because it seems to be mixing window.top coords with element iframe win.scrollX/Y. That is, rect.offsetX/Y coords are relative to the window.top, while win.scrollX/Y ... are just relative to the iframe where the highlighted element happens to be.

Maybe I am wrong, but this did immediately caught my eye when reading the code, before I even tested the scrolling issue. After reading the code I tested scrolling and saw the problems with the coords. ;)

@@ +1834,5 @@
> +
> +    mapLayout(this.layoutBordersBox, layout.borders);
> +    mapLayout(this.layoutPaddingBox, layout.padding);
> +
> +    let offsetY = layout.node.top + layout.offset.top;

Notice that:
layout.node.top = rect.top + win.scrollY;
layout.offset.top = rect.offsetY - win.scrollY;
(taken from compute())

So here offsetY is actually:
offsetY = rect.top + win.scrollY + rect.offsetY - win.scrollY;

(scrollY is canceled)

Is that on purpose?

::: browser/base/content/layout-view.html
@@ +36,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 ***** -->

This does not follow the correct boilerplate template. Please update according to:

http://www.mozilla.org/MPL/boilerplate-1.1/

@@ +39,5 @@
> +#
> +# ***** END LICENSE BLOCK ***** -->
> +
> +<meta charset="utf8">
> +<title>Layout View</title>

This needs to be an XHTML file with an associated DTD file for the localization of the strings used in the file.

Comment 10

[Kevin Dangoor]

Attachment: Firebug layout view

For what it's worth, I'm attaching a screenshot of Firebug's layout view (not the info presented around the node, but the info presented within Firebug's inspection interface). This is the info for the same node at limi.net as in the screenshot for the layout tool that Paul has built here.

Comment 11

[Paul Rouget [:paul]]

Not for later:
we could have 3 different view:
x square (like the current view)
x horizontal rect
x vertical rect

The ratio of the rect doesnt have to be big (1.5).

Could help to understand the abstract view.

Comment 12

[Rob Campbell [:rc] (:robcee)]

I like that idea. I think Kyle mentioned it last week too, suggesting we should mimic the aspect ratio of the selected node to make it apparent that the layout box is connected to the selection.

+1

Comment 13

[Paul Rouget [:paul]]

The Layout Panel will be implemented in bug 683954. This bug now depends on it.

Comment 14

[Paul Rouget [:paul]]

Designs update:
http://cl.ly/0o1Q2a3t0K1m2N3j180h/o
http://cl.ly/251w1u2Q190x332z2l2V/o

Comment 15

[Dave Camp (:dcamp)]

Should be able to get by with the Layout Panel for minotaur.

Comment 16

[Dave Camp (:dcamp)]

We're doing developer tool prioritization, filter on 'brontozaur' to ignore the spam.

Comment 17

[Paul Rouget [:paul]]

triage, filter on centaur

Comment 18

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

shorlander: Before I start this we need to decide on the UI. Personally, I like the way Firebug displays the box model on the page when a HTML node is highlighted in the HTML panel or the layout panel is hovered.

How do you think we should show the box model in the highlighter?

Comment 19

[Paul Rouget [:paul]]

Questions:

Can we get a summary of the different possible options (how it works in Chrome and Opera)?

Is there any alternative (because the existing implementation don't look very great)?
We can probably improve the design, but I was wondering if could "re-think" the concept of
highlighting regions (one region is either the padding, the margin or the borders).

Do we want to show the margins, the padding and the border at the same time?
Do we want to only show one region at a time?

Do we want to differentiate the regions with colors? Patterns?
Maybe there's no need to differentiate them if we want to show one region at a time.

When do we want to highlight the region? On mouseover on the layoutview?

Do we want the regions to "live" (be stuck) with the highlighter and follow the scroll/zoom/updates?

What happens if there is a CSS Animation on the margin?

We are planning to hide the veil when using the Rule View (bug 735575), how does it affect this bug?

Can we imagine having "grids/outlines" to extend the regions? (inspiration: http://dribbble.com/shots/463243-3-Column-Grid http://dribbble.com/shots/289030-Christowski-Blog-Info)

Comment 20

[Heather Arthur [:harth]]

Attachment: screenshot of opera

Screenshot of Opera. Rule lines are an interesting idea.

Comment 21

[Heather Arthur [:harth]]

Attachment: screenshot of opera

real screenshot.

Comment 22

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

Attachment: Firefox Box Model Highlighter (from HTML & Layout panels)

The current Firebug screenshots miss a lot of features. Here is an updated one that shows the difference between hovering a node in the HTML panel and hovering the various areas of the layout panel.

Comment 23

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

(In reply to Paul Rouget [:paul] from comment #19)
> Questions:
> 
> Can we get a summary of the different possible options (how it works in
> Chrome and Opera)?
> 

When hovering a node in the HTML panel Chrome, Opera & Firebug all use the same method, a simple box model highlighter overlays the node on the page. Internet Explorer does nothing when a node is hovered in the HTML panel.

In addition:
Chrome:
Hovering the content, padding, border and margin in the layout panel highlights only the thing that is hovered. Hovering position (offsets) shows a doorhanger with e.g. "div 70px x 70px" inside pointing to the element. Double-clicking a number in the layout view allows each value to be edited.

Opera:
When hovering elements in the webpage itself guidelines are displayed showing the edges of the node's border along with a box model style highlight. In the layout panel hovering the content, padding, border and margin highlights only the thing that is hovered. Double-clicking a value allows it to be edited.

Internet Explorer:
Does nothing when the layout panel is hovered. Single-clicking a value allows for it to be edited.

Firebug:
Hovering anything in the layout view shows the simple box model highlighter. The nodes positioned parent is outlined in a dotted red line with rulers (px) on the top and left borders (an absolutely positioned node's positioned parent is the closest parent that is absolutely or relatively positioned).

Guide lines are displayed denoting the content, padding, border and margin depending on which is hovered in the layout view. Single-clicking any value allows for it to be edited.

The ultimate:
Hovering anything in the layout view shows the simple box model highlighter. The nodes positioned parent is outlined in a dotted red line with rulers (px) on the top and left borders (an absolutely positioned node's positioned parent is the closest parent that is absolutely or relatively positioned).

Depending on the users preference 1. Guide lines are displayed denoting the content, padding, border and margin depending on which is hovered in the layout view. Single-clicking any value allows for it to be edited or 2. Hovering the content, padding, border and margin in the layout panel highlights only the thing that is hovered ... guide lines denote the bounds of each as they are highlighted.

Hovering position (offsets) shows a doorhanger with e.g. "div 70px x 70px" inside pointing to the element.

Double-clicking a number in the layout view allows each value to be edited.

> Is there any alternative (because the existing implementation don't look
> very great)?
> We can probably improve the design, but I was wondering if could "re-think"
> the concept of
> highlighting regions (one region is either the padding, the margin or the
> borders).
> 
> Do we want to show the margins, the padding and the border at the same time?
> Do we want to only show one region at a time?
> 
> Do we want to differentiate the regions with colors? Patterns?
> Maybe there's no need to differentiate them if we want to show one region at
> a time.
> 
> When do we want to highlight the region? On mouseover on the layoutview?
> 

Personally, I think the "ultimate" implementation from above would be best.

> Do we want the regions to "live" (be stuck) with the highlighter and follow
> the scroll/zoom/updates?
> 

That would make sense.

> What happens if there is a CSS Animation on the margin?
> 

The highlighter should be updated.

> We are planning to hide the veil when using the Rule View (bug 735575), how
> does it affect this bug?
> 

When hovering nodes or the layout view I would say that we should show it again during the time that they are hovered.

> Can we imagine having "grids/outlines" to extend the regions? (inspiration:
> http://dribbble.com/shots/463243-3-Column-Grid
> http://dribbble.com/shots/289030-Christowski-Blog-Info)

Do you mean allowing the edges of a highlight to be dragged to change these things for a node? Maybe show guides when a node is highlighted and allow the guides to be dragged to change the elements size ... this would be very awkward to use to change border width etc. Using text input also allows different units to be used %, px, pt etc.

Comment 24

[Paul Rouget [:paul]]

Brian wrote, in bug 770818 comment 3:
> #4 When the user is hovering over the various parts of the element in layout view, such as
> margin, border, etc the corresponding areas on the live element on the page are
> highlighted while the rest of the page is dimmed.

Brian,

if we remove the veil (as discussed in bug 770818), do you think just changing the size of the dashed border is enough (nothing is dimmed)?

Comment 25

[Paul Rouget [:paul]]

Changing the size of the dashed border (changing the size of the highlighted region) might not be enough as it only shows a bounding box, not a region.

Comment 26

[Paul Rouget [:paul]]

Attachment: v3

Comment 27

[Paul Rouget [:paul]]

Mac build will be available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-681e9b18fde0

Screencast:
http://www.youtube.com/watch?v=aePy2NpFtyI

Comment 28

[Paul Rouget [:paul]]

Todo:
- get a visual design + ui review

Comment 29

[Paul Rouget [:paul]]

Todo:
- get a visual design + ui review
- tests
- what to do for margin:auto (expand to parent padding box?)
- use a canvas for complex border design (stips)

Comment 30

[Paul Rouget [:paul]]

Interaction proposal:
- while inspecting (node not locked) we show the regions (margin/padding/borders/content)
- once locked, we hide the regions
- once locked and the mouse is overring the layout view, we show the targeted region (as in the screencast in comment 27)

Comment 31

[Paul Rouget [:paul]]

Todo:
- get a visual design + ui review
- tests
- what to do for margin:auto (expand to parent padding box?) (can happen in a follow-up bug)
- use a canvas for complex border design (strips) (can happen in a follow-up bug)
- show the regions when node not locked (can happen in a follow-up bug)

Comment 32

[Paul Silaghi, QA [:pauly]]

Hi guys,
Will this or bug 770818 be ready to go ahead in FF 15 release on Tuesday august 28?

Comment 36

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

Attachment: [e-mail]

Shorlander recently looked into this and provided this design.

Basically, he wants us to always use the box model in the same way that Chrome does.

For transformed elements we should be faithful to the box model so we should show the content and border boxes transformed but margin and padding should remain untransformed.

Hovering border, margin and padding in the layout view should show guides for content, border, padding or margin. The layout view's border needs to be made thicker in order to make this possible.

I believe getQuads() should give us the correct information.

Comment 37

[Patrick Brosset <:pbro>]

When it comes to css transformed elements, I'd like to suggest that we draw something like this: http://codepen.io/captainbrosset/full/lHpnK (I'll attach a screenshot too).
The idea is that you should easily see where the original box was supposed to be, and where the transformed box is now, possibly with 4 arrows, one for each corner.

Comment 38

[Patrick Brosset <:pbro>]

Attachment: display-normal-and-transformed-boxes.png

Comment 39

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

(In reply to Patrick Brosset from comment #38)
> Created attachment 815841 [details]
> display-normal-and-transformed-boxes.png

When an image is transformed only the border and content are moved. The margin, padding, border and content still take the same space on the page.

It would make sense to show the box model as normal but with the border and content transformed in this way. This is probably the next step as we need to get the box model in first.

Will you log a new bug for handling transformed elements in the box model highlighter?

Comment 40

[Patrick Brosset <:pbro>]

Done: bug 925744

Comment 41

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

Attachment: highlighter-2.png

We have decided on SVG for the highlighter due to it's capability to highlight complex shapes etc. We need to check for any performance issues when graphics are not accelerated.

The highlighter should also show guides.

Here is the latest mock-up.

Comment 42

[Patrick Brosset <:pbro>]

I think anyway something like SVG or canvas is mandatory for this. Two reasons I see are:
- if the element is css-transformed
- if the element has rounded corners
In both these cases, the highlighter will need to show a complex shape.

btw, Mike, are we going to be able to show border-radius? And do we even want to? I know chrome doesn't, and it feels a bit strange to be drawing rectangles on top of a rounded shape.

Comment 43

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

(In reply to Patrick Brosset from comment #42)
> 
> btw, Mike, are we going to be able to show border-radius? And do we even
> want to? I know chrome doesn't, and it feels a bit strange to be drawing
> rectangles on top of a rounded shape.

The box model never has rounded corners. If our highlighter is showing the box model then it shouldn't have rounded corners.

Comment 44

[Patrick Brosset <:pbro>]

Right, silly question.

Another point though, what about animated elements? Should the box model "follow" them around? If, say, the element currently selected has an animation assigned to it that increases its height or margin.

I would say yes, but that will require to re-draw the regions every now and then (maybe throttling it to 100ms or so).

Comment 45

[Patrick Brosset <:pbro>]

The current highlighter does follow animated elements today using, I think, "MozAfterPaint" events.

Comment 46

[Darrin Henein [:darrin]]

Highlighter Overlay Colors

Content (blue)  :  #80d4ff @ 0.4 alpha
Border (yellow) :  #ffe431 @ 0.4 alpha
Padding (green) :  #66cc52 @ 0.4 alpha
Margin (orange) :  #d89b28 @ 0.4 alpha

Comment 47

[Joe Walker [:jwalker]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #43)
> The box model never has rounded corners.

Why?

Comment 48

[Paul Rouget [:paul]]

(In reply to Joe Walker [:jwalker] from comment #47)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #43)
> > The box model never has rounded corners.
> 
> Why?

I agree with Mike, it'd be weird to show the rounded borders. Only borders have rounded borders, and they don't impact the box model.

Also, rounded borders "eat" the content, so it'd be weird to show a rounded border region overlapping with the content region.

Comment 49

[Albert Scheiner [:alberts]]

Is this still valid?

"The CSS box model describes the rectangular boxes that are generated for elements in the document tree and laid out according to the visual formatting model."
- http://www.w3.org/TR/CSS2/box.html

Comment 50

[Paul Rouget [:paul]]

Any update on this?

Comment 51

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

Yes, I am finishing off disable cache (moved into the TabActor) and then working on this.

Comment 52

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

For the time being I will base the patch on roc's patch from bug 917755 as that will give him more time. In the worst case scenario I could create a polyfill.

Comment 53

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

Attachment: box-model-highlighter-663778.patch WIP

This is WIP. I still need is to use getQuads to position the NodeInfobar and write tests.

Comment 54

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

Attachment: Rebased WIP

... and create a getBoxQuads polyfill.

getQuads does take scrolling into account but does not take frames into account (probably a bug). This means that we will need two methods:
- getBoxQuads polyfill
- getBoxQuadsWithFrameOffsets() method in highlighter.js

Comment 55

[Patrick Brosset <:pbro>]

Attachment: bug663778-node.getBoxQuads-polyfill.patch

Hi Mike, here's a polyfill for the getBoxQuads API.
It's inside LayoutHelpers.jsm.
I've also added a function called getAbsoluteBoxQuads which is what the highlighter uses. Indeed, getBoxQuads returns quads relative to the node's viewport, so this getAbsoluteBoxQuads function will loop up to get the quads relative to the top level window.
It's late so I might have forgotten stuff but (and I don't fully know why), it seems to work pretty well with respect to iframes and scrolling.
I'll attach a short videos demonstrating how it works on a simple test page.

Comment 56

[Patrick Brosset <:pbro>]

Oh by the way, this patch was built on top of attachment 8358519 [details] [diff] [review], so not your latest patch, as I didn't noticed you added a new one. I hope it'll be fine.

Comment 57

[Patrick Brosset <:pbro>]

Attachment: getquads-polyfilled-highlighter.mov

Just to show how the polyfill behaves on a simple test page, with scrolling and iframes.

Comment 58

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

Comment on attachment 8368224 [details] [diff] [review]
bug663778-node.getBoxQuads-polyfill.patch

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

r+ for the polyfill

Comment 59

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

Clearing needinfo request

Comment 60

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

Attachment: box-model-highlighter-663778.patch WIP

Can you make a first pass over this... nothing complicated.

Please grab the dependencies from:
http://hg.mozilla.org/users/mratcliffe_mozilla.com/box-model-highlighter

It will save you having to rebase them.

This is based on the native getBoxQuads implementation although the polyfill code is included (I will move that to a separate patch). When we have this fully working with the native implementation we can switch to the polyfill and make it more compatible.

Note that it is broken on zoomed and retina displays so run tests using --jsd and drag the browser window to a normal display before running any tests.

The only failing test I know of is browser_inspector_markup_navigation.js.

Comment 61

[Patrick Brosset <:pbro>]

Comment on attachment 8370144 [details] [diff] [review]
box-model-highlighter-663778.patch WIP

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

Ok, this is a preliminary review just looking at the code.
Indeed, I haven't had the time to play with the new highlighter (with the patch applied) yet.

Mike: can you provide a new patch that uses the polyfill whenever you're ready, just so I can test with the code that will actually be used (if we manage to get this in Aurora).

I have a few comments below, but I'm globally pretty happy with the patch so far.

::: browser/base/content/browser.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  @namespace html url("http://www.w3.org/1999/xhtml");
> +@namespace svg url("http://www.w3.org/2000/svg");

You've defined this @namespace in all of the browser.css files (this one and the themed ones), but you're actually using this namespace in highlighter.inc.css

Can the namespace be defined there instead? Maybe there's something about *.inc.css files that make this impossible, I don't know but I think these files are pre-processed in some ways.

::: browser/devtools/framework/selection.js
@@ +165,5 @@
> +    // set the node even if it is already set otherwise it is not possible to
> +    // e.g. highlight the same node twice.
> +    let rawValue = null;
> +    if (value && value.isLocal_toBeDeprecated()) {
> +      rawValue = value.rawNode();

Can you elaborate a bit more here?
Why did you need to do that change? Did the outline highlighter suffered from the same problem?

I see one potential problem with doing this: the markup-view, rule-view, computed-view, font-inspector and layout-view will refresh again even if the same node is reselected.
Indeed, most things in the inspector listen to selection.on("new-node-front") to refresh.
Maybe it's not an issue but I'm afraid we have code that sets the selection with the same node without checking.

::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
@@ +53,5 @@
>  
>    function assertH1Highlighted() {
> +    let deferred = promise.defer();
> +
> +    executeSoon(() => {

Why is this executeSoon needed? Can you add a comment?

::: browser/devtools/inspector/test/browser_inspector_bug_674871.js
@@ -49,5 @@
>      openInspector(aInspector => {
>        inspector = aInspector;
>        // Make sure the highlighter is shown so we can disable transitions
>        inspector.toolbox.highlighter.showBoxModel(getNodeFront(doc.body)).then(() => {
> -        getHighlighterOutline().setAttribute("disable-transitions", "true");

It's a good thing we can get rid of these.
And I do agree that transitions are not needed anymore with your patch, it would feel too "heavy" to have transitions on the box-model regions.

::: browser/devtools/inspector/test/browser_inspector_highlighter.js
@@ +65,5 @@
>  
>    EventUtils.synthesizeMouse(h1, 2, 2, {type: "mousemove"}, content);
>  }
>  
>  function testOutlineDimensions() {

I think the name of the function should be changed since we don't have an outline anymore.

@@ +99,4 @@
>      let outlineWidth = Math.floor(outlineDims.width);
>      let outlineHeight = Math.floor(outlineDims.height);
>  
> +    // TODO: Fix this when getBoxQuads() properly deals with zoom.

Since we're going to try and fix this bug with our polyfill, shouldn't we make it handle zoom too? In which case the 2 following asserts should stay.

::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
@@ +54,5 @@
>      aElement.ownerDocument.defaultView);
>  }
>  
>  function runTests() {
> +  executeSoon(testDiv1Highlighter);

Why is executeSoon needed here too? Can you add a comment if there's no other way.
Maybe the promise returned by inspector.toolbox.highlighterUtils.startPicker() somehow resolves too soon?

::: browser/devtools/inspector/test/browser_inspector_scrolling.js
@@ +33,5 @@
>  {
>    inspector = aInspector;
>  
> +  inspector.toolbox.highlighter.showBoxModel(getNodeFront(div)).then(() => {
> +    performScrollingTest();

nit: please remove the arrow function:
inspector.toolbox.highlighter.showBoxModel(getNodeFront(div)).then(performScrollingTest);

@@ +54,5 @@
>    }, false);
> +
> +  executeSoon(function() {
> +    // FIXME: this will fail on retina displays. EventUtils will only scroll
> +    // 25px down instead of 50.

Could you try using content.devicePixelRatio to make the test work on all types of screens?
I know this isn't actually part of the bug, but it's been a problem for a long time and since you're touching the file, might as well try and put it in.

::: browser/devtools/inspector/test/head.js
@@ +74,3 @@
>  
> +function getBoxModelStatus()
> +{

nit: please use the prevailing formatting style for the function opening brace.

::: browser/devtools/layoutview/view.css
@@ +69,5 @@
>    border-width: 25px;
>  }
>  
>  #borders {
> +  padding: 25px;

I noticed that with this change, the border region in the layout-view sidebar has changed, it used to be a thin solid line with box-shadow, now it looks exactly like the padding and margin regions.
I like the fact that it's wider than before, it was hard to position the mouse over the thin (2px?) line before. But I liked the way it looked before, the fact that it was a plain line with shadow made it quicker to understand what you were looking it.

Could you try making it look like a bit more like it used to look while preserving the padding: 25px you added?
Adding these rules to #borders {} could work:
border-width: 2px;
border-style: solid;
box-shadow: 0 0 16px #000;
But it's not perfect, better ask Darrin perhaps.

::: browser/devtools/layoutview/view.js
@@ +240,5 @@
>      this._lastRequest = lastRequest;
>      return this._lastRequest;
> +  },
> +
> +  showBoxModel: function(options={}) {

showBoxModel and hideBoxModel can now be supplanted by:
this.inspector.toolbox.highlighterUtils.highlightNodeFront()
and
this.inspector.toolbox.highlighterUtils.unhighlight()
I've moved this code from markup-view to toolbox recently so that it avoids duplicating this nasty if/else logic all over the place.

Having said this, I don't see where these methods are being called. They don't seem to be!
Did you intend to toggle the box model on mouse-over of the layout-view regions? I think this is an excellent idea (and it should only highlight the corresponding region, not the whole box-model), but if this isn't going to be part of this patch, better remove this code altogether (to keep the patch small).
Since we're trying to land this so it can be uplifted, better file a separate bug to deal with this.

::: browser/devtools/layoutview/view.xhtml
@@ +29,5 @@
>      </p>
>  
>      <div id="main">
>  
> +      <div id="margins" data-box="margin" tooltip="&margins.tooltip;">

These data attributes have probably been added to wire the showboxmodel-on-hover mechanism, and should be either used or removed (see my other comment in view.js).

::: toolkit/devtools/LayoutHelpers.jsm
@@ +74,5 @@
> +      return this._getBoxQuadsFromRect(offsetRect(paddingRect, "padding"));
> +    }
> +  },
> +
> +  getAbsoluteBoxQuads: function(node, options) {

I see you've moved the frame-offset logic to highlighter.js instead, so this function should be removed.

::: toolkit/devtools/server/actors/highlighter.js
@@ +315,5 @@
> +      padding: this._createSVGNode("padding", "polygon", this._boxModelContainer),
> +      content: this._createSVGNode("content", "polygon", this._boxModelContainer)
> +    };
> +
> +    this._guides = {

nit: please make the _boxModelNodes and _guides names consistent, so _guideNodes would be better maybe.

@@ +397,5 @@
>        barHeight: barHeight,
>      };
>    },
>  
> +  _createSVGNode: function(boxType, svgNodeType, parent) {

nit: please rename boxType into className, since that's what it is. It's only a box type some of the times. Also, I don't think the svg part is needed in svgNodeType, just nodeType would be enough.

@@ +551,5 @@
> +
> +    options.box = options.box || "content";
> +
> +    // We do not cache the rectangle purely because comparing all 16 points
> +    // unnecessarily complicates things.

I feel ok with not caching it anymore. If we do have perf problems in the future, this will be an easy change to make.
However, for someone looking at this code and not knowing that we did have caching before, this comment might seem strange. So, it's only a detail, but I would remove it.

@@ +552,5 @@
> +    options.box = options.box || "content";
> +
> +    // We do not cache the rectangle purely because comparing all 16 points
> +    // unnecessarily complicates things.
> +    let rect = this._getBounds("border");

I have to say it seems strange to me that the boxQuads API offers both bounds and points since it's basically the exact same information (or am I missing something?).
I would rather use the points only throughout this class.

Also, you're calling getBounds here which, in turn, calls getAdjustedQuads (which loops through frames), knowing that getAdjustedQuads will be called again 4 times below in the for..of loop.
You might want to rewrite this to reuse the result.

In fact, what we should do here is call getBoxQuads(boxType) 4 times, one for each type of box, and only then traverse the frames to offset all 4 boxes (to traverse only once). Walking up the DOM isn't an expensive thing, but getting offsets on each element as we go is, so better minimize the number of times we do this (especially knowing this will be done on mouse-over, so, potentially very fast)

@@ +592,5 @@
> +                }
> +                arr.splice(arr.lastIndexOf(val), 1);
> +              }
> +            }
> +          }

Looking at these last 20 lines, it's really to get what's being done here.
I would do 2 things:
- extract these 20 lines to another (private) function, and give it a name and comment that explain what this is about
- change the validX and validY names to something more self-explanatory.

If I understand this correctly, you're trying to get the min X/Y and max X/Y, but only if the box hasn't been transformed in a way that the guides would not be parallel to the page's edges, right?
For instance, if allY is [15, 10, 25, 30] (which could happen if the element is skewed), then there won't be any validY, which would end up hiding the top and bottom guides. Is my understanding correct?

If yes, do we really want to hide the guides when the element is transformed?
Indeed, they may not be super useful in this case. What was the reasoning behind this?

@@ +625,5 @@
> +   *         The guide to update
> +   * @param  {Integer} point
> +   *         x or y co-ordinate
> +   */
> +  _updateGuide: function(guide, point=-1) {

Please add more comments to this function about the fact that this will hide if no point is passed.

@@ +685,5 @@
>    /**
>     * Move the Infobar to the right place in the highlighter.
>     */
>    _moveInfobar: function() {
> +    const offset = 5;

Can you move this const to the top of the file and give it a more explicit name?
Something like INFO_BAR_OFFSET

@@ +687,5 @@
>     */
>    _moveInfobar: function() {
> +    const offset = 5;
> +
> +    let rect = this._getBounds("margin");

I'm again a little bit worried that we're calling getBounds again here, especially because _moveInfobar is called during _update, right after _highlightBoxModel is called.

The only other place where we call _moveInfobar is in _showInfobar, called itself in show, which also calls _update!

So basically, moveInfobar is called twice everytime we show the box model, and it is always called at the same time as highlightBoxModel, so we can probably make it accept a rect argument to avoid having it recompute it.

@@ +797,5 @@
> +        x: quads.p4.x + xOffset,
> +        y: quads.p4.y + yOffset,
> +        z: quads.p4.z
> +      },
> +      bounds: {

As I said in one of the comments in this file, if we decide not to use bounds and only rely on the points, we won't need this part.

Comment 62

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

Attachment: Patch 1: box-model-highlighter-663778.patch WIP

I still need to address pbrosset's review comments from comment 61 but this is the current patch.

Comment 63

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

Attachment: Patch 2: box-model-polyfill-663778.patch

This switches to using the polyfill that now plays well with zoom and Retina displays.

Comment 64

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

Comment on attachment 8370832 [details] [diff] [review]
Patch 1: box-model-highlighter-663778.patch WIP

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Missing feature
User impact if declined: The devtools highlighter changes make little sense with the simple frame that we currently use. This patch adds a box model highlighter and makes a large difference to the user experience.
Testing completed (on m-c, etc.): It will be
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None

Comment 65

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

Comment on attachment 8370835 [details] [diff] [review]
Patch 2: box-model-polyfill-663778.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Missing feature
User impact if declined: The devtools highlighter changes make little sense with the simple frame that we currently use. This patch adds a box model highlighter and makes a large difference to the user experience.
Testing completed (on m-c, etc.): It will be
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None

Comment 66

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

The patches need a little tidying but they are almost done.

At the moment you are best getting the prerequisites for this bug from http://hg.mozilla.org/users/mratcliffe_mozilla.com/box-model-highlighter/summary unless your hobby is rebasing.

The biggest issue at the moment is that we have broken the markup navigation test:
./mach mochitest-browser browser/devtools/markupview/test/browser_inspector_markup_navigation.js

Seems like some kind of timing issue we have uncovered.

Comment 67

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe - PTO until 28 JAN [:miker] [:mratcliffe] from comment #66)
> The patches need a little tidying but they are almost done.
> 
> At the moment you are best getting the prerequisites for this bug from
> http://hg.mozilla.org/users/mratcliffe_mozilla.com/box-model-highlighter/
> summary unless your hobby is rebasing.

Actually, the two patches apply cleanly without any prerequisites.

> 
> The biggest issue at the moment is that we have broken the markup navigation
> test:
> ./mach mochitest-browser
> browser/devtools/markupview/test/browser_inspector_markup_navigation.js
> 
> Seems like some kind of timing issue we have uncovered.

This test passes locally for me, I've also pushed to try to check: https://tbpl.mozilla.org/?tree=Try&rev=d0581b6c93cf

Comment 68

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

Seems like the C++ side of getBoxQuads() must be causing the issue.

Comment 69

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #68)
> Seems like the C++ side of getBoxQuads() must be causing the issue.

There do still seem to be some issues on the bc test, though at least for me they aren't showing up locally - https://tbpl.mozilla.org/php/getParsedLog.php?id=34156658&tree=Try

Comment 70

[Paul Rouget [:paul]]

Why did we change the layout view too? To match the highlighter?

I think it's much harder to understand which regions is which.

Can we do that in a followup bug?

Comment 71

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

(In reply to Paul Rouget [:paul] from comment #70)
> Why did we change the layout view too? To match the highlighter?
> 
> I think it's much harder to understand which regions is which.
> 
> Can we do that in a followup bug?

Because hovering each region moves the guides around that region and having a 2px border makes it difficult to hover the border.

Comment 72

[Paul Rouget [:paul]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #71)
> (In reply to Paul Rouget [:paul] from comment #70)
> > Why did we change the layout view too? To match the highlighter?
> > 
> > I think it's much harder to understand which regions is which.
> > 
> > Can we do that in a followup bug?
> 
> Because hovering each region moves the guides around that region and having
> a 2px border makes it difficult to hover the border.

hoooo! Nice. But still, the new look is confusing. What is content, border, margin, …?

Comment 73

[Patrick Brosset <:pbro>]

I somewhat agree with Paul. This was one of my comments in the review.
I think Paul's idea to keep this for a follow-up bug is very good, especially since we want to uplift this to Aurora. Let's keep it as small as possible. What do you think?

Comment 74

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

We have decided to make the box model panel colors the same match the box model highlighter colors.

We also decided not to attempt to uplift this as it is a new feature and also because it is too large.

Comment 75

[Paul Rouget [:paul]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #74)
> We have decided to make the box model panel colors the same match the box
> model highlighter colors.

Sweet! Mockup?

Comment 76

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

Attachment: box-model-highlighter-663778.patch WIP

(In reply to Paul Rouget [:paul] from comment #75)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #74)
> > We have decided to make the box model panel colors the same match the box
> > model highlighter colors.
> 
> Sweet! Mockup?

Why have a mockup when you can have the real thing? The layout view is now fixed so the guides now move around the hovered region. I have also given it the highlighter colors for easier identification of each region.

If you demo it then please use this patch as a lot of issues have been fixed.

Comment 77

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

Attachment: Layout view.PNG

Darrin: Would you like me to change anything in the new layout view design? The colors now match the highlighted regions and the guides move around those regions when they are hovered in the layout view.

Comment 78

[Tim Nguyen :ntim]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #77)
> Created attachment 8372221 [details]
> Layout view.PNG


In my opinion, the layout view should stay as it is now, then the colors should show up when hovering the layout view.

Comment 79

[Darrin Henein [:darrin]]

Comment on attachment 8372221 [details]
Layout view.PNG

Looks great, thanks! 2 questions: 

1. Where did the current border UI go? Right now we have a bold(er) line representing the border and 4 labels to indicate the width of each border. Could we not have just colored that to match the highlighter?

2. Can we decrease the box shadow intensity by around 50% on the yellow box? I know we have that right now but its less visible on our current blue theme.

Comment 80

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

Attachment: Addressed review comments - tests pass locally

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #61)
> Comment on attachment 8370144 [details] [diff] [review]
> box-model-highlighter-663778.patch WIP
> 
> Review of attachment 8370144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, this is a preliminary review just looking at the code.
> Indeed, I haven't had the time to play with the new highlighter (with the
> patch applied) yet.
> 
> Mike: can you provide a new patch that uses the polyfill whenever you're
> ready, just so I can test with the code that will actually be used (if we
> manage to get this in Aurora).
> 

Done

> I have a few comments below, but I'm globally pretty happy with the patch so
> far.
> 
> ::: browser/base/content/browser.css
> @@ +3,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> >  @namespace html url("http://www.w3.org/1999/xhtml");
> > +@namespace svg url("http://www.w3.org/2000/svg");
> 
> You've defined this @namespace in all of the browser.css files (this one and
> the themed ones), but you're actually using this namespace in
> highlighter.inc.css
> 
> Can the namespace be defined there instead? Maybe there's something about
> *.inc.css files that make this impossible, I don't know but I think these
> files are pre-processed in some ways.
> 

Unfortunately not. @namespace must come after any @charset and @import
rules and before any style rules. Because he .inc.css file literally becomes
part of browser.css, @namespace has to be declared at the top of that file.

> ::: browser/devtools/framework/selection.js
> @@ +165,5 @@
> > +    // set the node even if it is already set otherwise it is not possible to
> > +    // e.g. highlight the same node twice.
> > +    let rawValue = null;
> > +    if (value && value.isLocal_toBeDeprecated()) {
> > +      rawValue = value.rawNode();
> 
> Can you elaborate a bit more here?
> Why did you need to do that change? Did the outline highlighter suffered
> from the same problem?
> 
> I see one potential problem with doing this: the markup-view, rule-view,
> computed-view, font-inspector and layout-view will refresh again even if the
> same node is reselected.
> Indeed, most things in the inspector listen to
> selection.on("new-node-front") to refresh.
> Maybe it's not an issue but I'm afraid we have code that sets the selection
> with the same node without checking.
> 

Yes, the outline inspector suffers from the same issue. Without this change:
- Right-click a node and inspect element.
- Right-click the same node and try to inspect it again. Nothing happens, the
highlighter is not shown so to the user it seems broken.

> ::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
> @@ +53,5 @@
> >  
> >    function assertH1Highlighted() {
> > +    let deferred = promise.defer();
> > +
> > +    executeSoon(() => {
> 
> Why is this executeSoon needed? Can you add a comment?
> 

You are right. It turns out that the inspector is fully initialized before the
highlighter so doing this within openInspector() will cause highlighter tests to
fail:
  inspector.once("inspector-updated", runTests);
We have to use this instead:
  inspector.toolbox.once("node-highlight", runTests);

> ::: browser/devtools/inspector/test/browser_inspector_bug_674871.js
> @@ -49,5 @@
> >      openInspector(aInspector => {
> >        inspector = aInspector;
> >        // Make sure the highlighter is shown so we can disable transitions
> >        inspector.toolbox.highlighter.showBoxModel(getNodeFront(doc.body)).then(() => {
> > -        getHighlighterOutline().setAttribute("disable-transitions", "true");
> 
> It's a good thing we can get rid of these.
> And I do agree that transitions are not needed anymore with your patch, it
> would feel too "heavy" to have transitions on the box-model regions.
> 

Agreed.

> ::: browser/devtools/inspector/test/browser_inspector_highlighter.js
> @@ +65,5 @@
> >  
> >    EventUtils.synthesizeMouse(h1, 2, 2, {type: "mousemove"}, content);
> >  }
> >  
> >  function testOutlineDimensions() {
> 
> I think the name of the function should be changed since we don't have an
> outline anymore.
> 

Fixed

> @@ +99,4 @@
> >      let outlineWidth = Math.floor(outlineDims.width);
> >      let outlineHeight = Math.floor(outlineDims.height);
> >  
> > +    // TODO: Fix this when getBoxQuads() properly deals with zoom.
> 
> Since we're going to try and fix this bug with our polyfill, shouldn't we
> make it handle zoom too? In which case the 2 following asserts should stay.
> 

Yes, that is what I planned

> ::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
> @@ +54,5 @@
> >      aElement.ownerDocument.defaultView);
> >  }
> >  
> >  function runTests() {
> > +  executeSoon(testDiv1Highlighter);
> 
> Why is executeSoon needed here too? Can you add a comment if there's no
> other way.
> Maybe the promise returned by
> inspector.toolbox.highlighterUtils.startPicker() somehow resolves too soon?
> 

We need to use executeSoon there to strike a balance between performance and
testability. highlighter.js::pick() contains HIGHLIGHTER_PICKED_TIMER that is
used for the "picker-node-picked" event. Using this timer for
"picker-node-hover" causes the UI to become very sluggish. As to why pick has to
wait I have no idea.

> ::: browser/devtools/inspector/test/browser_inspector_scrolling.js
> @@ +33,5 @@
> >  {
> >    inspector = aInspector;
> >  
> > +  inspector.toolbox.highlighter.showBoxModel(getNodeFront(div)).then(() => {
> > +    performScrollingTest();
> 
> nit: please remove the arrow function:
> inspector.toolbox.highlighter.showBoxModel(getNodeFront(div)).
> then(performScrollingTest);
> 

Done

> @@ +54,5 @@
> >    }, false);
> > +
> > +  executeSoon(function() {
> > +    // FIXME: this will fail on retina displays. EventUtils will only scroll
> > +    // 25px down instead of 50.
> 
> Could you try using content.devicePixelRatio to make the test work on all
> types of screens?
> I know this isn't actually part of the bug, but it's been a problem for a
> long time and since you're touching the file, might as well try and put it
> in.
> 

I have added a simple patch to bug Bug 884796 to fix this. Because the fix is
in EventUtils robcee will need to review it as he is a testing peer.

> ::: browser/devtools/inspector/test/head.js
> @@ +74,3 @@
> >  
> > +function getBoxModelStatus()
> > +{
> 
> nit: please use the prevailing formatting style for the function opening
> brace.
> 

Done

> ::: browser/devtools/layoutview/view.css
> @@ +69,5 @@
> >    border-width: 25px;
> >  }
> >  
> >  #borders {
> > +  padding: 25px;
> 
> I noticed that with this change, the border region in the layout-view
> sidebar has changed, it used to be a thin solid line with box-shadow, now it
> looks exactly like the padding and margin regions.
> I like the fact that it's wider than before, it was hard to position the
> mouse over the thin (2px?) line before. But I liked the way it looked
> before, the fact that it was a plain line with shadow made it quicker to
> understand what you were looking it.
> 
> Could you try making it look like a bit more like it used to look while
> preserving the padding: 25px you added?
> Adding these rules to #borders {} could work:
> border-width: 2px;
> border-style: solid;
> box-shadow: 0 0 16px #000;
> But it's not perfect, better ask Darrin perhaps.
>

This is now colored the same as the highlighter regions. I have also pinged
Darrin for feedback.

> ::: browser/devtools/layoutview/view.js
> @@ +240,5 @@
> >      this._lastRequest = lastRequest;
> >      return this._lastRequest;
> > +  },
> > +
> > +  showBoxModel: function(options={}) {
> 
> showBoxModel and hideBoxModel can now be supplanted by:
> this.inspector.toolbox.highlighterUtils.highlightNodeFront()
> and
> this.inspector.toolbox.highlighterUtils.unhighlight()
> I've moved this code from markup-view to toolbox recently so that it avoids
> duplicating this nasty if/else logic all over the place.
> 
> Having said this, I don't see where these methods are being called. They
> don't seem to be!
> Did you intend to toggle the box model on mouse-over of the layout-view
> regions? I think this is an excellent idea (and it should only highlight the
> corresponding region, not the whole box-model), but if this isn't going to
> be part of this patch, better remove this code altogether (to keep the patch
> small).
> Since we're trying to land this so it can be uplifted, better file a
> separate bug to deal with this.
> 

Yes, I must have broken this on rebase. It is now fixed again.

> ::: browser/devtools/layoutview/view.xhtml
> @@ +29,5 @@
> >      </p>
> >  
> >      <div id="main">
> >  
> > +      <div id="margins" data-box="margin" tooltip="&margins.tooltip;">
> 
> These data attributes have probably been added to wire the
> showboxmodel-on-hover mechanism, and should be either used or removed (see
> my other comment in view.js).
> 

They are now used

> ::: toolkit/devtools/LayoutHelpers.jsm
> @@ +74,5 @@
> > +      return this._getBoxQuadsFromRect(offsetRect(paddingRect, "padding"));
> > +    }
> > +  },
> > +
> > +  getAbsoluteBoxQuads: function(node, options) {
> 
> I see you've moved the frame-offset logic to highlighter.js instead, so this
> function should be removed.
> 

Done

> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +315,5 @@
> > +      padding: this._createSVGNode("padding", "polygon", this._boxModelContainer),
> > +      content: this._createSVGNode("content", "polygon", this._boxModelContainer)
> > +    };
> > +
> > +    this._guides = {
> 
> nit: please make the _boxModelNodes and _guides names consistent, so
> _guideNodes would be better maybe.
> 

Changed

> @@ +397,5 @@
> >        barHeight: barHeight,
> >      };
> >    },
> >  
> > +  _createSVGNode: function(boxType, svgNodeType, parent) {
> 
> nit: please rename boxType into className, since that's what it is. It's
> only a box type some of the times. Also, I don't think the svg part is
> needed in svgNodeType, just nodeType would be enough.
> 

Agreed... done.

> @@ +551,5 @@
> > +
> > +    options.box = options.box || "content";
> > +
> > +    // We do not cache the rectangle purely because comparing all 16 points
> > +    // unnecessarily complicates things.
> 
> I feel ok with not caching it anymore. If we do have perf problems in the
> future, this will be an easy change to make.
> However, for someone looking at this code and not knowing that we did have
> caching before, this comment might seem strange. So, it's only a detail, but
> I would remove it.
> 

Removed

> @@ +552,5 @@
> > +    options.box = options.box || "content";
> > +
> > +    // We do not cache the rectangle purely because comparing all 16 points
> > +    // unnecessarily complicates things.
> > +    let rect = this._getBounds("border");
> 
> I have to say it seems strange to me that the boxQuads API offers both
> bounds and points since it's basically the exact same information (or am I
> missing something?).
> I would rather use the points only throughout this class.
> 
> Also, you're calling getBounds here which, in turn, calls getAdjustedQuads
> (which loops through frames), knowing that getAdjustedQuads will be called
> again 4 times below in the for..of loop.
> You might want to rewrite this to reuse the result.
> 
> In fact, what we should do here is call getBoxQuads(boxType) 4 times, one
> for each type of box, and only then traverse the frames to offset all 4
> boxes (to traverse only once). Walking up the DOM isn't an expensive thing,
> but getting offsets on each element as we go is, so better minimize the
> number of times we do this (especially knowing this will be done on
> mouse-over, so, potentially very fast)
> 

Yeah, we now cache the quads and reuse when possible.

> @@ +592,5 @@
> > +                }
> > +                arr.splice(arr.lastIndexOf(val), 1);
> > +              }
> > +            }
> > +          }
> 
> Looking at these last 20 lines, it's really to get what's being done here.
> I would do 2 things:
> - extract these 20 lines to another (private) function, and give it a name
> and comment that explain what this is about
> - change the validX and validY names to something more self-explanatory.
> 

Done

> If I understand this correctly, you're trying to get the min X/Y and max
> X/Y, but only if the box hasn't been transformed in a way that the guides
> would not be parallel to the page's edges, right?

Yup

> For instance, if allY is [15, 10, 25, 30] (which could happen if the element
> is skewed), then there won't be any validY, which would end up hiding the
> top and bottom guides. Is my understanding correct?
> 

Spot on

> If yes, do we really want to hide the guides when the element is transformed?
> Indeed, they may not be super useful in this case. What was the reasoning
> behind this?
> 

We discussed this at length in a daily meeting and decided that the guides are
very useful for lining up transformed elements, especially considering that
hovering the different layout regions in the layout panel will move the guides
around that region.

> @@ +625,5 @@
> > +   *         The guide to update
> > +   * @param  {Integer} point
> > +   *         x or y co-ordinate
> > +   */
> > +  _updateGuide: function(guide, point=-1) {
> 
> Please add more comments to this function about the fact that this will hide
> if no point is passed.
> 

Done

> @@ +685,5 @@
> >    /**
> >     * Move the Infobar to the right place in the highlighter.
> >     */
> >    _moveInfobar: function() {
> > +    const offset = 5;
> 
> Can you move this const to the top of the file and give it a more explicit
> name?
> Something like INFO_BAR_OFFSET
> 

Done

> @@ +687,5 @@
> >     */
> >    _moveInfobar: function() {
> > +    const offset = 5;
> > +
> > +    let rect = this._getBounds("margin");
> 
> I'm again a little bit worried that we're calling getBounds again here,
> especially because _moveInfobar is called during _update, right after
> _highlightBoxModel is called.
> 
> The only other place where we call _moveInfobar is in _showInfobar, called
> itself in show, which also calls _update!
> 
> So basically, moveInfobar is called twice everytime we show the box model,
> and it is always called at the same time as highlightBoxModel, so we can
> probably make it accept a rect argument to avoid having it recompute it.
> 

Fixed

> @@ +797,5 @@
> > +        x: quads.p4.x + xOffset,
> > +        y: quads.p4.y + yOffset,
> > +        z: quads.p4.z
> > +      },
> > +      bounds: {
> 
> As I said in one of the comments in this file, if we decide not to use
> bounds and only rely on the points, we won't need this part.

getBounds has been removed

Comment 81

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

(In reply to Darrin Henein [:darrin] from comment #79)
> Comment on attachment 8372221 [details]
> Layout view.PNG
> 
> Looks great, thanks! 2 questions: 
> 
> 1. Where did the current border UI go? Right now we have a bold(er) line
> representing the border and 4 labels to indicate the width of each border.
> Could we not have just colored that to match the highlighter?
> 

We needed to make the border wider in order to give an area for the layout view
region mouseover behavior (the guides move to surround the area that you are
mousing over). If the border is only a couple of pixels wide it is difficult to
hover it.

See https://www.youtube.com/watch?v=1D_nwI46n0o

> 2. Can we decrease the box shadow intensity by around 50% on the yellow box?
> I know we have that right now but its less visible on our current blue theme.

Done

Comment 82

[Patrick Brosset <:pbro>]

Comment on attachment 8373569 [details] [diff] [review]
Addressed review comments - tests pass locally

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

Thanks Mike for the new patch. I've applied it and played with it, it's looking really great.
The mouseover on the layout-view is pretty cool too.

Thanks for having addressed my comments and for your answers.
I just have a few remaining comments below, mostly about tests (indeed, I think the patch is functional and works great, we just need to focus on tests now).

r=me as soon as we manage to get it passing on try.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #80)
> Created attachment 8373569 [details] [diff] [review]
> Addressed review comments - tests pass locally
> 
> Try:
> https://tbpl.mozilla.org/?tree=Try&rev=c7fd883ced02
Seems to fail in debug builds with some highlighter tests.
I can help you on these since I spent quite a bit of time making them pass when working on the remote highlighter bug. Give me a shout if/when you need help.

> > ::: browser/devtools/framework/selection.js
> > @@ +165,5 @@
> > > +    // set the node even if it is already set otherwise it is not possible to
> > > +    // e.g. highlight the same node twice.
> > > +    let rawValue = null;
> > > +    if (value && value.isLocal_toBeDeprecated()) {
> > > +      rawValue = value.rawNode();
> > 
> > Can you elaborate a bit more here?
> > Why did you need to do that change? Did the outline highlighter suffered
> > from the same problem?
> > 
> > I see one potential problem with doing this: the markup-view, rule-view,
> > computed-view, font-inspector and layout-view will refresh again even if the
> > same node is reselected.
> > Indeed, most things in the inspector listen to
> > selection.on("new-node-front") to refresh.
> > Maybe it's not an issue but I'm afraid we have code that sets the selection
> > with the same node without checking.
> > 
> 
> Yes, the outline inspector suffers from the same issue. Without this change:
> - Right-click a node and inspect element.
> - Right-click the same node and try to inspect it again. Nothing happens, the
> highlighter is not shown so to the user it seems broken.
Ok got it. I was afraid this might cause mutliple refresh problems but as it turns out, the inspector sidebar tools don't refresh when the same node is selected again and it's nice to be able to click again (in the breadcrumbs for instance) and have the node highlight again briefly.

> > ::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
> > @@ +53,5 @@
> > >  
> > >    function assertH1Highlighted() {
> > > +    let deferred = promise.defer();
> > > +
> > > +    executeSoon(() => {
> > 
> > Why is this executeSoon needed? Can you add a comment?
> > 
> 
> You are right. It turns out that the inspector is fully initialized before
> the
> highlighter so doing this within openInspector() will cause highlighter
> tests to
> fail:
>   inspector.once("inspector-updated", runTests);
> We have to use this instead:
>   inspector.toolbox.once("node-highlight", runTests);
I don't understand why this can even work. Just before this line you do:
inspector.selection.setNode(doc.querySelector("h2"), null);
Passing |null| as a reason should not briefly highlight the node (by the way, can you change null into "test" instead? This is a new reason I've introduced specifically for tests), and therefore the node-highlight event shouldn't be fired.
In many tests I've worked on recently, the pattern I used that works well is:

inspector.once("inspector-updated", runTests);
inspector.selection.setNode(doc.querySelector("h2"), "test");

This should work fine normally.

> > ::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
> > @@ +54,5 @@
> > >      aElement.ownerDocument.defaultView);
> > >  }
> > >  
> > >  function runTests() {
> > > +  executeSoon(testDiv1Highlighter);
> > 
> > Why is executeSoon needed here too? Can you add a comment if there's no
> > other way.
> > Maybe the promise returned by
> > inspector.toolbox.highlighterUtils.startPicker() somehow resolves too soon?
> > 
> 
> We need to use executeSoon there to strike a balance between performance and
> testability. highlighter.js::pick() contains HIGHLIGHTER_PICKED_TIMER that is
> used for the "picker-node-picked" event. Using this timer for
> "picker-node-hover" causes the UI to become very sluggish. As to why pick
> has to
> wait I have no idea.
No, HIGHLIGHTER_PICKED_TIMER isn't used for the "picker-node-picked" event, the event is fired as soon as the node is picked, the timer is only used to wait for a bit before the highlighter goes away (so when you click on a node, it stays up for 1 second and then disappears).
For sure we shouldn't use this timer for any of the events.
So the comment you added in the file about this is wrong and should be removed.
So I still don't understand why executeSoon would be needed here, but TBH, I had a lot of problems related to timings with the highlighter myself when remoting it.
Let's leave it there for now, remove the comment, and anyway, since tests are still failing on try debug builds, I'll try and take a look with you.

::: browser/devtools/inspector/test/browser_inspector_bug_699308_iframe_navigation.js
@@ +18,5 @@
>      inspector.toolbox.highlighterUtils.startPicker().then(() => {
>        EventUtils.synthesizeMouse(content.document.body, 1, 1,
>          {type: "mousemove"}, content);
>        inspector.toolbox.once("picker-node-hovered", () => {
> +        cb();

nit: this can be rewritten:
inspector.toolbox.once("picker-node-hovered", cb);

::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
@@ +37,5 @@
> +        // We need to use executeSoon here to strike a balance between
> +        // performance and testability. highlighter.js::pick() contains
> +        // HIGHLIGHTER_PICKED_TIMER that is used for the "picker-node-picked"
> +        // event. Using this timer for the "picker-node-hover" causes the UI
> +        // to become very sluggish.

As explained in my comments, this should be removed.

@@ +54,5 @@
>  }
>  
>  function moveMouseOver(aElement, cb) {
>    inspector.toolbox.once("picker-node-hovered", () => {
> +    cb();

nit: can be rewritten:
inspector.toolbox.once("picker-node-hovered", cb);

Comment 83

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

Attachment: patch: Fixed some tests

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #82)
> Comment on attachment 8373569 [details] [diff] [review]
> Addressed review comments - tests pass locally
> 
> Review of attachment 8373569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Mike for the new patch. I've applied it and played with it, it's
> looking really great.
> The mouseover on the layout-view is pretty cool too.
> 
> Thanks for having addressed my comments and for your answers.
> I just have a few remaining comments below, mostly about tests (indeed, I
> think the patch is functional and works great, we just need to focus on
> tests now).
> 
> r=me as soon as we manage to get it passing on try.
> 
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #80)
> > Created attachment 8373569 [details] [diff] [review]
> > Addressed review comments - tests pass locally
> > 
> > Try:
> > https://tbpl.mozilla.org/?tree=Try&rev=c7fd883ced02
> Seems to fail in debug builds with some highlighter tests.
> I can help you on these since I spent quite a bit of time making them pass
> when working on the remote highlighter bug. Give me a shout if/when you need
> help.
> 

If you have time:
https://etherpad.mozilla.org/box-model-highlighter

> > > ::: browser/devtools/framework/selection.js
> > > @@ +165,5 @@
> > > > +    // set the node even if it is already set otherwise it is not possible to
> > > > +    // e.g. highlight the same node twice.
> > > > +    let rawValue = null;
> > > > +    if (value && value.isLocal_toBeDeprecated()) {
> > > > +      rawValue = value.rawNode();
> > > 
> > > Can you elaborate a bit more here?
> > > Why did you need to do that change? Did the outline highlighter suffered
> > > from the same problem?
> > > 
> > > I see one potential problem with doing this: the markup-view, rule-view,
> > > computed-view, font-inspector and layout-view will refresh again even if the
> > > same node is reselected.
> > > Indeed, most things in the inspector listen to
> > > selection.on("new-node-front") to refresh.
> > > Maybe it's not an issue but I'm afraid we have code that sets the selection
> > > with the same node without checking.
> > > 
> > 
> > Yes, the outline inspector suffers from the same issue. Without this change:
> > - Right-click a node and inspect element.
> > - Right-click the same node and try to inspect it again. Nothing happens, the
> > highlighter is not shown so to the user it seems broken.
> Ok got it. I was afraid this might cause mutliple refresh problems but as it
> turns out, the inspector sidebar tools don't refresh when the same node is
> selected again and it's nice to be able to click again (in the breadcrumbs
> for instance) and have the node highlight again briefly.
> 

Exactly.

> > > ::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
> > > @@ +53,5 @@
> > > >  
> > > >    function assertH1Highlighted() {
> > > > +    let deferred = promise.defer();
> > > > +
> > > > +    executeSoon(() => {
> > > 
> > > Why is this executeSoon needed? Can you add a comment?
> > > 
> > 
> > You are right. It turns out that the inspector is fully initialized before
> > the
> > highlighter so doing this within openInspector() will cause highlighter
> > tests to
> > fail:
> >   inspector.once("inspector-updated", runTests);
> > We have to use this instead:
> >   inspector.toolbox.once("node-highlight", runTests);
> I don't understand why this can even work. Just before this line you do:
> inspector.selection.setNode(doc.querySelector("h2"), null);
> Passing |null| as a reason should not briefly highlight the node (by the
> way, can you change null into "test" instead? This is a new reason I've
> introduced specifically for tests), and therefore the node-highlight event
> shouldn't be fired.
> In many tests I've worked on recently, the pattern I used that works well is:
> 
> inspector.once("inspector-updated", runTests);
> inspector.selection.setNode(doc.querySelector("h2"), "test");
> 
> This should work fine normally.
> 

Done

> > > ::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
> > > @@ +54,5 @@
> > > >      aElement.ownerDocument.defaultView);
> > > >  }
> > > >  
> > > >  function runTests() {
> > > > +  executeSoon(testDiv1Highlighter);
> > > 
> > > Why is executeSoon needed here too? Can you add a comment if there's no
> > > other way.
> > > Maybe the promise returned by
> > > inspector.toolbox.highlighterUtils.startPicker() somehow resolves too soon?
> > > 
> > 
> > We need to use executeSoon there to strike a balance between performance and
> > testability. highlighter.js::pick() contains HIGHLIGHTER_PICKED_TIMER that is
> > used for the "picker-node-picked" event. Using this timer for
> > "picker-node-hover" causes the UI to become very sluggish. As to why pick
> > has to
> > wait I have no idea.
> No, HIGHLIGHTER_PICKED_TIMER isn't used for the "picker-node-picked" event,
> the event is fired as soon as the node is picked, the timer is only used to
> wait for a bit before the highlighter goes away (so when you click on a
> node, it stays up for 1 second and then disappears).
> For sure we shouldn't use this timer for any of the events.
> So the comment you added in the file about this is wrong and should be
> removed.
> So I still don't understand why executeSoon would be needed here, but TBH, I
> had a lot of problems related to timings with the highlighter myself when
> remoting it.
> Let's leave it there for now, remove the comment, and anyway, since tests
> are still failing on try debug builds, I'll try and take a look with you.
> 

Okay, Mihai has logged bug 971785, which I suspect will fix a bunch of timing
issues in the highlighter. I will probably absorb that into this patch.

> :::
> browser/devtools/inspector/test/
> browser_inspector_bug_699308_iframe_navigation.js
> @@ +18,5 @@
> >      inspector.toolbox.highlighterUtils.startPicker().then(() => {
> >        EventUtils.synthesizeMouse(content.document.body, 1, 1,
> >          {type: "mousemove"}, content);
> >        inspector.toolbox.once("picker-node-hovered", () => {
> > +        cb();
> 
> nit: this can be rewritten:
> inspector.toolbox.once("picker-node-hovered", cb);
> 

Yeah, I have already changed that.

> ::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
> @@ +37,5 @@
> > +        // We need to use executeSoon here to strike a balance between
> > +        // performance and testability. highlighter.js::pick() contains
> > +        // HIGHLIGHTER_PICKED_TIMER that is used for the "picker-node-picked"
> > +        // event. Using this timer for the "picker-node-hover" causes the UI
> > +        // to become very sluggish.
> 
> As explained in my comments, this should be removed.
> 

Agreed

> @@ +54,5 @@
> >  }
> >  
> >  function moveMouseOver(aElement, cb) {
> >    inspector.toolbox.once("picker-node-hovered", () => {
> > +    cb();
> 
> nit: can be rewritten:
> inspector.toolbox.once("picker-node-hovered", cb);

Done

Comment 84

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

Attachment: patch 1: Fixed more tests

Merged fix from bug 971785

Comment 86

[Patrick Brosset <:pbro>]

Attachment: inline-boxes.png

I have a remark regarding inline boxes. In the screenshot, you see a link (display:inline <a> tag) that wraps on 2 lines.

In this case, the padding, border, margin apply to each of the line boxes, not the outer container.

I've added a red border on the link and a padding to illustrate the point.
You see that our current highlighter displays wrong information. Or, not really wrong, but not correctly positioned along the line boxes.

I checked in firebug and chrome, turns out they do the same as us.
So sticking to this representation may be less disturbing to users, but it's definitely not correct, and I think a highlighter that would correctly highlight wrapping inline boxes would help better understand how things work in CSS.

Mike: this comes from the polyfilled version of getBoxQuads that we use, for sure, but do you know if the final version will behave the same?

Comment 87

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #86)
> Created attachment 8376132 [details]
> inline-boxes.png
> 
> I have a remark regarding inline boxes. In the screenshot, you see a link
> (display:inline <a> tag) that wraps on 2 lines.
> 
> In this case, the padding, border, margin apply to each of the line boxes,
> not the outer container.
> 
> I've added a red border on the link and a padding to illustrate the point.
> You see that our current highlighter displays wrong information. Or, not
> really wrong, but not correctly positioned along the line boxes.
> 
> I checked in firebug and chrome, turns out they do the same as us.
> So sticking to this representation may be less disturbing to users, but it's
> definitely not correct, and I think a highlighter that would correctly
> highlight wrapping inline boxes would help better understand how things work
> in CSS.
> 
> Mike: this comes from the polyfilled version of getBoxQuads that we use, for
> sure, but do you know if the final version will behave the same?

Completely agree... all tools currently do this wrong. I remember researching this for Firebug but I never got around to implementing inline highlighting. Basically, *left and *right are honored by inlines but *top and *bottom are ignored. Also, the style is only applied to the start and end of the inline element.

The only way I could find was:
let range = document.createRange();
range.selectNodeContents(textNode);
let rects = range.getClientRects();
let textNodeLeft = rects[0].left;
...
And then building the correct shape out of a combination of the rects and the nodes computed style. This enhancement should be part of a new bug.

Comment 88

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

Attachment: patch 1: Fixed more tests

Hopefully fixed debugger tests.

Comment 89

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

Attachment: Disabled dom.send_after_paint_to_content for DevTools tests and added logging to EventUtils.emit()

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

Comment 90

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

Attachment: patch 1: Drastically improved logging

Added some logging to EventEmitter.emit in order to nail down try failures.

Comment 91

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

Latest try:
https://tbpl.mozilla.org/?tree=Try&rev=191b08f8f413

Comment 92

[Mihai Sucan [:msucan]]

Comment on attachment 8376353 [details] [diff] [review]
patch 1: Drastically improved logging

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

As discussed in the meeting, I'm going through the code, to see if I can spot potential race conditions.

toolbox.js startPicker() and stopPicker() seem to also inconsistently resolve the promises they return. I dont know well this code, but here are my comments about it:

* startPicker() returns a promise that is resolved when done() is invoked - that happens after initInspector(), after selectTool(), and after highlighter.pick() (when isRemoteHighlightable).

* if !isRemoteHighlightable, the code does walker.pick(), but it doesnt wait for it, and it calls done(). Why? Not saying this is wrong, just making sure this isnt an error.

* a quick successive call to startPicker() starts the whole process again (before _isPicking=true). Is this safe/expected? Should it return the same promise? What is the expected behavior in such cases?

* if this._isPicking=true returns a resolved promise, which is inconsistent with the first call. The first call resolves *after* highlighter.pick(), but _isPicking is set to true *before* pick() happens. So any call during this time will have unexpected behavior. Why _isPicking set to true after initInspector()+selectTool()? I would've expected it to be set in done().

* stopPicker() is similar: it sets _isPicking=false after initInspector(), but the promise resolves when done() is invoked. The done() function is called when highlighter/walker.cancelPick() resolves. Maybe I am missing something, but these appear to be potential inconsistencies causing intermittent failures.

See below for an additional comments.

::: browser/devtools/framework/toolbox.js
@@ +1109,1 @@
>      return walker.then(outstanding, outstanding);

This doesnt look quite right. this._destroying should be:

this._destroying = walker.then(outstanding, outstanding);

so you can always return this._destroying. The current return is not the same as the _destroying promise you return at the beginning of the function.

Comment 93

[thinsoldier]

Please have the colors for border/padding/margin areas match the colors already used in firebug (preferably) or chrome dev tools. It makes it easier to switch back and forth between browsers while building pages.

Comment 94

[thinsoldier]

Please don't make it "different" just for the sake of being different. Us developers don't want different. We wan't a complete equivalent to firebug that's fully integrated into Firefox and fully supported by Mozilla.

Comment 95

[Darrin Henein [:darrin]]

(In reply to thinsoldier from comment #93)
> Please have the colors for border/padding/margin areas match the colors
> already used in firebug (preferably) or chrome dev tools. It makes it easier
> to switch back and forth between browsers while building pages.

FWIW, while the exact hues vary, the colors used are consistent with what other browsers/tools are using: "orange" for margin, "yellow" for borders, "green" for padding and "blue" for content.

Comment 96

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

Attachment: patch 1: All tests pass, "only" leaks left.

Currently based on bug 974056 simply because it makes it easier to debug tests by uncommenting devtools.dump.emit in head.js

Comment 97

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

Attachment: 1-box-model-shutdown-fixes-changes-663778.patch

Fixes for a bunch of exceptions on shutdown. These fixes appear to improve the accuracy of the leak detector during test runs.

Comment 98

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

Attachment: 2-box-model-layout-view-changes-663778.patch

Changes to the layout view. This includes new styling and the ability to move highlighter guides as each region is hovered.

Comment 99

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

Attachment: 3-box-model-misc-fixes-changes-663778.patch

A few misc fixes:
1. Remove mouseover and mouseout removeEventListeners from the markup view as they are never added in the first place.
2. Remove reference to foldgutter.css from codemirror's editor.js as this missing file was causing hundreds of errors in test runs.
3. A couple of missing semicolons (I just can't help fixing them).
4. Fix querySelectorAll in the NodeListActor. Input from the markup view's search box is used as a querySelector in this method so this change swallows the exception in the case that a bad selector is entered.

Comment 100

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

Attachment: 4-box-model-highlighter-663778.patch

Changes to tests.

I have disabled dom.send_after_paint_to_content for all devtools tests except tilt (just in case it Victor needs it). This pref was causing an infinite loop when the highlighter is displayed e.g. Show highlighter -> MozAfterPaint -> Show highlighter etc.

The rest of the changes are simple test fixes.

Comment 101

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

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #100)
> Created attachment 8386704 [details] [diff] [review]
> 4-box-model-highlighter-663778.patch
> 
> Changes to tests.
> 
> I have disabled dom.send_after_paint_to_content for all devtools tests
> except tilt (just in case it Victor needs it). This pref was causing an
> infinite loop when the highlighter is displayed e.g. Show highlighter ->
> MozAfterPaint -> Show highlighter etc.
> 
> The rest of the changes are simple test fixes.

Actually, strike that, I had reordered the patches. This is the box model highlighter implementation.

Comment 102

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

Attachment: 5-box-model-test-changes-663778.patch

Changes to tests.

I have disabled dom.send_after_paint_to_content for all devtools tests except tilt (just in case it Victor needs it). This pref was causing an infinite loop when the highlighter is displayed e.g. Show highlighter -> MozAfterPaint -> Show highlighter etc.

The rest of the changes are simple test fixes.

Comment 103

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

Some tests are still failing but this shouldn't hold up reviews any longer. See https://tbpl.mozilla.org/?tree=Try&rev=716cbf08fd6b for an example. I think that "ReferenceError: ok is not defined" is quite a classic.

Comment 104

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

Attachment: 1-box-model-shutdown-fixes-changes-663778.patch

Mihai just fixed something that this patch addressed, rebased.

Comment 105

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

Note that devtools.dump.emit is set to true at the moment for markup and inspector tests. This is just a temporary measure as it can be used to work out why tests are failing.

Comment 106

[Patrick Brosset <:pbro>]

Comment on attachment 8386704 [details] [diff] [review]
4-box-model-highlighter-663778.patch

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

Thanks, I see that all my comments raised in the last review have been taken into account.
This patch seems general ok to me, I just have one somewhat important comment about the new inspector-panel update process. See my comments below.
Happy to R+ this patch with this resolved.

::: browser/devtools/inspector/inspector-panel.js
@@ +443,5 @@
>      progress.outstanding.add(done);
>      return done;
>    },
>  
> +  _onHighlighterReady: function() {

If I'm not mistaken, you've introduced this to make sure the "inspector-updated" event is fired only after the highlighter has moved to the node currently being inspected.

Several comments about this:

- it took me some time to understand the logic here. I think it would make it a lot easier to understand if the client-side part of the highlighter (HighlighterUtils in toolbox.js) would signal its process just like any other inspector tool:

let doneUpdating = this.inspector.updating("highlighter");

and then later, when the `highlighter-ready` event has been received:

doneUpdating();

This would make it consistent with the rule-view, breadcrumb, ...
It would also simplify the `checkDone` function (would be back to what it was before).

- another thing is (and I haven't looked into this too much so I might be wrong): what about when the highlighter isn't used? The update process will then never be done. Indeed, `this._highlighterReady` is only set to true when the `highlighter-ready` event is received, but if we're not highlighting the node when selecting it (I think this happens when we navigate to a new page and auto-select the default node), then it means `inspector-updated` will never be emitted, which may be bad.

::: toolkit/devtools/LayoutHelpers.jsm
@@ +420,5 @@
> +
> +
> +
> +  /********************************************************************
> +   * GetBoxQuads POLYFILL START

Can you add a comment explaining why we need it and giving the number of the bug we need fixed before removing this.

::: toolkit/devtools/server/actors/highlighter.js
@@ +498,5 @@
>        } else {
>          // Nothing to highlight (0px rectangle like a <script> tag for instance)
>          this.hide();
>        }
> +      events.emit(this._inspector.walker, "highlighter-ready");

Not a big deal but I think it would be better for the highlighter to emit its own events and then have the HighlighterActor do the bridge to the inspector. This would make the highlighter API more self-contained and would remove the need for the second constructor parameter.

this.emit("ready");

and then in the HighlighterActor:

this._boxModelHighlighter.on("ready", () => {
  events.emit(this._inspector.walker, "highlighter-ready");
});

Same comment for the `highlighter-hide` event.

Comment 107

[Patrick Brosset <:pbro>]

Comment on attachment 8386712 [details] [diff] [review]
1-box-model-shutdown-fixes-changes-663778.patch

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

I have to admit I'm not very comfortable with these changes.

The goal is to make sure we don't throw various errors when the toolbox is closed.
I've seen this happen during tests a lot, but for this case, I'm pretty confident we can fix the tests themselves instead of adding defensive if conditions in certain functions. It especially feels strange when we're adding an `if(isDestroyed){return}` to one function only. Either we find a global way of making destroyed class instances safe to call, or we don't call their methods at all.
Again, if this is only a problem during tests, this should be easy to fix in each test (I'm fixing some of those in bug 968727).

It also seems to happen when quickly opening and closing the toolbox (via the keyboard shortcuts for instance), but fixing this should probably be done in a separate bug.

Checking that things still exist in a callback or promise handler seems fine because these are called asynchronously and an object may have been destroyed in the meantime, but in other cases, I think it would be better solved by tracing the issue back to the root cause rather than making some functions more defensive.

If I'm not mistaken, a lot of the errors are due to the inspector trying to select a new node and the toolbox being closed before everything can be updated accordingly. If this is indeed the case, it should be relatively easy to come up with something at inspector-panel level that tells us if we are being destroyed, and if yes, then selection.js shouldn't try to fire `new-node-front` events.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +251,5 @@
>      this.panel.hidePopup();
>    },
>  
>    isShown: function() {
> +    if (!this.panel) {

I see that most changes in this patch are essentially making the code more defensive so that we can avoid errors in certain situations like when closing a window or ending tests.
I'm not against adding this condition here, but it feels pretty strange to have it here and in no other function of this class. Why not in `show`, `hide`, `setSize`, `empty`, .... ?
I think the best solution would be to find out why `isShown` is being called *after* the panel has been removed.
Does this only happen when running tests?
If yes, I'm pretty sure it's easy to avoid. I've been working recently on refactoring tests, and most of time, just waiting for events solve these kinds of problems.

::: browser/devtools/webconsole/webconsole.js
@@ +408,5 @@
>      };
>  
> +    if (!this.webConsoleClient) {
> +      // Toolbox is in the process of shutting down.
> +      return deferred.resolve();

Can you move this `if` to the top of the function and return instead `promise.resolve()`.
Also maybe check with msucan why/when does `this.webConsoleClient` is falsy?

::: toolkit/devtools/server/actors/inspector.js
@@ +1191,5 @@
>      if (options.center && options.start) {
>        throw Error("Can't specify both 'center' and 'start' options.");
>      }
> +    if (!node) {
> +      return;

The return type of this protocol method is `array:domnode`.
I'm guessing it should be made nullable too, or maybe you should be returning an empty array here.

::: toolkit/devtools/server/actors/styles.js
@@ +446,5 @@
>      }
>    },
>  
>    getLayout: method(function(node, options) {
> +    if (!node) {

I think a better pattern for protocol methods is to return a rejected promise.
It avoids having to come up with a new json object return type.

::: toolkit/devtools/server/protocol.js
@@ +545,5 @@
>     * @returns a request packet.
>     */
>    write: function(fnArgs, ctx) {
>      let str = JSON.stringify(this.template, (key, value) => {
> +      if (value instanceof Arg && fnArgs) {

This seems harmless enough, but I guess worth asking dcamp's feedback, no?

::: toolkit/devtools/styleinspector/css-logic.js
@@ +170,5 @@
>  
>      this._matchedRules = null;
>      this._matchedSelectors = null;
>      let win = this.viewedDocument.defaultView;
> +    if (win) {

I think this check would be better at the top of the function, coupled with the already existing `if (!aViewedElement) {`
This if should both check if the the element exists and if it has a parent window.

Comment 108

[Patrick Brosset <:pbro>]

Note that I haven't entirely reviewed attachment 8386712 [details] [diff] [review], I'd like your point of view on my general comment first, and maybe hear what other thinks.

Comment 109

[Patrick Brosset <:pbro>]

Comment on attachment 8386693 [details] [diff] [review]
2-box-model-layout-view-changes-663778.patch

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

::: browser/devtools/layoutview/view.js
@@ +261,5 @@
>  let elts;
>  let tooltip;
>  
> +let onmouseover = function(e) {
> +  let region = e.target.getAttribute("data-box");

This should work: `e.target.dataset.box`

Comment 110

[Brian Grinstead [:bgrins]]

Comment on attachment 8386693 [details] [diff] [review]
2-box-model-layout-view-changes-663778.patch

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

::: browser/devtools/layoutview/view.js
@@ +261,5 @@
>  let elts;
>  let tooltip;
>  
> +let onmouseover = function(e) {
> +  let region = e.target.getAttribute("data-box");

Note: if this is a XUL element, dataset will be undefined

Comment 111

[Patrick Brosset <:pbro>]

Comment on attachment 8386703 [details] [diff] [review]
3-box-model-misc-fixes-changes-663778.patch

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

LGTM

Comment 112

[Patrick Brosset <:pbro>]

Comment on attachment 8386703 [details] [diff] [review]
3-box-model-misc-fixes-changes-663778.patch

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

LGTM

::: toolkit/devtools/server/actors/inspector.js
@@ +1405,5 @@
> +
> +    try {
> +      nodeList = baseNode.rawNode.querySelectorAll(selector);
> +    } catch(e) {
> +      // Bad selector. Do nothing as the selector can come from a searchbox.

Actually, I had originally done the same change in another bug but finally removed the try/catch because of this comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=962647#c15
I think it makes more sense to keep the exception on the server side as it may be useful to client code to know that a selector was invalid.

Comment 113

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

Attachment: 4-Addressed comments and made highlighter persistent for tests

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #106)
> Comment on attachment 8386704 [details] [diff] [review]
> 4-box-model-highlighter-663778.patch
> 
> Review of attachment 8386704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, I see that all my comments raised in the last review have been taken
> into account.
> This patch seems general ok to me, I just have one somewhat important
> comment about the new inspector-panel update process. See my comments below.
> Happy to R+ this patch with this resolved.
> 
> ::: browser/devtools/inspector/inspector-panel.js
> @@ +443,5 @@
> >      progress.outstanding.add(done);
> >      return done;
> >    },
> >  
> > +  _onHighlighterReady: function() {
> 
> If I'm not mistaken, you've introduced this to make sure the
> "inspector-updated" event is fired only after the highlighter has moved to
> the node currently being inspected.
> 
> Several comments about this:
> 
> - it took me some time to understand the logic here. I think it would make
> it a lot easier to understand if the client-side part of the highlighter
> (HighlighterUtils in toolbox.js) would signal its process just like any
> other inspector tool:
> 
> let doneUpdating = this.inspector.updating("highlighter");
> 
> and then later, when the `highlighter-ready` event has been received:
> 
> doneUpdating();
> 
> This would make it consistent with the rule-view, breadcrumb, ...
> It would also simplify the `checkDone` function (would be back to what it
> was before).
> 
> - another thing is (and I haven't looked into this too much so I might be
> wrong): what about when the highlighter isn't used? The update process will
> then never be done. Indeed, `this._highlighterReady` is only set to true
> when the `highlighter-ready` event is received, but if we're not
> highlighting the node when selecting it (I think this happens when we
> navigate to a new page and auto-select the default node), then it means
> `inspector-updated` will never be emitted, which may be bad.
> 

I have removed highlighter-ready from this.inspector.updating. We just don't
need it. If a tool only needs to wait for the highlighter it can listen for the
highlighter-ready event directly. This also addresses all of your comments
above.

> ::: toolkit/devtools/LayoutHelpers.jsm
> @@ +420,5 @@
> > +
> > +
> > +
> > +  /********************************************************************
> > +   * GetBoxQuads POLYFILL START
> 
> Can you add a comment explaining why we need it and giving the number of the
> bug we need fixed before removing this.
> 

Done

> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +498,5 @@
> >        } else {
> >          // Nothing to highlight (0px rectangle like a <script> tag for instance)
> >          this.hide();
> >        }
> > +      events.emit(this._inspector.walker, "highlighter-ready");
> 
> Not a big deal but I think it would be better for the highlighter to emit
> its own events and then have the HighlighterActor do the bridge to the
> inspector. This would make the highlighter API more self-contained and would
> remove the need for the second constructor parameter.
> 
> this.emit("ready");
> 
> and then in the HighlighterActor:
> 
> this._boxModelHighlighter.on("ready", () => {
>   events.emit(this._inspector.walker, "highlighter-ready");
> });
> 
> Same comment for the `highlighter-hide` event.

Agreed, done.

I also needed to add a pref (devtools.inspector.highlighter.autohide) to prevent the highlighter from hiding during tests. This is because it uses a timeout of 1000ms before hiding. If a server is under load and it takes e.g. markup-view more than 1000ms to send it's ready event then the highlighter is already hidden. That means that on a line directly after highlighting a node the highlighter may no longer be visible.

Comment 114

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

Attachment: 1 Addressed comments

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #107)
> Comment on attachment 8386712 [details] [diff] [review]
> 1-box-model-shutdown-fixes-changes-663778.patch
> 
> Review of attachment 8386712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have to admit I'm not very comfortable with these changes.
> 
> The goal is to make sure we don't throw various errors when the toolbox is
> closed.
> I've seen this happen during tests a lot, but for this case, I'm pretty
> confident we can fix the tests themselves instead of adding defensive if
> conditions in certain functions. It especially feels strange when we're
> adding an `if(isDestroyed){return}` to one function only. Either we find a
> global way of making destroyed class instances safe to call, or we don't
> call their methods at all.
> Again, if this is only a problem during tests, this should be easy to fix in
> each test (I'm fixing some of those in bug 968727).
> 
> It also seems to happen when quickly opening and closing the toolbox (via
> the keyboard shortcuts for instance), but fixing this should probably be
> done in a separate bug.
> 
> Checking that things still exist in a callback or promise handler seems fine
> because these are called asynchronously and an object may have been
> destroyed in the meantime, but in other cases, I think it would be better
> solved by tracing the issue back to the root cause rather than making some
> functions more defensive.
> 
> If I'm not mistaken, a lot of the errors are due to the inspector trying to
> select a new node and the toolbox being closed before everything can be
> updated accordingly. If this is indeed the case, it should be relatively
> easy to come up with something at inspector-panel level that tells us if we
> are being destroyed, and if yes, then selection.js shouldn't try to fire
> `new-node-front` events.
>

This is a good point and we should address it sometime but as discussed in the daily meeting this is caused by the toolbox being closed and opened again in between tests. We could ensure that we wait for all tools to finish dealing with all of their events before allowing the toolbox to close but that would result in a very slow response when closing the toolbox and when switching toolbox tabs, which is not good for users. For now let's just prevent the exceptions that are occurring... this is just a workaround for those issues to make real errors easy to find in test logs.
 
> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +251,5 @@
> >      this.panel.hidePopup();
> >    },
> >  
> >    isShown: function() {
> > +    if (!this.panel) {
> 
> I see that most changes in this patch are essentially making the code more
> defensive so that we can avoid errors in certain situations like when
> closing a window or ending tests.
> I'm not against adding this condition here, but it feels pretty strange to
> have it here and in no other function of this class. Why not in `show`,
> `hide`, `setSize`, `empty`, .... ?
> I think the best solution would be to find out why `isShown` is being called
> *after* the panel has been removed.
> Does this only happen when running tests?
> If yes, I'm pretty sure it's easy to avoid. I've been working recently on
> refactoring tests, and most of time, just waiting for events solve these
> kinds of problems.
> 

See previous comment.

> ::: browser/devtools/webconsole/webconsole.js
> @@ +408,5 @@
> >      };
> >  
> > +    if (!this.webConsoleClient) {
> > +      // Toolbox is in the process of shutting down.
> > +      return deferred.resolve();
> 
> Can you move this `if` to the top of the function and return instead
> `promise.resolve()`.
> Also maybe check with msucan why/when does `this.webConsoleClient` is falsy?
> 

Done

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +1191,5 @@
> >      if (options.center && options.start) {
> >        throw Error("Can't specify both 'center' and 'start' options.");
> >      }
> > +    if (!node) {
> > +      return;
> 
> The return type of this protocol method is `array:domnode`.
> I'm guessing it should be made nullable too, or maybe you should be
> returning an empty array here.
> 

Yup, now returning an empty array.

> ::: toolkit/devtools/server/actors/styles.js
> @@ +446,5 @@
> >      }
> >    },
> >  
> >    getLayout: method(function(node, options) {
> > +    if (!node) {
> 
> I think a better pattern for protocol methods is to return a rejected
> promise.
> It avoids having to come up with a new json object return type.
> 

Done

> ::: toolkit/devtools/server/protocol.js
> @@ +545,5 @@
> >     * @returns a request packet.
> >     */
> >    write: function(fnArgs, ctx) {
> >      let str = JSON.stringify(this.template, (key, value) => {
> > +      if (value instanceof Arg && fnArgs) {
> 
> This seems harmless enough, but I guess worth asking dcamp's feedback, no?
> 

Emailed Dave

> ::: toolkit/devtools/styleinspector/css-logic.js
> @@ +170,5 @@
> >  
> >      this._matchedRules = null;
> >      this._matchedSelectors = null;
> >      let win = this.viewedDocument.defaultView;
> > +    if (win) {
> 
> I think this check would be better at the top of the function, coupled with
> the already existing `if (!aViewedElement) {`
> This if should both check if the the element exists and if it has a parent
> window.

Not possible. To get win we need this.viewedDocument which is what the core of this method is doing.

Comment 115

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

Comment on attachment 8386708 [details] [diff] [review]
5-box-model-test-changes-663778.patch

I will go over these tests again now that we have a green try as we probably don't need all changes.

Comment 116

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #113)
> I also needed to add a pref (devtools.inspector.highlighter.autohide) to
> prevent the highlighter from hiding during tests. This is because it uses a
> timeout of 1000ms before hiding. If a server is under load and it takes e.g.
> markup-view more than 1000ms to send it's ready event then the highlighter
> is already hidden. That means that on a line directly after highlighting a
> node the highlighter may no longer be visible.

(I didn't look at the actual change so take this with a grain of salt, but) Keep in mind that new prefs make browser startup slower, so if this is something that only tests need, it might be better to use a property in the highlighter that the test preamble could set unconditionally.

Comment 117

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

(In reply to Panos Astithas [:past] from comment #116)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #113)
> > I also needed to add a pref (devtools.inspector.highlighter.autohide) to
> > prevent the highlighter from hiding during tests. This is because it uses a
> > timeout of 1000ms before hiding. If a server is under load and it takes e.g.
> > markup-view more than 1000ms to send it's ready event then the highlighter
> > is already hidden. That means that on a line directly after highlighting a
> > node the highlighter may no longer be visible.
> 
> (I didn't look at the actual change so take this with a grain of salt, but)
> Keep in mind that new prefs make browser startup slower, so if this is
> something that only tests need, it might be better to use a property in the
> highlighter that the test preamble could set unconditionally.

There have also been a few user requests for this feature so allowing them to set it via about:config is probably a win. Also, that change has already landed as part of another bug.

Comment 118

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

Attachment: 1-box-model-layout-view-changes-663778.patch

Comment 119

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

Attachment: 2-box-model-misc-fixes-changes-663778.patch

Comment 120

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

Attachment: 3-box-model-highlighter-663778.patch

pbrosset has already reviewed this but I have added the devtools.inspector.highlighter.autohide pref as the highlighter hiding itself was our last race condition.

Comment 121

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

Attachment: 4-box-model-tests-charset-mozafterpaint-changes-663778.patch

These are the most repetitive changes to tests. I broke this out from the main test patch as it makes the review way easier.

Comment 122

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

Attachment: 5-box-model-test-changes-663778.patch

Main test changes.

Comment 123

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

Oh, and of course, our green try:
https://tbpl.mozilla.org/?tree=Try&rev=d20b65200065

Comment 124

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

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #117)
> There have also been a few user requests for this feature so allowing them
> to set devtools.inspector.highlighter.autohide via about:config is probably
> a win. Also, that change has already landed as part of another bug.

I am wrong, this did not land as part of another bug. Personally I think a pref makes the most sense but if jwalker thinks it should be a toggle I am happy to change it.

Comment 125

[Joe Walker [:jwalker]]

Comment on attachment 8388263 [details] [diff] [review]
3-box-model-highlighter-663778.patch

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

::: browser/devtools/framework/toolbox.js
@@ +1162,5 @@
>  
>      // Releasing the walker (if it has been created)
>      // This can fail, but in any case, we want to continue destroying the
>      // inspector/highlighter/selection
> +    let walker = this._destroying = this._walker ? this._walker.release() : promise.resolve();

It looks like we might have blown 80 columns and there's a confusion as to if the double equals is double assignment.
So how about:

let walker = (this._destroying = this._walker) ?
             this._walker.release() :
             promise.resolve();

Comment 126

[Joe Walker [:jwalker]]

Comment on attachment 8388265 [details] [diff] [review]
4-box-model-tests-charset-mozafterpaint-changes-663778.patch

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

::: browser/devtools/markupview/test/head.js
@@ +14,5 @@
>  
> +// dom.send_after_paint_to_content is set to true (non-default) in
> +// 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);

Do we actually need to do this everywhere?

Comment 127

[Joe Walker [:jwalker]]

Comment on attachment 8388266 [details] [diff] [review]
5-box-model-test-changes-663778.patch

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

::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
@@ +33,5 @@
>      Task.spawn(function() {
>        yield hoverH1InMarkupView();
>        yield assertH1Highlighted();
> +
> +      // We no longer hide the highlighter during tests so no need to check that

In general we shouldn't be commenting on how it used to be unless that's vital to understanding now. We've got VCS to explain deletions.

Comment 128

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

(In reply to Joe Walker [:jwalker] from comment #125)
> Comment on attachment 8388263 [details] [diff] [review]
> 3-box-model-highlighter-663778.patch
> 
> Review of attachment 8388263 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/framework/toolbox.js
> @@ +1162,5 @@
> >  
> >      // Releasing the walker (if it has been created)
> >      // This can fail, but in any case, we want to continue destroying the
> >      // inspector/highlighter/selection
> > +    let walker = this._destroying = this._walker ? this._walker.release() : promise.resolve();
> 
> It looks like we might have blown 80 columns and there's a confusion as to
> if the double equals is double assignment.
> So how about:
> 
> let walker = (this._destroying = this._walker) ?
>              this._walker.release() :
>              promise.resolve();

Makes sense, fixed.

(In reply to Joe Walker [:jwalker] from comment #127)
> Comment on attachment 8388266 [details] [diff] [review]
> 5-box-model-test-changes-663778.patch
> 
> Review of attachment 8388266 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
> @@ +33,5 @@
> >      Task.spawn(function() {
> >        yield hoverH1InMarkupView();
> >        yield assertH1Highlighted();
> > +
> > +      // We no longer hide the highlighter during tests so no need to check that
> 
> In general we shouldn't be commenting on how it used to be unless that's
> vital to understanding now. We've got VCS to explain deletions.

(In reply to Joe Walker [:jwalker] from comment #126)
> Comment on attachment 8388265 [details] [diff] [review]
> 4-box-model-tests-charset-mozafterpaint-changes-663778.patch
> 
> Review of attachment 8388265 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/markupview/test/head.js
> @@ +14,5 @@
> >  
> > +// dom.send_after_paint_to_content is set to true (non-default) in
> > +// 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);
> 
> Do we actually need to do this everywhere?

We had discussed this in our daily meetings and the general consensus was that in the future tools will most likely involve tests that use the highlighter. Victor also wanted it enabled for Tilt tests because flooding things with useless MozAfterPaint stuff is not a good idea.

Comment 129

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

Attachment: 3-box-model-highlighter-663778.patch

Rebased

Comment 130

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

Attachment: 4-box-model-tests-charset-changes-663778.patch

Moved mozafterpaint out to new patch as we are now using a flag instead of a pref.

Comment 131

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

Attachment: 5-box-model-test-changes-663778.patch

Rebased

Comment 132

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

Attachment: 6-testing-flag-663778.patch

Moved mozafterpaint fix out to new patch as we are now using a flag instead of a pref.

Our head.js files now contain gDevTools.testing = true which prevents the highlighter from hiding and disables dom.send_after_paint_to_content, which we agreed in a meeting should be false for all of our tools because they are all likely to interact with the highlighter at some point.

Try run to follow.

Comment 133

[Joe Walker [:jwalker]]

Comment on attachment 8389835 [details] [diff] [review]
6-testing-flag-663778.patch

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

::: browser/devtools/profiler/test/head.js
@@ +28,3 @@
>  registerCleanupFunction(function () {
>    helpers = null;
> +  gDevTools.testing = false;

This bugged me before and it still bugs me...
Sometimes we integrate into existing clean-up functions, and sometimes we have a new one. I think it makes sense if we keep them the same system everywhere rather than merging them into existing clean-up function, unless there is some cost in a clean-up function that I'm not aware of. Using the same block everywhere will help us remove them all at the same time.

::: browser/devtools/styleinspector/test/head.js
@@ +30,5 @@
> +SimpleTest.registerCleanupFunction(() => {
> +  gDevTools.testing = false;
> +
> +  Services.prefs.clearUserPref("devtools.debugger.log");
> +  Services.prefs.clearUserPref("devtools.dump.emit");

This shouldn't be turned on by default?

Comment 134

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

Attachment: 6-testing-flag-663778.patch

(In reply to Joe Walker [:jwalker] from comment #133)
> Comment on attachment 8389835 [details] [diff] [review]
> 6-testing-flag-663778.patch
> 
> Review of attachment 8389835 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/profiler/test/head.js
> @@ +28,3 @@
> >  registerCleanupFunction(function () {
> >    helpers = null;
> > +  gDevTools.testing = false;
> 
> This bugged me before and it still bugs me...
> Sometimes we integrate into existing clean-up functions, and sometimes we
> have a new one. I think it makes sense if we keep them the same system
> everywhere rather than merging them into existing clean-up function, unless
> there is some cost in a clean-up function that I'm not aware of. Using the
> same block everywhere will help us remove them all at the same time.
> 

I am happier with it in one block too... done.

> ::: browser/devtools/styleinspector/test/head.js
> @@ +30,5 @@
> > +SimpleTest.registerCleanupFunction(() => {
> > +  gDevTools.testing = false;
> > +
> > +  Services.prefs.clearUserPref("devtools.debugger.log");
> > +  Services.prefs.clearUserPref("devtools.dump.emit");
> 
> This shouldn't be turned on by default?

If you mean devtools.dump.emit then no, it is amazingly useful for debugging issues but it logs a lot of information that makes test logs difficult to read.

It is a simple matter of switching it on in head.js, pushing to try, reproducing the orange and then reading the log.

Comment 135

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

Attachment: 4-box-model-tests-charset-changes-663778.patch

Added a null check in a destructor to fix a webconsole test and return null from _highlightBoxModel to fix XPCShell test failures.

Comment 136

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

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

Comment 137

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_dbg_listtabs-02.js leaks.
https://hg.mozilla.org/integration/fx-team/rev/583b3de7f71a

https://tbpl.mozilla.org/php/getParsedLog.php?id=36079631&tree=Fx-Team

Comment 138

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

Attachment: 3-box-model-highlighter-663778.patch

Added null check to destructor.

Comment 139

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

Attachment: 6-testing-flag-663778.patch

Fixed a number of tests.

Comment 140

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

Because these changes uncover leaks in unrelated tests we can't wait for try runs to complete before pushing as new leaks are bound to appear (it turned out to be 3 different leaks that caused the previous set of patches to bounce).

bc and xpcshell tests are all green locally so all we can do is land when not many others are landing... fingers crossed.

https://hg.mozilla.org/integration/fx-team/rev/c20ad6e9c62e
https://hg.mozilla.org/integration/fx-team/rev/9b860b0a7282
https://hg.mozilla.org/integration/fx-team/rev/8dea188bf8a0
https://hg.mozilla.org/integration/fx-team/rev/fad8f0885e5b
https://hg.mozilla.org/integration/fx-team/rev/acf2659a7478
https://hg.mozilla.org/integration/fx-team/rev/436a9dfc6cb4

Comment 141

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c20ad6e9c62e
https://hg.mozilla.org/mozilla-central/rev/9b860b0a7282
https://hg.mozilla.org/mozilla-central/rev/8dea188bf8a0
https://hg.mozilla.org/mozilla-central/rev/fad8f0885e5b
https://hg.mozilla.org/mozilla-central/rev/acf2659a7478
https://hg.mozilla.org/mozilla-central/rev/436a9dfc6cb4