Closed Bug 1430918 Opened 6 years ago Closed 6 years ago

Rotate grid outline in Layout panel when writing modes / RTL are used

Categories

(DevTools :: Inspector, enhancement, P2)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox59 wontfix, firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

(Whiteboard: [designer-tools])

Attachments

(2 files)

Bug 1430918 - Draw grid outline from origin.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1430918 - Rotate grid outline for writing mode.
59 bytes, text/x-review-board-request
gl
: review+
Details 
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.
Whiteboard: [designer-tools]
Assignee: nobody → jryans
Status: NEW → ASSIGNED
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?
Flags: needinfo?(victoria)
Flags: needinfo?(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.
Flags: needinfo?(gl)
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.
Flags: needinfo?(victoria)
Comment on attachment 8950124 [details]
Bug 1430918 - Draw grid outline from origin.

https://reviewboard.mozilla.org/r/219394/#review225094
Attachment #8950124 - Flags: review?(gl) → review+
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().
Attachment #8950125 - Flags: review?(gl) → review+
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`.
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)
Pushed by jryans@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/dfa8cc56e469
https://hg.mozilla.org/mozilla-central/rev/fc2aa42ecf9d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: