Closed Bug 1430916 Opened 3 years ago Closed 3 years ago

Rotate grid line numbers when writing modes / RTL are used

Categories

(DevTools :: Inspector, enhancement, P2)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

(Whiteboard: [designer-tools])

Attachments

(1 file)

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.
Whiteboard: [designer-tools]
Assignee: nobody → jryans
Status: NEW → ASSIGNED
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 on attachment 8946972 [details]
Bug 1430916 - Rotate grid line numbers for writing mode.

Masterpiece.
Attachment #8946972 - Flags: review?(gl)
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.
Attachment #8946972 - Flags: review?(pbrosset)
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.)
(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 on attachment 8946972 [details]
Bug 1430916 - Rotate grid line numbers for writing mode.

https://reviewboard.mozilla.org/r/216810/#review223174
Attachment #8946972 - Flags: review?(pbrosset) → review+
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/55030d05dee0
Rotate grid line numbers for writing mode. r=pbro
https://hg.mozilla.org/mozilla-central/rev/55030d05dee0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.