1430916 - Rotate grid line numbers when writing modes / RTL are used

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

With bug 1303171, grid inspector supports writing modes and RTL.  However, the line numbers are typically 90 degrees off from what would look most natural.

Let's clean that up before enabling grid for WM content.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1430916 - Rotate grid line numbers for writing mode.

Adjust grid highlighter line numbers to rotate to the expected position outside
the grid container even in the presence of writing mode and text direction.

This is also refactors and preserves the recently added behavior to flip line
numbers inside the container if they are near the viewport edge.

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

Comment 2

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

Comment on attachment 8946972 [details]
Bug 1430916 - Rotate grid line numbers for writing mode.

https://reviewboard.mozilla.org/r/216810/#review222908

::: devtools/server/actors/highlighters/css-grid.js:9
(Diff revision 1)
>  
>  "use strict";
>  
> +const Services = require("Services");
> +const DevToolsUtils = require("devtools/shared/DevToolsUtils");
> +

Remove this blank line

::: devtools/server/actors/highlighters/css-grid.js:1200
(Diff revision 1)
> -
>      let minBoxSize = arrowSize * 2 + padding;
>      boxWidth = Math.max(boxWidth, minBoxSize);
>      boxHeight = Math.max(boxHeight, minBoxSize);
>  
> -    let { width, height } = this._winDimensions;
> +    // Determine default box edge.

Add more details about what the box edge is used for

