1402633 - Display the window dimensions with the ruler tools

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

Similar to Chrome DevTools' ruler, we should display the window dimension when the ruler tool is toggled on.

Comment 1

[Liam Hodgins]

Attachment: Bug 1402633 - Display the window dimensions with the ruler tools.

Review commit: https://reviewboard.mozilla.org/r/182958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182958/

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Gonna pass the final review to Patrick. I think this is good to go, but since I helped with the coding directions, I would like a second eye on it.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8911503 [details]
Bug 1402633 - Display the window dimensions with the ruler tools.

https://reviewboard.mozilla.org/r/182958/#review190054

Looks good, thank you!
I have 4 comments about this (and one tiny comment in the code below):

- The most important I think is that we need a new automated test for this. Something that will check that the new element is visible, and that it contains the right dimensions, even when you resize. There are 2 rulers tests already that you can use as inspiration I think: \devtools\client\inspector\test\browser_inspector_highlighter-rulers_01.js (and 02.js).
- My second comment is about the design for this. The text inside the infobar is much bigger than the one in the rulers. I would reduce the font-size down a little bit. Not make it as small as text in the rulers itself, but a bit smaller still. 
- Gabriel and I talked about what Chrome does, which is display this infobar only on resize. I kind of like that, but am ready to go for either solutions. The current one is nice because it's ready and easy. Having said that, the area this infobar covers might get in the way of inspecting stuff in the upper right corner. In any case, if we're going with show on resize only, we should do that in a separate commit or bug, because that will become more involved.
- Finally, I was surprised at first that the width and height changed while zooming the page in our out, and then realized that that was expected because the rulers do follow the zoom level too. In fact I found bug 1144587 when we had filed when we implemented the rulers originally. So, just something to keep in mind for later. That we'll eventually want the rulers and infobar to remain fixed even when zooming and show the actual px for the viewport, not the css pixels.

Anyway, good work on this so far.

::: devtools/server/actors/highlighters/rulers.js:205
(Diff revision 1)
> +    createNode(window, {
> +      nodeType: "span",
> +      parent: viewportInfobarContainer,
> +      attributes: {
> +        "class": "viewport-dimensions",
> +        "id": "viewport-dimensions"
> +      },
> +      prefix
> +    });

Is this node used at all? I don't see it being used in JS nor styled in CSS. So it looks like we should remove this part.

Comment 4

[Liam Hodgins]

For the automated tests, when the windows needs to be resized, how is this done in a testing file? The testActor doesn't seem to having any method that would perform this.

Comment 5

[Patrick Brosset <:pbro>]

That's a good question. Off the top of my head, I don't know of a test that does that already. Also, I don't know whether this would be a problem for other tests running after. We would have to restore the previous window size before ending the test.

I have an idea that might be simpler: resizing devtools instead.

Indeed, the default position of the toolbox is to be docked at the bottom of the browser window. So your tests could a. open the toolbox, b. show the rulers, c. check that the size element appears and get its content, d. make the toolbox a bit taller, e. check that the size element is still here and contains a different content than before (you could go as far as parsing the text content and checking that the window height is now smaller than it used to be).

I'm unsure about how to change the toolbox size dynamically during a test though. I know that its size is stored in the pref devtools.toolbox.footer.height, but changing the value of this pref does not resize devtools dynamically while it is open.

I think you might have to access the splitter element directly with toolbox._host._splitter to move it programmatically.
I hope this helps!

Comment 6

[Liam Hodgins]

Attachment: Bug 1402633 - Display the window dimensions with the ruler tools

Review commit: https://reviewboard.mozilla.org/r/191858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191858/

Comment 7

[Liam Hodgins]

Comment on attachment 8911503 [details]
Bug 1402633 - Display the window dimensions with the ruler tools.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182958/diff/1-2/

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8920911 [details]
Bug 1402633 - Display the window dimensions with the ruler tools

Clearing review for now since the 2 commits actually need to be squashed. Also, change the r=gl to r=pbro.

Comment 9

[Liam Hodgins]

Comment on attachment 8920911 [details]
Bug 1402633 - Display the window dimensions with the ruler tools

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191858/diff/1-2/

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8920911 [details]
Bug 1402633 - Display the window dimensions with the ruler tools

https://reviewboard.mozilla.org/r/191858/#review197620

