1430918 - Rotate grid outline in Layout panel 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 the writing mode / RTL support in bug 1303171, the grid highlighter is correctly adjusted to match the content on page, but the grid outline in the Layout panel is still shown in horizontal, LTR mode.

We should rotate the grid outline to match the content.

Comment 1

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

Jen mentioned to me that she's not sure we even want to have a grid outline in the layout panel at all.  If it should be removed, then there's no need to do more work to adjust it for RTL & writing mode.

Victoria and Gabriel, should the grid outline in layout panel be removed?

Comment 2

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

From the HackerYou visit, we did find that people found the grid outline useful so I am hesitant to remove it, and there are many more possibilities that can be unlocked with the grid outline.

Comment 3

[Victoria Wang [:victoria]]

I don't have an opinion on this, but wanted to suggest that if it would help to have a stop-gap solution, we could just hide the grid outline in RTL/writing mode so that it's not displaying incorrect info.

Comment 4

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

Attachment: Bug 1430918 - Draw grid outline from origin.

The transforms that can be applied to the grid outline to adjust for writing
mode may include translation and reflection based on the outline's width or
height.  To achieve the expected result, we need to start drawing the outline at
(0, 0) instead of (1, 1).

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

Comment 5

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

Attachment: Bug 1430918 - Rotate grid outline for writing mode.

Adjust the grid outline in the Inspector's Layout panel as needed to match the
writing mode and text direction of the grid container.

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

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=636b270967c0b5117c56b31c5043d3ca83e3ff11

Comment 7

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

Comment on attachment 8950124 [details]
Bug 1430918 - Draw grid outline from origin.

https://reviewboard.mozilla.org/r/219394/#review225094

Comment 8

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

Comment on attachment 8950125 [details]
Bug 1430918 - Rotate grid outline for writing mode.

https://reviewboard.mozilla.org/r/219396/#review225092

