Closed Bug 1303171 Opened 5 years ago Closed 3 years ago

Support RTL and Vertical Writing Modes in the Grid Inspector

Categories

(DevTools :: Inspector, enhancement, P1)

enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: gl, Assigned: jryans)

References

(Blocks 2 open bugs)

Details

(Keywords: DevAdvocacy, Whiteboard: [DevRel:P1][designer-tools])

Attachments

(1 file)

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.
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
Summary: Support RTL in the grid highlighter → Support RTL in the Grid Inspector
Blocks: 1347964
Assignee: nobody → zer0
Status: NEW → ASSIGNED
No longer blocks: 1347964
Summary: Support RTL in the Grid Inspector → Support RTL and Vertical Writing Modes in the Grid Inspector
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.
Assignee: zer0 → nobody
Status: ASSIGNED → NEW
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.
(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.
Keywords: DevAdvocacy
Whiteboard: [devRel:P1]
Assignee: nobody → jryans
Priority: P3 → P1
Whiteboard: [devRel:P1] → [DevRel:P1][designer-tools]
Severity: normal → enhancement
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?
Flags: needinfo?(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
Flags: needinfo?(gl)
Status: NEW → ASSIGNED
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 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
Attachment #8941648 - Flags: review?(gl) → review+
Filed follow up bugs:

* Rotate line numbers (bug 1430916)
* Rotate grid outline (bug 1430918)
* Enable WM / RTL support for Grid Inspector (bug 1430919)
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.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d5fce5140351
Adjust highlighters to account for writing mode and text dir. r=gl
https://hg.mozilla.org/mozilla-central/rev/d5fce5140351
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.