Closed
Bug 1303171
Opened 8 years ago
Closed 7 years ago
Support RTL and Vertical Writing Modes in the Grid Inspector
Categories
(DevTools :: Inspector, enhancement, P1)
DevTools
Inspector
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.
Updated•8 years ago
|
Blocks: devtools/highlighters
Comment 1•8 years ago
|
||
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
Updated•8 years ago
|
Summary: Support RTL in the grid highlighter → Support RTL in the Grid Inspector
Updated•8 years ago
|
Assignee: nobody → zer0
Updated•8 years ago
|
Status: NEW → ASSIGNED
Updated•7 years ago
|
Summary: Support RTL in the Grid Inspector → Support RTL and Vertical Writing Modes in the Grid Inspector
Comment 2•7 years ago
|
||
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.
Reporter | ||
Updated•7 years ago
|
Assignee: zer0 → nobody
Status: ASSIGNED → NEW
Comment 3•7 years ago
|
||
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.
Reporter | ||
Comment 4•7 years ago
|
||
(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.
Updated•7 years ago
|
Keywords: DevAdvocacy
Whiteboard: [devRel:P1]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jryans
Priority: P3 → P1
Whiteboard: [devRel:P1] → [DevRel:P1][designer-tools]
Updated•7 years ago
|
Severity: normal → enhancement
Assignee | ||
Comment 5•7 years ago
|
||
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)
Reporter | ||
Comment 6•7 years ago
|
||
(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)
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
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!
Reporter | ||
Comment 9•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 10•7 years ago
|
||
Filed follow up bugs:
* Rotate line numbers (bug 1430916)
* Rotate grid outline (bug 1430918)
* Enable WM / RTL support for Grid Inspector (bug 1430919)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
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
Comment 14•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•