::: devtools/client/inspector/grids/components/GridOutline.js:267
(Diff revision 1)
>        y += height;
>      }
>  
> +    // Transform the cells as needed to match the grid container's writing mode.
> +    let cellGroupStyle = {};
> +    if (WRITING_MODE_ADJUST_ENABLED) {

Add a new line before and after this if statement block

::: devtools/client/inspector/grids/components/GridOutline.js:339
(Diff revision 1)
>      let { color } = grid;
>  
>      return dom.g(
>        {
> -        id: "grid-cell-group",
> -        "className": "grid-cell-group",
> +        id: "grid-outline-group",
> +        "className": "grid-outline-group",

Let's remove the quotations around className and style while we are at it.

::: devtools/client/inspector/grids/grid-inspector.js:339
(Diff revision 1)
>          color,
>          gridFragments: grid.gridFragments,
>          highlighted: nodeFront == this.highlighters.gridHighlighterShown,
>          nodeFront,
> +        writingMode: grid.writingMode,
> +        direction: grid.direction,

Move this after color

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-writing-mode.js:63
(Diff revision 1)
> +    <div id="cellb">Cell B</div>
> +    <div id="cellc">Cell C</div>
> +  </div>
> +`;
> +
> +async function enableGrid(doc, highlighters, store, index) {

Move these helper functions to the bottom of the file

::: devtools/client/inspector/grids/types.js:32
(Diff revision 1)
> +
> +  // The writing mode of the grid container
> +  writingMode: PropTypes.string,
> +
> +  // The text direction of the grid container
> +  direction: PropTypes.string,

Move this after color

::: devtools/server/actors/layout.js:111
(Diff revision 1)
> +
>      let form = {
>        actor: this.actorID,
>        gridFragments: this.gridFragments,
> +      writingMode,
> +      direction,

Move this after actor.

::: devtools/shared/fronts/layout.js:60
(Diff revision 1)
> +   * Added in Firefox 60.
> +   */
> +  get direction() {
> +    if (!this._form.direction) {
> +      return "ltr";
> +    }

Add a new line after this if statement block

::: devtools/shared/fronts/layout.js:78
(Diff revision 1)
> +   * Added in Firefox 60.
> +   */
> +  get writingMode() {
> +    if (!this._form.writingMode) {
> +      return "horizontal-tb";
> +    }

Same as above.

::: devtools/shared/layout/dom-matrix-2d.js:248
(Diff revision 1)
> +  let { width, height } = size;
> +  let { direction, writingMode } = style;
> +
> +  switch (writingMode) {
> +    case "horizontal-tb":
> +      // This is the initial value.  No further adjustment needed.

NIT: Only use one space after the "."

::: devtools/shared/layout/dom-matrix-2d.js:280
(Diff revision 1)
> +      console.error(`Unexpected writing-mode: ${writingMode}`);
> +  }
> +
> +  switch (direction) {
> +    case "ltr":
> +      // This is the initial value.  No further adjustment needed.

NIT: Only use one space after the "."

::: devtools/shared/layout/dom-matrix-2d.js:305
(Diff revision 1)
> + *   a, c, e,
> + *   b, d, f,
> + *   0, 0, 1
> + * to the format used by the `matrix()` CSS transform function:
> + *   a, b, c, d, e, f
> + * @param  {Array} M

NIT: Add a new line to separate the comment and @param

::: devtools/shared/layout/dom-matrix-2d.js:310
(Diff revision 1)
> + * @param  {Array} M
> + *         The matrix in this module's 9 element format.
> + * @return {String}
> + *         The matching 6 element CSS transform function.
> + */
> +function toCSSTransform(M) {

Usually we see Object.prototype.toX() for such a method name. Since this is not the case, I think we should rename this to getCSSTransformMatrix() or getCSSTransform().

Comment 9

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

Comment on attachment 8950125 [details]
Bug 1430918 - Rotate grid outline for writing mode.

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

Comment 10

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

Comment on attachment 8950125 [details]
Bug 1430918 - Rotate grid outline for writing mode.

https://reviewboard.mozilla.org/r/219396/#review225092

> Add a new line before and after this if statement block

Okay, done.

> Let's remove the quotations around className and style while we are at it.

Okay, done.  Also cleaned up a few others in this file.

> Move this after color

Done.

> Move these helper functions to the bottom of the file

Done.

> Move this after color

Done.

> Move this after actor.

Done.

> Add a new line after this if statement block

Done.

> Same as above.

Done.

> NIT: Only use one space after the "."

We seem to have both styles across the tree...  I'll change this for now, but I am not convinced it's worth a review nit.  I filed an RFC (https://github.com/devtools-html/rfcs/issues/37) for further bikeshedding.

> NIT: Add a new line to separate the comment and @param

Done.

> Usually we see Object.prototype.toX() for such a method name. Since this is not the case, I think we should rename this to getCSSTransformMatrix() or getCSSTransform().

Okay, I went with `getCSSMatrixTransform`.

Comment 11

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 23da26d6c7dae528a935625bfde22afbee2487e2 -d c72fcea8dc04: rebasing 446901:23da26d6c7da "Bug 1430918 - Draw grid outline from origin. r=gl"
merging devtools/client/inspector/grids/components/GridOutline.js
rebasing 446902:95d6d7e99190 "Bug 1430918 - Rotate grid outline for writing mode. r=gl" (tip)
merging devtools/client/inspector/grids/components/GridOutline.js
merging devtools/server/actors/layout.js
warning: conflicts while merging devtools/client/inspector/grids/components/GridOutline.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 12

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

Comment on attachment 8950124 [details]
Bug 1430918 - Draw grid outline from origin.

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

Comment 13

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

Comment on attachment 8950125 [details]
Bug 1430918 - Rotate grid outline for writing mode.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfa8cc56e469
Draw grid outline from origin. r=gl
https://hg.mozilla.org/integration/autoland/rev/fc2aa42ecf9d
Rotate grid outline for writing mode. r=gl

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/dfa8cc56e469
https://hg.mozilla.org/mozilla-central/rev/fc2aa42ecf9d