1303171 - Support RTL and Vertical Writing Modes in the Grid Inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P1)

Description

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

Our usage of bounds.left and bounds.top in css-grid.js make this not work for different writing directions.

We think this should depend on the horizontal and vertical writing directions, which you can get from the computed style of the currentnode.

Comment 1

[Julian Descottes [:jdescottes]]

STRs:
- go to http://labs.jensimmons.com/examples/grid-content-1.html
- open inspector
- add the attribute dir="rtl" to the HTML element
- open layout panel
- enable grid for <ul> element

=> grid is misplaced

Comment 2

[Jen Simmons [:jensimmons]]

I just created a test page for seeing whether our Grid Inspector only works in a horizontal-tb writing mode, for LTR (which is the current case) — or if it supports all the other ways humans writing languages. Which, sadly, it does not yet.

Inspect the Grids at:
http://labs.jensimmons.com/2017/03-003.html

If you don't know about Writing Modes, here's an article I wrote: https://24ways.org/2016/css-writing-modes/
I also recently did a presentation at CSS Day. The video should be up sometime soon. Or meanwhile, you can look at the slides: http://jensimmons.com/presentation/writing-modes

And of course, I'd be happy to answer any questions about this.

It seems the way the Inspector was engineered, it assumes `writing-mode: horizontal-tb; direction: ltr;`. Which is bad. It should not make such assumptions. 

I hope the demo helps.

Comment 3

[Jen Simmons [:jensimmons]]

We really want to bump this up in priority. I know it doesn't seem exciting, because for "most" use-cases, this effort won't do anything. But it's fundamental to making sure this tool works for everyone, all countries, all languages.

Comment 4

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

(In reply to Jen Simmons [:jensimmons] from comment #3)
> We really want to bump this up in priority. I know it doesn't seem exciting,
> because for "most" use-cases, this effort won't do anything. But it's
> fundamental to making sure this tool works for everyone, all countries, all
> languages.

This is actually the immediate next item we plan on tackling after the negative line numbers.

Comment 5

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

I wrote up various notes on writing mode support:

https://docs.google.com/document/d/13RHNjV6jwq9zBQ0x1U1RNaNWMdesM93_5chgITP6MM8/edit

After some analysis, I believe it makes sense to aim for handling the writing mode client side in the matrix computation.

:gl, could you review the document and let me know if you agree?

Comment 6

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) (all hands, then away until Jan 3) from comment #5)
> I wrote up various notes on writing mode support:
> 
> https://docs.google.com/document/d/
> 13RHNjV6jwq9zBQ0x1U1RNaNWMdesM93_5chgITP6MM8/edit
> 
> After some analysis, I believe it makes sense to aim for handling the
> writing mode client side in the matrix computation.
> 
> :gl, could you review the document and let me know if you agree?

Proposal A sounds good

Comment 7

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

Attachment: Bug 1303171 - Adjust highlighters to account for writing mode and text dir.

The `getCurrentMatrix` function is used by grid and other highlighters to
account for the element's position, transforms, etc.  Here we extend it with
extra logic to account for the element's writing mode and text direction.

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

Comment 8

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

The initial version I submitted appears to work well with Jen's test page:

http://labs.jensimmons.com/2017/03-003.html

All writing mode and direction combinations are handled.  The changes are compatible with other transforms on the element as expected.

Known issues:

* The line numbers are rotated 90 degrees from what would look best with the non-default writing modes
  * I'd suggest looking at this in a separate bug, since there's already active work in this area as part of bug 1396666
* No tests so far
  * I wasn't sure what the best way would be to test highlighter position like we're doing here, but I'm open to suggestions!

Comment 9

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

Comment on attachment 8941648 [details]
Bug 1303171 - Adjust highlighters to account for writing mode and text dir.

https://reviewboard.mozilla.org/r/211884/#review217966

This looks really good and code looks a lot cleaner than I would've imagined. 

Adding to the known issues is that the grid outline the outline you see in the grid panel should probably reflect how the grid looks with writing mode and text direction transformation.

::: devtools/server/actors/highlighters/utils/canvas.js:15
(Diff revision 1)
>    identity,
>    isIdentity,
>    multiply,
>    scale,
>    translate,
> +  rotate,

Move reflectAboutY and rotate after scale to keep this list alphabetically sorted.

::: devtools/server/actors/highlighters/utils/canvas.js:314
(Diff revision 1)
> -  // Finally, we translate the origin based on the node's padding and border values.
> +  // Translate the origin based on the node's padding and border values.
>    currentMatrix = multiply(currentMatrix,
>      translate(paddingLeft + borderLeft, paddingTop + borderTop));
>  
> +  // Adjust as needed to match the writing mode and text direction of the element.
> +  let writingModeMatrix = getWritingModeMatrix(element, computedStyle);

We can probably put a pref around this step 6 to hold it back a bit.

::: devtools/server/actors/highlighters/utils/canvas.js:333
(Diff revision 1)
> + * @param  {CSSStyleDeclaration} computedStyle
> + *         The computed style for the element.
> + * @return {Array}
> + *         The matrix with adjustments for writing mode and text decoration, if any.
> + */
> +function getWritingModeMatrix(element, computedStyle) {

The functions in this file is also alphabetically sorted. Can we move this below getPointsFromDiagonal? (Sorry, this is my OCD to make working with JS somewhat sane)

::: devtools/server/actors/highlighters/utils/canvas.js:381
(Diff revision 1)
> +        rowLength = element.offsetHeight;
> +      }
> +      currentMatrix = multiply(currentMatrix, translate(rowLength, 0));
> +      currentMatrix = multiply(currentMatrix, reflectAboutY());
> +      break;
> +

NIT: unnecessary new line

Comment 10

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

Filed follow up bugs:

* Rotate line numbers (bug 1430916)
* Rotate grid outline (bug 1430918)
* Enable WM / RTL support for Grid Inspector (bug 1430919)

Comment 11

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

Comment on attachment 8941648 [details]
Bug 1303171 - Adjust highlighters to account for writing mode and text dir.

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

Comment 12

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

Comment on attachment 8941648 [details]
Bug 1303171 - Adjust highlighters to account for writing mode and text dir.

https://reviewboard.mozilla.org/r/211884/#review217966

> Move reflectAboutY and rotate after scale to keep this list alphabetically sorted.

Okay, moved.

> We can probably put a pref around this step 6 to hold it back a bit.

Okay, added a pref `devtools.highlighter.writingModeAdjust`.

> The functions in this file is also alphabetically sorted. Can we move this below getPointsFromDiagonal? (Sorry, this is my OCD to make working with JS somewhat sane)

Okay, moved.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5fce5140351
Adjust highlighters to account for writing mode and text dir. r=gl

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d5fce5140351