Looks great, thanks! 
I just have a few more comments, so I'll clear the R? flag. Please ask again once you've addressed these comments.

::: devtools/client/inspector/test/browser_inspector_highlighter-rulers_03.js:7
(Diff revision 2)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +// Test the creation of the geometry highlighter elements.

Please update this comment, it seems to have been copy/pasted from another file and does not describe what this test does.

::: devtools/client/inspector/test/browser_inspector_highlighter-rulers_03.js:32
(Diff revision 2)
> +
> +  yield highlighter.finalize();
> +});
> +
> +function* isShown(highlighterFront, inspector, testActor) {
> +  info("Checking the highlighter is displayed when asked");

Here, you're not checking that the highlighter is displayed, but that the infobar is. So please update this comment.

::: devtools/client/inspector/test/browser_inspector_highlighter-rulers_03.js:39
(Diff revision 2)
> +  // ones, so the body is given
> +  let body = yield getNodeFront("body", inspector);
> +  yield highlighterFront.show(body);
> +
> +  let hidden = yield testActor.getHighlighterNodeAttribute(
> +  ID + "viewport-infobar-container", "hidden", highlighterFront);

Please use `${ID}viewport-infobar-container` just like you did in the next function.

::: devtools/client/inspector/test/browser_inspector_highlighter-rulers_03.js:39
(Diff revision 2)
> +  // ones, so the body is given
> +  let body = yield getNodeFront("body", inspector);
> +  yield highlighterFront.show(body);
> +
> +  let hidden = yield testActor.getHighlighterNodeAttribute(
> +  ID + "viewport-infobar-container", "hidden", highlighterFront);

nit: We usually indent wrapped lines with 2 extra spaces.

::: devtools/client/inspector/test/browser_inspector_highlighter-rulers_03.js:41
(Diff revision 2)
> +  yield highlighterFront.show(body);
> +
> +  let hidden = yield testActor.getHighlighterNodeAttribute(
> +  ID + "viewport-infobar-container", "hidden", highlighterFront);
> +
> +  isnot(hidden, "true", "highlighter is visible after show");

Same comment about updating the text here. It should say that the infobar is visible, not the highlighter.

::: devtools/client/inspector/test/browser_inspector_highlighter-rulers_03.js:59
(Diff revision 2)
> +
> +  is(dimensionText, windowText, "Dimension text was created successfully");
> +}
> +
> +function* resizeInspector(highlighterFront, inspector, testActor) {
> +  info("Moving inspector to left size");

Rephrasing suggestion: "Docking the toolbox to the side of the browser to change the window size".

::: devtools/client/inspector/test/browser_inspector_highlighter-rulers_03.js:64
(Diff revision 2)
> +function* recheckRightLabelsContent(highlighterFront, inspector, testActor) {
> +  info("Checking the rulers dimension tooltip have new correct text");
> +  let windowDimensions = yield testActor.getWindowDimensions();
> +  let windowHeight = Math.round(windowDimensions.height);
> +  let windowWidth = Math.round(windowDimensions.width);
> +  let windowText = windowHeight + "px \u00D7 " + windowWidth + "px";
> +  let dimensionText = yield testActor.getHighlighterNodeTextContent(
> +    `${ID}viewport-infobar-container`, highlighterFront);
> +  is(dimensionText, windowText, "Checking splitter");
> +}

This is exactly the same function as `hasRightLabelsContent`. No need to duplicate the code, you can just call `hasRightLabelsContent` again after `resizeInspector`.

Comment 11

[Liam Hodgins]

Comment on attachment 8920911 [details]
Bug 1402633 - Display the window dimensions with the ruler tools

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191858/diff/2-3/

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8920911 [details]
Bug 1402633 - Display the window dimensions with the ruler tools

https://reviewboard.mozilla.org/r/191858/#review200894

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7722636c165f
Display the window dimensions with the ruler tools r=pbro

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7722636c165f

Comment 15

[Chris Mills [:cmills]]

I have documented this:

I've updated the Rulers page with this information:
https://developer.mozilla.org/en-US/docs/Tools/Rulers

I've added a note to the Fx59 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/59#Changes_for_web_developers

Let me know if you think this looks OK. Thanks!

Comment 16

[Patrick Brosset <:pbro>]

Looks great, thanks!