Open Bug 1373134 Opened 7 years ago Updated 2 years ago

Scroll into view of a grid cell via the grid outline

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: micah, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 9 obsolete files)

Bug1373134.patch
43.09 KB, patch
Details | Diff | Splinter Review 
Right-clicking a grid cell in the grid outline will open a context menu where we can click on a menu item that will scroll into view of the grid cell on the web page.
Priority: -- → P3
Comment on attachment 8879372 [details]
Bug 1373134 - Add a context menu to scroll into view of a grid cell in the grid outline.

Passing this one to pbro.
Attachment #8879372 - Flags: review?(gl) → review?(pbrosset)
Comment on attachment 8879372 [details]
Bug 1373134 - Add a context menu to scroll into view of a grid cell in the grid outline.

https://reviewboard.mozilla.org/r/150678/#review162030

Thanks.
I see a few problems with this change:
- The "Scroll to view" string is hard coded in English here. DevTools is localized so we can't hard code strings like that. See the Grid.js component for an example of how the l10n module is used to get translated strings.
- You are re-creating your own context menu here (using a <div>), but we already have a context menu library that you should use. You can see an example of the Menu and MenuItem classes being used in style-inspector-menu.js.
- Finally, you implemented the actual scroll in the css-grid.js highlighter module. I don't think this should be done here. The highlighter's job is to show lines etc. on the page, and it only has a show method. So here you're forced to use the show method to just scroll to the node. It feels a bit out of place. I think it would be better to use a method on the inspector instead. In fact we already have one (`scrollIntoView`). You could add optional parameters to it: xOffset and yOffset. This way you'd be able to scroll to the node (the grid container) passing the offset that corresponds to the position of the cell.

Oh and, I'd like to know if there's been UI discussions about this. I see the value of scrolling a given cell into view, but I have 2 remarks, and I don't know if these have been discussed already:
- what about clicking on the cell instead of adding a menu? 
- What about having a contextual menu on the grid DOM container instead, with a "scroll node into view" instead? I find myself very often having to click on the node to select it in the inspector, and then right-clicking on the node in the inspector to use the "scroll into view" menu. On a page with many grids, it's frustrating to not see the grid in the page. In fact, maybe clicking on the node should just scroll the grid into view, and clicking on the inspector icon should select it in the inspector.
Attachment #8879372 - Flags: review?(pbrosset)
Comment on attachment 8879372 [details]
Bug 1373134 - Add a context menu to scroll into view of a grid cell in the grid outline.

https://reviewboard.mozilla.org/r/150678/#review162030

Thanks for the review! I'll be using your suggestions when I work on this again.

We've (gl and I) had a small discussion about an alternative to the context menu, which is using modified keys to scroll to the cell when the cursor hovers over it in the grid outline. I think using modified keys, or even just clicking on the cell, feels better. But I can see the use of a context menu would allow for future functionality to be added easily, and also in a centralized place, for inspecting single grid cells.
I worked on scrolling to grid containers via toggling a grid item in the Layout panel's grid list. You are also able to scroll to a grid by holding the ctrl key and hovering over a cell within the grid outline, or by toggling the "Automatically scroll to grid cell" checkbox.

I uploaded a GIF demonstrating the above:
https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/demos/Peek%202017-07-24%2020-37.gif

The code: 
https://github.com/TigleyM/firefox-devtools-patches/blob/master/WIP/Bug1373134_autoscroll.patch

Overall, we don't necessarily need the context menu to easily navigate a page with several grids or large grids. But I thought I would upload these for everyone to view.
Flags: needinfo?(pbrosset)
Flags: needinfo?(jensimmons)
I really like it! Personally, when I use the layout panel's grid section on a page that has many grids, I always have to do this:
1. click the checkbox for the grid I want to see
2. click on the inspector icon next to it so it becomes selected in the DOM tree
3. right click on the corresponding node in the DOM and select "scroll into view"
Here you're autoscrolling to the node when the checkbox is selected, so 1 step instead of 3! So I like that very much.

I also find the auto-scroll to cell on hover of the outline interesting. I think this can be very useful for large grids.
I'm only wondering about the mechanics of it, and perhaps Jen will have a better idea than me about this. My only comments are:
- holding the ctrl key while hovering might be too hidden
- adding a checkbox is more discoverable, but also our various settings are sort of getting out of hand a little bit, maybe it's time we re-organized them in a drop-down menu, that opens when you click on a cog-like icon, like we do in other places of DevTools.