::: devtools/server/actors/highlighters/css-grid.js:1295
(Diff revision 1)
> -        x += textCenterPos;
> +    // Default to drawing outside the edge, but move inside when close to viewport.
> +    let minOffsetFromEdge = OFFSET_FROM_EDGE * displayPixelRatio;
> +    let { width, height } = this._winDimensions;
> +    width *= displayPixelRatio;
> +    height *= displayPixelRatio;
> +    switch (boxEdge) {

Add a new line before this switch statement block

::: devtools/server/actors/highlighters/css-grid.js:1295
(Diff revision 1)
> -        x += textCenterPos;
> +    // Default to drawing outside the edge, but move inside when close to viewport.
> +    let minOffsetFromEdge = OFFSET_FROM_EDGE * displayPixelRatio;
> +    let { width, height } = this._winDimensions;
> +    width *= displayPixelRatio;
> +    height *= displayPixelRatio;
> +    switch (boxEdge) {

Add some more docs about what this switch statement does and what each case does to the x, y and boxEdge because of the minOffsetFromEdge. There are comments that were lost in the refactor that can probably be reused or repurpose.

::: devtools/server/actors/highlighters/css-grid.js:1297
(Diff revision 1)
> +    let { width, height } = this._winDimensions;
> +    width *= displayPixelRatio;
> +    height *= displayPixelRatio;
> +    switch (boxEdge) {
> +      case "left":
> +        if (x < minOffsetFromEdge) {

I think I need you to add a jsdoc for each case.

::: devtools/server/actors/highlighters/css-grid.js:1322
(Diff revision 1)
> +          y -= 2 * padding;
> +        }
> +        break;
> +    }
> +
> +    drawBubbleRect(

Let's not put all these arguments in their own line, and just follow the format as we have for drawBubbleRect.

Comment 3

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

Comment on attachment 8946972 [details]
Bug 1430916 - Rotate grid line numbers for writing mode.

Masterpiece.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8946972 [details]
Bug 1430916 - Rotate grid line numbers for writing mode.

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

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8946972 [details]
Bug 1430916 - Rotate grid line numbers for writing mode.

https://reviewboard.mozilla.org/r/216810/#review223154

::: devtools/server/actors/highlighters/css-grid.js:80
(Diff revision 2)
>  // off parts of the arrow box container.
>  const OFFSET_FROM_EDGE = 25;
>  
> +// Boolean pref to enable adjustment for writing mode and RTL content.
> +DevToolsUtils.defineLazyGetter(this, "WRITING_MODE_ADJUST_ENABLED", () => {
> +  return Services.prefs.getBoolPref("devtools.highlighter.writingModeAdjust");

This doesn't seem related to your commit because the pref existed before, but why do we even have a pref at all?
It seems to me like we just always want this behavior. Is this because we want to let it bake for a while?

::: devtools/server/actors/highlighters/css-grid.js:1216
(Diff revision 2)
> -          if (y + padding > height) {
> -            y -= arrowSize;
> +    function rotateEdgeRight(edge) {
> +      switch (edge) {
> +        case "top": return "right";
> +        case "right": return "bottom";
> +        case "bottom": return "left";
> +        case "left": return "top";
> +        default: return edge;
> -          }
> +      }
> -        }
> +    }
>  
> -        drawBubbleRect(this.ctx, x, y, boxWidth, boxHeight, radius, margin, arrowSize,
> -                       boxAlignment);
> +    function rotateEdgeLeft(edge) {
> +      switch (edge) {
> +        case "top": return "left";
> +        case "right": return "top";
> +        case "bottom": return "right";
> +        case "left": return "bottom";
> +        default: return edge;
> +      }
> +    }
>  
> -        y += textCenterPos;
> +    function reflectEdge(edge) {
> +      switch (edge) {
> +        case "top": return "bottom";
> +        case "right": return "left";
> +        case "bottom": return "top";
> +        case "left": return "right";
> +        default: return edge;
>        }
>      }

There is no need to re-initialize these functions everytime the renderGridLineNumber function is called, these should be functions at the module scope level, not inside this method.
Plus, moving them will make the method shorter/easier to read.

::: devtools/server/actors/highlighters/css-grid.js:1248
(Diff revision 2)
>        }
>      }
>  
> -    if (dimensionType === ROWS) {
> -      if (lineNumber > 0) {
> -        boxAlignment = "left";
> +    // Rotate box edge as needed for writing mode and text direction.
> +    if (WRITING_MODE_ADJUST_ENABLED) {
> +      let computedStyle = this.win.getComputedStyle(this.currentNode);

Please use this helper to get the computed style instead:

`const {getComputedStyle} = require("./utils/markup");`

This way it's more consistent with how other highlighters do it, plus this helper knows how to get style for pseudo elements too, which this simple line doesn't.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8946972 [details]
Bug 1430916 - Rotate grid line numbers for writing mode.

https://reviewboard.mozilla.org/r/216810/#review223154

> This doesn't seem related to your commit because the pref existed before, but why do we even have a pref at all?
> It seems to me like we just always want this behavior. Is this because we want to let it bake for a while?

The pref is previously used in a different module, so this is here to make it available in this module.

When we first added the rotation of the grid highlighter for writing mode in bug 1303171, it was agreed that we should enable _all_ grid WM / RTL support together in the same release: the highlighter, the line numbers, the outline in layout panel.  Bug 1430919 exists to flip it on by default later in 60.

> There is no need to re-initialize these functions everytime the renderGridLineNumber function is called, these should be functions at the module scope level, not inside this method.
> Plus, moving them will make the method shorter/easier to read.

Okay, fair enough.  I only placed them here because they have no connection to other logic in the module outside this function.  Plus :gl will demand that they are sorted alphabetically... :S

> Please use this helper to get the computed style instead:
> 
> `const {getComputedStyle} = require("./utils/markup");`
> 
> This way it's more consistent with how other highlighters do it, plus this helper knows how to get style for pseudo elements too, which this simple line doesn't.

Ah okay, sounds good to me.  (Can a pseudo-element be a grid containers?  Anyway, seems fine to use even if not.)

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8946972 [details]
Bug 1430916 - Rotate grid line numbers for writing mode.

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

Comment 8

[Patrick Brosset <:pbro>]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) (at work week) from comment #6)
> Ah okay, sounds good to me.  (Can a pseudo-element be a grid containers? 
> Anyway, seems fine to use even if not.)
Yes it can, although it's not very useful to do this.

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8946972 [details]
Bug 1430916 - Rotate grid line numbers for writing mode.

https://reviewboard.mozilla.org/r/216810/#review223174

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55030d05dee0
Rotate grid line numbers for writing mode. r=pbro

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/55030d05dee0