In any case, I like this much better. I also looked at the code changes, and they look pretty good to me so far.
Flags: needinfo?(pbrosset)
I think we should go with the checkbox option. This was one of the suggestions from Jen I believe when this feature proposal came up - it helps with discoverability and is consistent compare to the context menu and keyboard options. 

I think the settings idea would be a follow up if we believe it is necessary, and should not block landing this feature.
Summary: Add a context menu to scroll into view of a grid cell in the grid outline → Scroll into view of a grid cell via the grid outline
@pbro

We decided to go with the checkbox option and also included holding down the ctrl key while hovering over the outline to scroll into view of a grid cell. 

I agree the settings section will eventually get out of hand as various features are added to the CSS Grid Inspector. I think it would be worth filing a bug that involves re-organizing the settings in a drop-down menu. We can also discuss the design for such a menu there.
Comment on attachment 8900090 [details]
Bug 1373134 - Scroll into view of a grid cell via the grid outline.

https://reviewboard.mozilla.org/r/171442/#review176714

Looks good Micah. Thanks. I have tested the commit locally, I really like the feature.
I have a few more comments, and we need tests with this. Are you going to add tests within the same commit or in a separate one?

::: devtools/client/inspector/grids/actions/highlighter-settings.js:17
(Diff revision 1)
>  } = require("./index");
>  
>  module.exports = {
>  
>    /**
>     * Update the grid highlighter's show grid areas preference.

This comment is now obsolete. Please update it to reflect what the function actually does.

I'm wondering if this action function should be in this module or not.
It has nothing to do with the highlighter in fact. It is not a setting of the highlighter. 
Instead, it is a setting of the outline.

We don't have an outline action module though. Maybe this should go in /actions/grids.js? Or maybe a new /actions/outline.js module?

I'm not too sure about all this, so feel free to choose what makes the more sense.

::: devtools/client/inspector/grids/components/GridOutline.js:146
(Diff revision 1)
>      }
>  
>      return height;
>    },
>  
> -  onHighlightCell({ target, type }) {
> +  onHighlightCell({ ctrlKey, target, type, }) {

nit: remove the ending , after type

::: devtools/client/inspector/grids/grid-inspector.js:539
(Diff revision 1)
> +        const x = Number(xOffset) + 5;
> +        const y = Number(yOffset) - 100;

Where do 5 and -100 come from? What's the reason for these offsets? If they are needed, they will need to be moved to named constants instead.

::: devtools/client/inspector/grids/grid-inspector.js:611
(Diff revision 1)
> +    const x = Number(xOffset) + 5;
> +    const y = Number(yOffset) - 100;

Same here. Having constants will avoid having to repeat these numbers here.

::: devtools/server/actors/inspector.js:693
(Diff revision 1)
> +   *         The y-offset of the selected node.
>     */
> -  scrollIntoView: function () {
> +  scrollIntoView: function (xOffset, yOffset) {
>      this.rawNode.scrollIntoView(true);
> +
> +    if (xOffset && yOffset) {

What if xOffset is 0 but yOffset is 100? Then you won't go in the if statement, which is wrong because you still want to scroll down, right?

So this should check if numbers have been passed.
Attachment #8900090 - Flags: review?(pbrosset)
Hi. I think I like this. I would like to get it behind a flag so I can try it out and see how it feels.

It might be a bit surprising if the shift to another part of the page happens as a jump, instead of a scroll. Is it possible to get the page to move, to scroll, to the new place, instead of jumping there in a flash?

I also wonder.... I noticed that I actually expect the page to scroll to the Grid happen when I click on the target https://monosnap.com/file/J5z70Ao0ojvNaXxvhnALWqfkw60bef.png. Perhaps there's a way to finesse the already existing tools so the scroll never happens in some situations (when clicking certain things, like the checkbook maybe), but always happens in other situations (like clicking the target), which would eliminate the need for a checkbox. 

It's impossible to tell though, without trying it first. So I'm all for shipping this as planned thus far, and deciding how to take it to the next level after we've had a chance to use it in real world situations.
Flags: needinfo?(jensimmons)
A few other thoughts in response to this:

> – holding the ctrl key while hovering might be too hidden
> – adding a checkbox is more discoverable, but also our various settings are
>   sort of getting out of hand a little bit, maybe it's time we re-organized them
>   in a drop-down menu, that opens when you click on a cog-like icon, like we do
>   in other places of DevTools.

I agree that making something a keyboard-command only is too hidden. In fact, if we want people to learn keyboard commands, then we need to include them in writing. Like how any dropdown menu for any Mac application has a list of commands, and their keyboard shortcuts written right there. Then when people are ready, they can memorize the ones they want. 

It strikes me that the list of settings is actually made up of two different types of thing, once we add this new "Automatically scroll to grid cell" one. 

Display line numbers, Display area names, and Extend lines infinitely all affect the _content_ of the Grid Inspector. Of all the things this tool can display, which one do you want? Line names and other things will be added to that list. They all perform the role of allowing users to show/hide the markings.

Automatically scroll to grid cell is different. That adjusts the setting of how this tool behaves. It's a true setting. Maybe you want it to jump around, maybe you don't. It is that kind of tool setting that makes sense placed in a dropdown behind a gear. 

I would not want to put the show/hide content 'settings' in a gear. They are not a setting for how the tool behaves, but rather options for the content of the Inspector.

Ideally we won't have very many, if any tool settings. Like I said in the above comment, maybe we'll reach a level of maturity with scroll-to-grid-cell behavior that simply works well all the time and doesn't need a user toggle. (Although let's not worry about that yet. We need to play with it first in order to know what such defaults might be.) If we end up with several tool settings, that's when a Gear would make sense. Doesn't make sense to create that kind of drop-down with one item.

I do not believe the content options should ever be behind a gear. It should be one click to reach a toggle for showing/hiding the numbers, the names, etc. Again, those are not settings for the tool — those _are_ the tool itself.
Comment on attachment 8900090 [details]
Bug 1373134 - Scroll into view of a grid cell via the grid outline.

https://reviewboard.mozilla.org/r/171442/#review176714

> This comment is now obsolete. Please update it to reflect what the function actually does.
> 
> I'm wondering if this action function should be in this module or not.
> It has nothing to do with the highlighter in fact. It is not a setting of the highlighter. 
> Instead, it is a setting of the outline.
> 
> We don't have an outline action module though. Maybe this should go in /actions/grids.js? Or maybe a new /actions/outline.js module?
> 
> I'm not too sure about all this, so feel free to choose what makes the more sense.

We decided to move this action creator into a new actions/outline.js module.

> Where do 5 and -100 come from? What's the reason for these offsets? If they are needed, they will need to be moved to named constants instead.

The reason for the y-offset is because the scrollIntoView() method will only scroll to the very top edge of the grid highlight, thus cutting off line numbers and infobars I imagine we would want to see as soon as we scroll into view. So I decided to offset the y-coordinate by -100  so that these were also viewable. I will make sure to attach some images to show you what I mean.

As for the x-offset, this was for avoiding the scrollIntoView(xOffset, yOffset) method from evaluating the xOffset argument as falsey. However, I implemented the fix in your last issue of this review.
Attached image WithoutOffsets.gif (obsolete) — 
Notice it will only scroll to the edge of the container when toggling each grid container. It especially breaks scrolling into view of the grids cells because without the y-offset, there is no way to tell which part of the grid container it should be scrolling to besides the top (or end) of the container according to the scrollIntoView() docs I found.
(In reply to Patrick Brosset <:pbro> from comment #10)
> Comment on attachment 8900090 [details]
> Bug 1373134 - Scroll into view of a grid cell via the grid outline.
> 
> https://reviewboard.mozilla.org/r/171442/#review176714
> 
> Looks good Micah. Thanks. I have tested the commit locally, I really like
> the feature.
> I have a few more comments, and we need tests with this. Are you going to
> add tests within the same commit or in a separate one?

I will be adding tests in a separate one!
Comment on attachment 8900090 [details]
Bug 1373134 - Scroll into view of a grid cell via the grid outline.

https://reviewboard.mozilla.org/r/171442/#review177824

Thanks for this new commit Micah.
I'm giving R+ here because the code looks good, but we need 2 things before resolving the bug:
- another commit with an integration test for this,
- a decision for whether we land this feature behind a pref so as to give time to experiment and mature the feature. I tend to agree with Jen that we should do this, like we've done other things.

As for smooth scrolling, I also think this would be good, but I suggest doing this separately. We have a scrollIntoView feature in the inspector's context menu already, and I think if we do it for grid, we should do it there too. But then, do we want it there? Probably yes, but worth having the discussion I think. Could there be a problem on very long pages?

::: devtools/client/inspector/grids/actions/highlighter-settings.js
(Diff revision 2)
>    UPDATE_SHOW_GRID_LINE_NUMBERS,
>    UPDATE_SHOW_INFINITE_LINES,
>  } = require("./index");
>  
>  module.exports = {
> -

Since this file is now unchanged, make sure to revert it completely to its previous state, so that no useless mercurial changes are recorded (i.e. here you removed an empty line that was here before, better add it back).

::: devtools/client/inspector/grids/grid-inspector.js:37
(Diff revision 2)
> +// The x/y offset when scrolling to a grid cell/container. We want to offset the
> +// y-coordinate 100 above the the actual y-coordinate of the container/cell's origin
> +// point so that the innerHeight of the window is not preventing us from seeing the entire
> +// grid and its infobars when we scrollIntoView.

Thanks for the comment. This helps!

::: devtools/client/inspector/grids/grid-inspector.js:41
(Diff revision 2)
>  
> +// The x/y offset when scrolling to a grid cell/container. We want to offset the
> +// y-coordinate 100 above the the actual y-coordinate of the container/cell's origin
> +// point so that the innerHeight of the window is not preventing us from seeing the entire
> +// grid and its infobars when we scrollIntoView.
> +const GRID_SCROLL_XOFFSET = 0;

Tbh, I'm not sure I understood your explanation of the reason for having the x offset in comment 14, sorry. You seemed to say it was not needed anymore, but here it is in the code.
It's good that it's a constant now, but can't we just remove it entirely now? It's 0, and everywhere it's used, it's just added to another number, so I think the constant can be removed entirely.

::: devtools/client/inspector/grids/grid-inspector.js:548
(Diff revision 2)
> +
>      this.showGridHighlighter(node, highlighterSettings);
>  
> +    if (keyModifierState || Services.prefs.getBoolPref(AUTOSCROLL_TO_GRID_CELLS)) {
> +      if (xOffset && yOffset) {
> +        const x = Number(xOffset) + GRID_SCROLL_XOFFSET;

So, here you would just have:
`const x = Number(xOffset);`

::: devtools/client/inspector/grids/grid-inspector.js:620
(Diff revision 2)
>     */
> -  onToggleGridHighlighter(node) {
> +  onToggleGridHighlighter(node, xOffset, yOffset) {
>      let highlighterSettings = this.getGridHighlighterSettings(node);
>      this.toggleGridHighlighter(node, highlighterSettings);
>  
> +    const x = Number(xOffset) + GRID_SCROLL_XOFFSET;

Here too.

::: devtools/client/locales/en-US/layout.properties:10
(Diff revision 2)
>  # LOCALIZATION NOTE This file contains the Layout Inspector strings.
>  # The Layout Inspector is a panel accessible in the Inspector sidebar.
>  
> +# LOCALIZATION NOTE (layout.autoScrollToGridCells):
> +# Setting option to automatically scroll highlighted cell.
> +layout.autoScrollToGridCells=Automatically scroll to grid cell

nit: maybe use "cells" instead of "cell"?

::: devtools/server/actors/inspector.js:691
(Diff revision 2)
> +   *         The x-offset of the selected node.
> +   * @param  {Number||null} yOffset
> +   *         The y-offset of the selected node.
>     */
> -  scrollIntoView: function () {
> +  scrollIntoView: function (xOffset, yOffset) {
>      this.rawNode.scrollIntoView(true);

I agree with Jen that perhaps we should scroll smoothly instead of jumping. Maybe we need to do this as a separate bug though, because if we do, we might also need to do it for the inspector's scrollIntoView context menu option.

In any case, scrolling smoothly can be done with:
`this.rawNode.scrollIntoView({block: "start", inline: "nearest", behavior: "smooth"});`
Attachment #8900090 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset <:pbro> from comment #17)
> Comment on attachment 8900090 [details]
> Bug 1373134 - Scroll into view of a grid cell via the grid outline.
> 
> https://reviewboard.mozilla.org/r/171442/#review177824
> 
> Thanks for this new commit Micah.
> I'm giving R+ here because the code looks good, but we need 2 things before
> resolving the bug:
> - another commit with an integration test for this,
> - a decision for whether we land this feature behind a pref so as to give
> time to experiment and mature the feature. I tend to agree with Jen that we
> should do this, like we've done other things.
> 
> As for smooth scrolling, I also think this would be good, but I suggest
> doing this separately. We have a scrollIntoView feature in the inspector's
> context menu already, and I think if we do it for grid, we should do it
> there too. But then, do we want it there? Probably yes, but worth having the
> discussion I think. Could there be a problem on very long pages?
> 
> ::: devtools/client/inspector/grids/actions/highlighter-settings.js
> (Diff revision 2)
> >    UPDATE_SHOW_GRID_LINE_NUMBERS,
> >    UPDATE_SHOW_INFINITE_LINES,
> >  } = require("./index");
> >  
> >  module.exports = {
> > -
> 
> Since this file is now unchanged, make sure to revert it completely to its
> previous state, so that no useless mercurial changes are recorded (i.e. here
> you removed an empty line that was here before, better add it back).
> 
> ::: devtools/client/inspector/grids/grid-inspector.js:37
> (Diff revision 2)
> > +// The x/y offset when scrolling to a grid cell/container. We want to offset the
> > +// y-coordinate 100 above the the actual y-coordinate of the container/cell's origin
> > +// point so that the innerHeight of the window is not preventing us from seeing the entire
> > +// grid and its infobars when we scrollIntoView.
> 
> Thanks for the comment. This helps!
> 
> ::: devtools/client/inspector/grids/grid-inspector.js:41
> (Diff revision 2)
> >  
> > +// The x/y offset when scrolling to a grid cell/container. We want to offset the
> > +// y-coordinate 100 above the the actual y-coordinate of the container/cell's origin
> > +// point so that the innerHeight of the window is not preventing us from seeing the entire
> > +// grid and its infobars when we scrollIntoView.
> > +const GRID_SCROLL_XOFFSET = 0;
> 
> Tbh, I'm not sure I understood your explanation of the reason for having the
> x offset in comment 14, sorry. You seemed to say it was not needed anymore,
> but here it is in the code.
> It's good that it's a constant now, but can't we just remove it entirely
> now? It's 0, and everywhere it's used, it's just added to another number, so
> I think the constant can be removed entirely.
> 
> ::: devtools/client/inspector/grids/grid-inspector.js:548
> (Diff revision 2)
> > +
> >      this.showGridHighlighter(node, highlighterSettings);
> >  
> > +    if (keyModifierState || Services.prefs.getBoolPref(AUTOSCROLL_TO_GRID_CELLS)) {
> > +      if (xOffset && yOffset) {
> > +        const x = Number(xOffset) + GRID_SCROLL_XOFFSET;
> 
> So, here you would just have:
> `const x = Number(xOffset);`
> 
> ::: devtools/client/inspector/grids/grid-inspector.js:620
> (Diff revision 2)
> >     */
> > -  onToggleGridHighlighter(node) {
> > +  onToggleGridHighlighter(node, xOffset, yOffset) {
> >      let highlighterSettings = this.getGridHighlighterSettings(node);
> >      this.toggleGridHighlighter(node, highlighterSettings);
> >  
> > +    const x = Number(xOffset) + GRID_SCROLL_XOFFSET;
> 
> Here too.
> 
> ::: devtools/client/locales/en-US/layout.properties:10
> (Diff revision 2)
> >  # LOCALIZATION NOTE This file contains the Layout Inspector strings.
> >  # The Layout Inspector is a panel accessible in the Inspector sidebar.
> >  
> > +# LOCALIZATION NOTE (layout.autoScrollToGridCells):
> > +# Setting option to automatically scroll highlighted cell.
> > +layout.autoScrollToGridCells=Automatically scroll to grid cell
> 
> nit: maybe use "cells" instead of "cell"?
> 
> ::: devtools/server/actors/inspector.js:691
> (Diff revision 2)
> > +   *         The x-offset of the selected node.
> > +   * @param  {Number||null} yOffset
> > +   *         The y-offset of the selected node.
> >     */
> > -  scrollIntoView: function () {
> > +  scrollIntoView: function (xOffset, yOffset) {
> >      this.rawNode.scrollIntoView(true);
> 
> I agree with Jen that perhaps we should scroll smoothly instead of jumping.
> Maybe we need to do this as a separate bug though, because if we do, we
> might also need to do it for the inspector's scrollIntoView context menu
> option.
> 
> In any case, scrolling smoothly can be done with:
> `this.rawNode.scrollIntoView({block: "start", inline: "nearest", behavior:
> "smooth"});`

Thanks for the review! I have updated the code with your suggestions. The feature is also hidden behind a pref now with the latest patch update.

I am also working on a second commit for the unit tests right now. I just realized I probably shouldn't have submitted another review request with the since the first commit has been approved. I apologize for that!
Comment on attachment 8902093 [details]
Bug 1373134 - Unit tests.

https://reviewboard.mozilla.org/r/173524/#review178934

Thanks for this test. However this test only checks what happens when various prefs and checkboxes are toggled. We also need a test that verifies that we actually scroll the page when hovering over the outline (also with the modifier key) and when selecting a grid in the list.

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-scroll-into-view-of-grid.js:62
(Diff revision 1)
> +    state.highlighterSettings.scrollToGridCell);
> +  checkbox.click();
> +  yield onCheckboxChange;
> +
> +  ok(Services.prefs.getBoolPref(AUTOSCROLL_TO_GRID_CELLS),
> +    "'Display numbers on lines' is pref on.");

The comment is incorrect. It should be about autoscroll, not about line numbers,
Attachment #8902093 - Flags: review?(pbrosset)
Comment on attachment 8902093 [details]
Bug 1373134 - Unit tests.

https://reviewboard.mozilla.org/r/173524/#review178934

Thanks for the review! I've extended the tests to check for the page actually scrolling when a grid is selected or outline is hovered via ctrlKey or toggling the "Automatically scroll to grid cell" setting. 

My approach for checking if the page actually scrolled was checking if the node next to the currently selected grid was visible in the viewport after highlighting a grid cell close to the next node. I found this approach being similarly used in `client/inspector/markup/test/browser_markup_keybindings_scrolltonode.js` . 

I also created some helper methods to cut down some of the code in the main test. But please let me know if these should moved somewhere else.
Comment on attachment 8902093 [details]
Bug 1373134 - Unit tests.

https://reviewboard.mozilla.org/r/173524/#review180348

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-scroll-into-view-of-grid.js:6
(Diff revision 3)
> +// Tests that the grid outline does reflect the grid in the page even after the grid has
> +// changed.

Obsolete comment. Please make sure it describes the current test.

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-scroll-into-view-of-grid.js:11
(Diff revision 3)
> +  .container {
> +    display: grid;
> +    grid-template-columns: repeat(2, 20vw);
> +    grid-template-rows: repeat(10, 20vw);
> +  }
> +  .container2 {
> +    display: grid;
> +    grid-template-columns: repeat(2, 20vw);
> +    grid-template-rows: repeat(20, 20vw);
> +  }

On test machines and VMs, we don't control how big or small the viewport is going to be.
This test relies on the screen being such that when grid 2 is selected, then grid 1 isn't visible anymore, right?
So to make sure that this works all the time, we should use viewport units. Which you did, so that's great. But you used vw for rows. You should use vh here.

For instance, this will make sure all your grids are always higher than the viewport:

`grid-template-rows: repeat(10, 20vh);`

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-scroll-into-view-of-grid.js:143
(Diff revision 3)
> +  let isInViewport = yield testActor.eval(`
> +    let node = content.document.querySelector("${selector}");
> +    let rect = node.getBoundingClientRect();
> +    (rect.bottom >= 0 && rect.right >= 0 &&
> +      rect.top <= content.innerHeight && rect.left <= content.innerWidth)
> +  `);

It would be better to create a new method in `TestActor` for this rather than using `eval`.
`eval` is fine for very special things we know we're not going to need in the future, but checking if a node is in the viewport seems useful.

So, in the test-actor.js file, in the `TestActor` class, you should create a new method like:

`isInViewport(selector)`.

Note that in this class `selector` can be special because the class has its own `_querySelector` implementation that can be used to target elements inside iframes too, so it's useful.

This way the `checlElementIsInViewport` can just do something like:

```
let isInViewport = yield testActor.isInViewport(selector);
is(isInViewport, expected, ...);
```

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-scroll-into-view-of-grid.js:145
(Diff revision 3)
> +    let rect = node.getBoundingClientRect();
> +    (rect.bottom >= 0 && rect.right >= 0 &&
> +      rect.top <= content.innerHeight && rect.left <= content.innerWidth)

We recently added support for the IntersectionObserver API in Firefox, so I was wondering whether this code could be made better using the new API. But I think it's based on a callback that would only be called async if the viewport was scrolled, so I don't think it's a good idea to use it. Your code seems right.

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-scroll-into-view-of-grid.js:166
(Diff revision 3)
> +function* selectGrid(gridIndex, gridList, highlighters, store) {
> +  let gridCheckBox = gridList.children[gridIndex].querySelector("input");
> +  let onHighlighterShown = highlighters.once("grid-highlighter-shown");
> +  let onGridCheckboxChange = waitUntilState(store, state =>
> +    state.grids[gridIndex].highlighted);
> +  gridCheckBox.click();
> +  yield onHighlighterShown;
> +  yield onGridCheckboxChange;
> +}

It's probably worth moving this to the head.js file. I suspect a lot of other tests do the same thing.
Fine if this is done in another bug though.
Attachment #8902093 - Flags: review?(pbrosset) → review+
Comment on attachment 8902093 [details]
Bug 1373134 - Unit tests.

https://reviewboard.mozilla.org/r/173524/#review180348

> It's probably worth moving this to the head.js file. I suspect a lot of other tests do the same thing.
> Fine if this is done in another bug though.

I can file another bug for moving this piece of code to `head.js` .
Comment on attachment 8902093 [details]
Bug 1373134 - Unit tests.

https://reviewboard.mozilla.org/r/173524/#review180348

> I can file another bug for moving this piece of code to `head.js` .

Here is the link to the bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1396339
Attached patch Bug1373134.patch (obsolete) — — Splinter Review 
Contains implementation of bug.
Attachment #8879372 - Attachment is obsolete: true
Attachment #8900090 - Attachment is obsolete: true
Attachment #8901046 - Attachment is obsolete: true
Attachment #8902093 - Attachment is obsolete: true
Attached patch Bug1373134_tests.patch (obsolete) — — Splinter Review 
Unit tests for implementation.
Attached patch Bug1373134_tests.patch (obsolete) — — Splinter Review 

        Attachment #8903980 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
  
Just for future reference, this is what this bug implements and tests for:

- If you select a grid from the grid list, it will scroll into view of that grid container node on the page 
- If you hold down ctrl and hover over a cell in the outline, it will scroll into view of that cell on the page
- Likewise, you can also click the "Automatically scroll to grid cell" settings option so that you can just hover over a cell in the outline and have it scroll into view without having to hold down the ctrl key.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1373134 - Folded (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8902093 -
        Attachment is obsolete: true

        Attachment #8903979 -
        Attachment is obsolete: true

        Attachment #8903981 -
        Attachment is obsolete: true

        Attachment #8904411 -
        Flags: review+

    
    

    
  
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3873c57a4e3e72a3f0ae43b50fc4114d266d41c8

Changes involved splitting up tests into 3 separate files and using the waitForDOM test utility.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Bug1373134.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Patch with updated tests.

        Attachment #8904411 -
        Attachment is obsolete: true
Severity: normal → enhancement

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Bug1373134.patch
        — 
          — Splinter Review
      

    


  

        Attachment #8909185 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Micah Tigley [:micah] from comment #33)
> Just for future reference, this is what this bug implements and tests for:
> 
> - If you select a grid from the grid list, it will scroll into view of that
> grid container node on the page 
> - If you hold down ctrl and hover over a cell in the outline, it will scroll
> into view of that cell on the page
> - Likewise, you can also click the "Automatically scroll to grid cell"
> settings option so that you can just hover over a cell in the outline and
> have it scroll into view without having to hold down the ctrl key.

Why doesn't it scroll the grid cell into view by clicking on it? That would be more intuitive I guess. (Could show a tooltip when hovering the grid cells explaining the behavior for further clarification.)

Sebastian
Assignee: tigleym → nobody
Status: ASSIGNED → NEW

    
  
Product: Firefox → DevTools

    
  
Severity: normal → S3

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: